feature/projects-all-the-way-down #3323

Merged
konrad merged 123 commits from feature/projects-all-the-way-down into main 2023-05-30 10:09:40 +00:00
3 changed files with 0 additions and 13 deletions
Showing only changes of commit bbaddb9406 - Show all commits

View File

@ -18,7 +18,6 @@ export interface IProject extends IAbstract {
subscription: ISubscription
position: number
backgroundBlurHash: string
childProjectIds: number[]
parentProjectId: number

This is really error prone. In the moment the project including child projects is coming from the backend we should replace this with an array of childProjects instead. All projects should be included in a flat id-Map inside the store.
Having hierarchies just adds complexity everywhere. If a component wants to use a list it can get it directly from the store.

This is really error prone. In the moment the project including child projects is coming from the backend we should replace this with an array of `childProjects` instead. All projects should be included in a flat id-`Map` inside the store. Having hierarchies just adds complexity everywhere. If a component wants to use a list it can get it directly from the store.

we should replace this with an array of childProjects instead.

What's that? The projects in childProjects are just regular projects.

> we should replace this with an array of `childProjects` instead. What's that? The projects in `childProjects` are just regular projects.

Coming from the api – yes I know. But using them like this in the frontend – not a good pattern. I know we did this similar in other places already. But here we this is the first time for projects. Because of that I would like to prevent the introduction of this pattern here. So when we get the regular projects child array from the api we replace them with their ids instead and add the child projects to the general projects list in the store. That general list is a list of all projects – including all child projects. If we just want the root projects we can create a filter for projects that don't have a parent.

Coming from the api – yes I know. But using them like this in the frontend – not a good pattern. I know we did this similar in other places already. But here we this is the first time for projects. Because of that I would like to prevent the introduction of this pattern here. So when we get the regular projects child array from the api we replace them with their ids instead and add the child projects to the general projects list in the store. That general list is a list of __all__ projects – including all child projects. If we just want the root projects we can create a filter for projects that don't have a parent.

Adding to this – of course we have to reverse this when we send stuff back to the api. By the way for all of this zod is really helpfull…

Adding to this – of course we have to reverse this when we send stuff back to the api. By the way for all of this zod is really helpfull…

We already save all projects in the store, regardless of whether they are a child or not. The navigation then starts with a filtered list of that.

Maybe we could just ignore childProjects completely and only use parentProjectId? And then build the list of child projects dynamically when needed? Not sure about the performance implications here.

We already save all projects in the store, regardless of whether they are a child or not. The navigation then starts with a filtered list of that. Maybe we could just ignore `childProjects` completely and only use `parentProjectId`? And then build the list of child projects dynamically when needed? Not sure about the performance implications here.

Maybe we could just ignore childProjects completely and only use parentProjectId? And then build the list of child projects dynamically when needed? Not sure about the performance implications here.

This is basically the idea I have only in the reverse direction. It would be better to have the id of the child though because than we don't have to iterate through all projects everytime we want to find all childs. Instead we can get via the id which should be much faster.

I wouldn't ignore the childProjects because the data inside would need to be manually synced. Instead that property should not exist in the frontend data model.

> Maybe we could just ignore childProjects completely and only use parentProjectId? And then build the list of child projects dynamically when needed? Not sure about the performance implications here. This is basically the idea I have only in the reverse direction. It would be better to have the id of the child though because than we don't have to iterate through all projects everytime we want to find all childs. Instead we can get via the id which should be much faster. I wouldn't ignore the childProjects because the data inside would need to be manually synced. Instead that property should not exist in the frontend data model.

See normalizr. The utility is unmaintained but the examples are still valid. Since our store is flux inspired this applies to use as well.

See [normalizr](https://github.com/paularmstrong/normalizr/tree/a213bbc6e7bf328cdd46f61a3367b952dc5f30da). The utility is unmaintained but the examples are still valid. Since our store is flux inspired this applies to use as well.

I wouldn't ignore the childProjects because the data inside would need to be manually synced. Instead that property should not exist in the frontend data model.

The api only uses the childProjects property when returning a response with all projects. It won't use it to update the parent <-> child relation.

> I wouldn't ignore the childProjects because the data inside would need to be manually synced. Instead that property should not exist in the frontend data model. The api only uses the `childProjects` property when returning a response with all projects. It won't use it to update the parent <-> child relation.

It would be better to have the id of the child though because than we don't have to iterate through all projects everytime we want to find all childs. Instead we can get via the id which should be much faster.

Makes sense, I wonder how good that would work with the dragging and dropping of projects though.

> It would be better to have the id of the child though because than we don't have to iterate through all projects everytime we want to find all childs. Instead we can get via the id which should be much faster. Makes sense, I wonder how good that would work with the dragging and dropping of projects though.

The api only uses the childProjects property when returning a response with all projects. It won't use it to update the parent <-> child relation.

Okay that means that we could simply return ids of the childProjects via a new property (e.g.) childProjectIds? That would make that step from the API obsolete.

I wonder how good that would work with the dragging and dropping of projects though.

Why would that be a problem?

> The api only uses the `childProjects` property when returning a response with all projects. It won't use it to update the parent <-> child relation. Okay that means that we could simply return ids of the childProjects via a new property (e.g.) `childProjectIds`? That would make that step from the API obsolete. > I wonder how good that would work with the dragging and dropping of projects though. Why would that be a problem?

Note that the store still could provide a getChildProjects computed that would return a function where you can pass in the id of a project and would get a reactive list of child projects.

Note that the store still could provide a `getChildProjects` computed that would return a function where you can pass in the id of a project and would get a reactive list of child projects.

Okay that means that we could simply return ids of the childProjects via a new property (e.g.) childProjectIds?

I think we should be able to, yes.

Would you add that as a new property to the Project Model and then map it out in the constructor?

That would make that step from the API obsolete.

Not really, since we need to fetch all children anyway.

> Okay that means that we could simply return ids of the childProjects via a new property (e.g.) childProjectIds? I think we should be able to, yes. Would you add that as a new property to the Project Model and then map it out in the constructor? > That would make that step from the API obsolete. Not really, since we need to fetch all children anyway.

Note that the store still could provide a getChildProjects computed that would return a function where you can pass in the id of a project and would get a reactive list of child projects.

Should that include the children of children (of children... and so on) as well?

> Note that the store still could provide a getChildProjects computed that would return a function where you can pass in the id of a project and would get a reactive list of child projects. Should that include the children of children (of children... and so on) as well?

I started moving this from the current implementation to one where the store only has a flat map and does not store the children directly. It works for the basics, but I could not get any version of drag n' drop to work with that. Not sure what I did wrong.

One problem is the api returns the projects already in the child projects structure. This makes it easy to handle it as such when parsing the data from the api.

Another issue I have with that approach is how it requires a new ref in the projects navigation component, which holds all children for each project of the current tree. That's the same as a property of the Projects model, but feels a lot hackier.

Here's what I did:

Index: src/modelTypes/IProject.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/modelTypes/IProject.ts b/src/modelTypes/IProject.ts
--- a/src/modelTypes/IProject.ts	(revision 0d0b3c0ca7ac1a1894a4c49091f7b138df4f9818)
+++ b/src/modelTypes/IProject.ts	(date 1680095041514)
@@ -18,7 +18,8 @@
 	subscription: ISubscription
 	position: number
 	backgroundBlurHash: string
-	childProjects: IProject[] | null
+	// childProjects: IProject[] | null
+	childProjectIds: number[]
 	parentProjectId: number
 	
 	created: Date
Index: src/components/home/ProjectsNavigationWrapper.vue
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/components/home/ProjectsNavigationWrapper.vue b/src/components/home/ProjectsNavigationWrapper.vue
--- a/src/components/home/ProjectsNavigationWrapper.vue	(revision 0d0b3c0ca7ac1a1894a4c49091f7b138df4f9818)
+++ b/src/components/home/ProjectsNavigationWrapper.vue	(date 1680095818765)
@@ -24,7 +24,6 @@
 	.filter(p => !p.isArchived && p.isFavorite)
 	.map(p => ({
 		...p,
-		childProjects: [],
 	}))
 	.sort((a, b) => a.position - b.position))
 </script>
Index: src/components/home/ProjectsNavigation.vue
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/components/home/ProjectsNavigation.vue b/src/components/home/ProjectsNavigation.vue
--- a/src/components/home/ProjectsNavigation.vue	(revision 0d0b3c0ca7ac1a1894a4c49091f7b138df4f9818)
+++ b/src/components/home/ProjectsNavigation.vue	(date 1680095997699)
@@ -26,7 +26,7 @@
 			>
 				<section>
 					<BaseButton
-						v-if="p.childProjects.length > 0"
+						v-if="childProjects[p.id].length > 0"
 						@click="collapsedProjects[p.id] = !collapsedProjects[p.id]"
 						class="collapse-project-button"
 					>
@@ -67,7 +67,7 @@
 				</section>
 				<ProjectsNavigation
 					v-if="!collapsedProjects[p.id]"
-					v-model="p.childProjects"
+					v-model="childProjects[p.id]"
 					:can-edit-order="true"
 				/>
 			</li>
@@ -114,11 +114,15 @@
 // TODO: child projects
 const collapsedProjects = ref<{ [id: IProject['id']]: boolean }>({})
 const availableProjects = ref<IProject[]>([])
+const childProjects = ref<{ [id: IProject['id']]: boolean }>({})
 watch(
 	() => props.modelValue,
 	projects => {
 		availableProjects.value = projects
-		projects.forEach(p => collapsedProjects.value[p.id] = false)
+		projects.forEach(p => {
+			collapsedProjects.value[p.id] = false
+			childProjects.value[p.id] = projectStore.getChildProjects(p.id)
+		})
 	},
 	{immediate: true},
 )
@@ -149,8 +153,8 @@
 
 	if (project.parentProjectId !== parentProjectId && project.parentProjectId > 0) {
 		const parentProject = projectStore.getProjectById(project.parentProjectId)
-		const childProjectIndex = parentProject.childProjects.findIndex(p => p.id === project.id)
-		parentProject.childProjects.splice(childProjectIndex, 1)
+		const childProjectIndex = parentProject.childProjectIds.findIndex(pId => pId === project.id)
+		parentProject.childProjectIds.splice(childProjectIndex, 1)
 	}
 
 	try {
Index: src/stores/projects.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/stores/projects.ts b/src/stores/projects.ts
--- a/src/stores/projects.ts	(revision 0d0b3c0ca7ac1a1894a4c49091f7b138df4f9818)
+++ b/src/stores/projects.ts	(date 1680096142590)
@@ -34,6 +34,9 @@
 	const getProjectById = computed(() => {
 		return (id: IProject['id']) => typeof projects.value[id] !== 'undefined' ? projects.value[id] : null
 	})
+	const getChildProjects = computed(() => {
+		return (id: IProject['id']) => projectsArray.value.filter(p => p.parentProjectId === id)
+	})
 
 	const findProjectByExactname = computed(() => {
 		return (name: string) => {
@@ -67,24 +70,24 @@
 		}
 
 		if (updateChildren) {
-			project.childProjects?.forEach(p => setProject(p))
+			project.childProjects?.forEach(p => setProject(new ProjectModel(p)))
 		}
 
 		// if the project is a child project, we need to also set it in the parent
 		if (project.parentProjectId) {
 			const parent = projects.value[project.parentProjectId]
 			let foundProject = false
-			parent.childProjects = parent.childProjects?.map(p => {
+			parent.childProjectIds = parent.childProjectIds?.forEach(p => {
 				if (p.id === project.id) {
 					foundProject = true
-					return project
 				}
-
-				return p
 			})
 			// If we hit this, the project now has a new parent project which it did not have before
 			if (!foundProject) {
-				parent.childProjects.push(project)
+				if (!parent.childProjectIds) {
+					parent.childProjectIds = []
+				}
+				parent.childProjectIds.push(project.id)
 			}
 			setProject(parent, false)
 		}
@@ -197,6 +200,7 @@
 		hasProjects: readonly(hasProjects),
 
 		getProjectById,
+		getChildProjects,
 		findProjectByExactname,
 		searchProject,
 
Index: src/models/project.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/models/project.ts b/src/models/project.ts
--- a/src/models/project.ts	(revision 0d0b3c0ca7ac1a1894a4c49091f7b138df4f9818)
+++ b/src/models/project.ts	(date 1680096142588)
@@ -22,7 +22,7 @@
 	subscription: ISubscription = null
 	position = 0
 	backgroundBlurHash = ''
-	childProjects = []
+	childProjectIds = []
 	parentProjectId = 0
 	
 	created: Date = null
@@ -47,7 +47,8 @@
 			this.subscription = new SubscriptionModel(this.subscription)
 		}
 		
-		this.childProjects = this.childProjects.map(p => new ProjectModel(p))
+		// debugger
+		this.childProjectIds = this.childProjects?.map(p => p.id) || []
 
 		this.created = new Date(this.created)
 		this.updated = new Date(this.updated)
I started moving this from the current implementation to one where the store only has a flat map and does not store the children directly. It works for the basics, but I could not get any version of drag n' drop to work with that. Not sure what I did wrong. One problem is the api returns the projects already in the child projects structure. This makes it easy to handle it as such when parsing the data from the api. Another issue I have with that approach is how it requires a new ref in the projects navigation component, which holds all children for each project of the current tree. That's the same as a property of the `Projects` model, but feels a lot hackier. Here's what I did: ```patch Index: src/modelTypes/IProject.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/src/modelTypes/IProject.ts b/src/modelTypes/IProject.ts --- a/src/modelTypes/IProject.ts (revision 0d0b3c0ca7ac1a1894a4c49091f7b138df4f9818) +++ b/src/modelTypes/IProject.ts (date 1680095041514) @@ -18,7 +18,8 @@ subscription: ISubscription position: number backgroundBlurHash: string - childProjects: IProject[] | null + // childProjects: IProject[] | null + childProjectIds: number[] parentProjectId: number created: Date Index: src/components/home/ProjectsNavigationWrapper.vue IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/src/components/home/ProjectsNavigationWrapper.vue b/src/components/home/ProjectsNavigationWrapper.vue --- a/src/components/home/ProjectsNavigationWrapper.vue (revision 0d0b3c0ca7ac1a1894a4c49091f7b138df4f9818) +++ b/src/components/home/ProjectsNavigationWrapper.vue (date 1680095818765) @@ -24,7 +24,6 @@ .filter(p => !p.isArchived && p.isFavorite) .map(p => ({ ...p, - childProjects: [], })) .sort((a, b) => a.position - b.position)) </script> Index: src/components/home/ProjectsNavigation.vue IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/src/components/home/ProjectsNavigation.vue b/src/components/home/ProjectsNavigation.vue --- a/src/components/home/ProjectsNavigation.vue (revision 0d0b3c0ca7ac1a1894a4c49091f7b138df4f9818) +++ b/src/components/home/ProjectsNavigation.vue (date 1680095997699) @@ -26,7 +26,7 @@ > <section> <BaseButton - v-if="p.childProjects.length > 0" + v-if="childProjects[p.id].length > 0" @click="collapsedProjects[p.id] = !collapsedProjects[p.id]" class="collapse-project-button" > @@ -67,7 +67,7 @@ </section> <ProjectsNavigation v-if="!collapsedProjects[p.id]" - v-model="p.childProjects" + v-model="childProjects[p.id]" :can-edit-order="true" /> </li> @@ -114,11 +114,15 @@ // TODO: child projects const collapsedProjects = ref<{ [id: IProject['id']]: boolean }>({}) const availableProjects = ref<IProject[]>([]) +const childProjects = ref<{ [id: IProject['id']]: boolean }>({}) watch( () => props.modelValue, projects => { availableProjects.value = projects - projects.forEach(p => collapsedProjects.value[p.id] = false) + projects.forEach(p => { + collapsedProjects.value[p.id] = false + childProjects.value[p.id] = projectStore.getChildProjects(p.id) + }) }, {immediate: true}, ) @@ -149,8 +153,8 @@ if (project.parentProjectId !== parentProjectId && project.parentProjectId > 0) { const parentProject = projectStore.getProjectById(project.parentProjectId) - const childProjectIndex = parentProject.childProjects.findIndex(p => p.id === project.id) - parentProject.childProjects.splice(childProjectIndex, 1) + const childProjectIndex = parentProject.childProjectIds.findIndex(pId => pId === project.id) + parentProject.childProjectIds.splice(childProjectIndex, 1) } try { Index: src/stores/projects.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/src/stores/projects.ts b/src/stores/projects.ts --- a/src/stores/projects.ts (revision 0d0b3c0ca7ac1a1894a4c49091f7b138df4f9818) +++ b/src/stores/projects.ts (date 1680096142590) @@ -34,6 +34,9 @@ const getProjectById = computed(() => { return (id: IProject['id']) => typeof projects.value[id] !== 'undefined' ? projects.value[id] : null }) + const getChildProjects = computed(() => { + return (id: IProject['id']) => projectsArray.value.filter(p => p.parentProjectId === id) + }) const findProjectByExactname = computed(() => { return (name: string) => { @@ -67,24 +70,24 @@ } if (updateChildren) { - project.childProjects?.forEach(p => setProject(p)) + project.childProjects?.forEach(p => setProject(new ProjectModel(p))) } // if the project is a child project, we need to also set it in the parent if (project.parentProjectId) { const parent = projects.value[project.parentProjectId] let foundProject = false - parent.childProjects = parent.childProjects?.map(p => { + parent.childProjectIds = parent.childProjectIds?.forEach(p => { if (p.id === project.id) { foundProject = true - return project } - - return p }) // If we hit this, the project now has a new parent project which it did not have before if (!foundProject) { - parent.childProjects.push(project) + if (!parent.childProjectIds) { + parent.childProjectIds = [] + } + parent.childProjectIds.push(project.id) } setProject(parent, false) } @@ -197,6 +200,7 @@ hasProjects: readonly(hasProjects), getProjectById, + getChildProjects, findProjectByExactname, searchProject, Index: src/models/project.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/src/models/project.ts b/src/models/project.ts --- a/src/models/project.ts (revision 0d0b3c0ca7ac1a1894a4c49091f7b138df4f9818) +++ b/src/models/project.ts (date 1680096142588) @@ -22,7 +22,7 @@ subscription: ISubscription = null position = 0 backgroundBlurHash = '' - childProjects = [] + childProjectIds = [] parentProjectId = 0 created: Date = null @@ -47,7 +47,8 @@ this.subscription = new SubscriptionModel(this.subscription) } - this.childProjects = this.childProjects.map(p => new ProjectModel(p)) + // debugger + this.childProjectIds = this.childProjects?.map(p => p.id) || [] this.created = new Date(this.created) this.updated = new Date(this.updated)

I got something working! See 2557b182dd

There are a few cases where the performance is really bad but I didn't manage to reproduce that reliably (let alone profile it).

I got something working! See 2557b182dde8f40f4be903e65c0485d46c5a185a There are a few cases where the performance is really bad but I didn't manage to reproduce that reliably (let alone profile it).
created: Date

View File

@ -22,7 +22,6 @@ export default class ProjectModel extends AbstractModel<IProject> implements IPr
subscription: ISubscription = null
position = 0
backgroundBlurHash = ''
childProjectIds = []
parentProjectId = 0

Coming from the recent discussion: wouldn't it be better if this was undefined or null?

Coming from the recent discussion: wouldn't it be better if this was `undefined` or `null`?

You mean to make it clear if this was not set, since it will be overridden with 0 by the api anyway?

You mean to make it clear if this was not set, since it will be overridden with 0 by the api anyway?

Okay, wasn't aware of this. So the 0 here is coming from the golang format. I think we should use the equivalent in js for the frontend, similar to how we use camelCase for variable names.

EDIT:
Keeping this unresolved because I still want to check something.

Okay, wasn't aware of this. So the `0` here is coming from the golang format. I think we should use the equivalent in js for the frontend, similar to how we use camelCase for variable names. **EDIT:** Keeping this `unresolved` because I still want to check something.
created: Date = null
@ -47,8 +46,6 @@ export default class ProjectModel extends AbstractModel<IProject> implements IPr
this.subscription = new SubscriptionModel(this.subscription)
}
this.childProjectIds = this.childProjects?.map(p => p.id) || []
this.created = new Date(this.created)
this.updated = new Date(this.updated)

If we receive child projects like this from the server: Do we have that info somewhere else?

If we receive child projects like this from the server: Do we have that info somewhere else?

We don't actually receive them like this anymore, so this is obsolete, and I've removed it.

Each project has a parent project id. To get all child projects we need to iterate over them and return all projects with a parent project id of the project we're interested in. We don't need to that anywhere so I think it's fine to leave at that.

We don't actually receive them like this anymore, so this is obsolete, and I've removed it. Each project has a parent project id. To get all child projects we need to iterate over them and return all projects with a parent project id of the project we're interested in. We don't need to that anywhere so I think it's fine to leave at that.
}

View File

@ -110,15 +110,6 @@ export const useProjectStore = defineStore('project', () => {
const cancel = setModuleLoading(setIsLoading)
const projectService = new ProjectService()
const oldProject = projects.value[project.id]
if (oldProject && oldProject.parentProjectId !== project.parentProjectId && oldProject.parentProjectId > 0) {
const parentProject = projects.value[oldProject.parentProjectId]
if (parentProject) {
const childProjectIndex = parentProject.childProjectIds.findIndex(pId => pId === project.id)
parentProject.childProjectIds.splice(childProjectIndex, 1)
}
}
try {
await projectService.update(project)
setProject(project)