Tree Data Grid x Parent/Child Relation Dataset x Failures on subsequent component mount

[tnaum-ms]

I'm currently investigating an issue that arises when I mount my SlickGrid Tree Data as a React component for the second time. The problem occurs when I toggle between different data view formats in my application. Specifically, switching away from the Tree Data view and then returning to it results in a failure of the Tree Data Component.

The data set I’m working with follows a Parent/Child structure, where the id and parentId fields are of type string.
(It would be helpful to include a note in the documentation indicating the preferred or faster data input method.)

The SlickGrid Tree Data Grid throws the following error:

Each data element must implement a unique 'id' property

Upon investigation, I discovered that on the second mount (not just a second render), one of the internal data mutation functions introduces duplicates into my data set. This causes the ensureIdUniqueness function to fail.

The issue seems to be related to the function located here:

slickgrid-universal/packages/common/src/services/utilities.ts

Line 158 in ce8b400

export function unflattenParentChildArrayToTree<P, T extends P & { [childrenPropName: string]: P[]; }>(flatArray: P[], options?: { aggregators?: Aggregator[]; childrenPropName?: string; collapsedPropName?: string; identifierPropName?: string; levelPropName?: string; parentPropName?: string; initiallyCollapsed?: boolean; }): T[] {

This specific loop appears to be introducing duplicates:

slickgrid-universal/packages/common/src/services/utilities.ts

Lines 173 to 187 in ce8b400

	Object.keys(all).forEach((id) => {
	const item = all[id];
	if (!(parentPropName in item) || item[parentPropName] === null || item[parentPropName] === undefined || item[parentPropName] === '') {
	roots.push(item);
	} else if (item[parentPropName] in all) {
	const p = all[item[parentPropName]];
	if (!(childrenPropName in p)) {
	p[childrenPropName] = [];
	}
	p[childrenPropName].push(item);
	if (p[collapsedPropName] === undefined) {
	p[collapsedPropName] = options?.initiallyCollapsed ?? false;
	}
	}
	});

I'm continuing to investigate the root cause, but since this issue only occurs on the second mount, I haven’t yet identified any obvious side effects. If this is something you’ve encountered before, I would greatly appreciate any insights or suggestions you could share.

Thank you for your assistance.
Best, Tomasz

[ghiscoding]

Hmm I was hoping to never touch this code again, it took me near 6 months to get Tree Data working. I agree that calling this function more than once shouldn't impact its final array or should at the very least also provide the same output. It is important to note that the function does mutate the input array as it was mentioned in the JSDocs, but yes the result should always be the same and not add extra rows on the second run. But even then, the question would be why are you mounting it twice? Can't you make sure that you only mount the component once and avoid performing the same execution? I'm saying this because dealing with the Tree Data multiple time can be costly, I think I already mentioned that in your other discussion but... SlickGrid doesn't support Tree Data by itself, it can only deal with flat data, so the way I got it working was to keep copies of both the flat and hierarchical arrays and then keep then in sync whenever a CRUD operation comes in. So having to recreate both arrays (flat/hierarchical) multiple times is not something you should do too often, it's costly in terms of performance

are you able to provide copies of the data? I think the first troubleshooting would be to see what happens when you run this function twice on the same array, I'm assuming the 2nd run modifies the array. If you can't provide the data, then you could try to troubleshoot it on your side by running the same function unflattenParentChildArrayToTree() twice.

oh and yeah I saw you mentioned Hello Team earlier, there's no team here, it's just me, welcome to the world of open source 😅

[ghiscoding]

Maybe just a check to see if it's already in the children array? Something along these lines (untested).

const idx = p[childrenPropName]?.findIndex(x => x.id === item.id);
if (idx < 0) {
  p[childrenPropName].push(item);
} else {
  p[childrenPropName][idx] = item; // replace existing one (probably equal to the same item)
}

I don't think I could use a Set because this must be a simple array, going with a Set would be at least a breaking change since it would be different on the user side (unless we convert it back to an array but that's extra work that I try to avoid for obvious perf reasons... mutating the original array is also for perf reasons)

BTW, there's another push here that would require the same kind of changes for the items in the root

slickgrid-universal/packages/common/src/services/utilities.ts

Lines 175 to 177 in ce8b400

	if (!(parentPropName in item) || item[parentPropName] === null || item[parentPropName] === undefined || item[parentPropName] === '') {
	roots.push(item);
	} else if (item[parentPropName] in all) {

[tnaum-ms]

After learning about the side effects on my data and the children array, I was able to work around it in my application code with something as simple as this:

Perhaps a workaround like this could be recommended for corner cases like mine, avoiding the need for another lookup in the codebase? While this might not help those who don't read the docs ;) it could be beneficial to add a mention in the tree data example source code.

By the way, I'm not too concerned about roots since it's a local variable that gets initialized as empty here:

slickgrid-universal/packages/common/src/services/utilities.ts

Line 165 in ce8b400

const roots: T[] = []; // items without parent which at the root

[ghiscoding]

By the way, I'm not too concerned about roots since it's a local variable that gets initialized as empty here:

oh yes you're right on that one. I think my suggestion still makes sense though compare to your function, because your suggestion requires to again loop through every single rows of your dataset which is still not ideal on the perf side.

I think an easier patch for you, until we implement my suggestion above, would be to simply keep a deep copy of your original array and simply reapply this copy every time you mount (it should be faster than deleting all children). For the deep copy, you can use some of my utils for this, the extend can work (which is node-extend and is the same implementation as jQuery extend). const arrayCopy = extend(true, [], originalArray)... but that keeps another copy in memory (not ideal).
BUT wait, that's not actually a good suggestion if you do CRUD stuff on your data, then you're out of sync, so forget the suggestion :D

so up to you, at least you have more than 1 ways to go around the issue. I would still like to try my suggestion above and push a PR with this if it makes sense

[ghiscoding]

@tnaum-ms So I gave a try to my previous suggestion above and it works exactly like it should even after calling the unflattenParentChildArrayToTree() function multiple times.

The way I've tested this was to first modify my unit tests to call unflattenParentChildArrayToTree() twice and as expected the array is getting duplicate items (the issue that you mentioned above). I then modified the code with my suggested fix and then called the unflatten function twice or even multiple times and it now always produce the same result which fixes the issue you've mentioned. 🎉

The fix is in PR #1658 (merged) and will be released next weekend via a patch version (done).

Thanks a lot for all the troubleshooting that you've done, this helped quite a lot to identify the problem and bring a fix for this bug. 😃

[ghiscoding]

The fix is now available everywhere. Cheers