Add ITreeNode.userData property.

[yaron-lavi-deel]

No description provided.

[palantirtech]

Thanks for your interest in palantir/blueprint, @yaronlavi! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request.

[adidahiya]

I don't think this is necessary. See discussion in #1989

[yaron-lavi-deel]

I'm sorry, but I disagree:

• any decent tree component has the ability to attach custom data to each node, this is a necessity to every production level application.
• I was puzzled the tree component doesn't have it. I was asked to do a PR on this, which I did and it passed all tests.
• the change is practically harmless, but will greatly amplify the tree component capabilities.
• I know can hold my own data objects against the data node ID, but this simply complicates a simple and common solution.

Please reconsider.

[adidahiya]

What does the complex code look like when you have to maintain your own data structure for node-level data? How are you constructing the tree in the first place?

[yaron-lavi-deel]

That’s easy: - each node represents a different entity in my app, so I’d be storing like a node type enumeration. - second, some node user data will contain direct object pointers to the objects their represent, instead me holding an node-object lookup. The tree is constructed by calling several business API, and building nodes according to the results. Each node represents a different object entity. I then hold nodes in state, including their expand/collapse state. It’s possible of course at this stage to construct yet another node-object lookup, but holding a simple object ref per each tree node is much more elegant, efficient and straight forward. Thanks, Yaron

[giladgray]

@yaronlavi: the proposal on #1989 to simply maintain a separate dictionary keyed by node ID seems more than sufficient for this use case. that's arguably a better approach than including userData in the contents prop tree, which is already a massive, unwieldy data structure.

[yaron-lavi-deel]

As I wrote before to Adi: - the ability to store user data per node is common, and critical to any component being used with production-level web apps. - everything can be self-developed :), however we all use components to focus on the business and not on the “plumbing”. I don’t see any serious app not needing this. - the size of contents is irrelevant here; a tree by definition is a large component and I’ve evaluated a few. I love the tree and blueprint in general, but in order to use conveniently and elegantly this feature is really missing. Please reconsider. Thanks, Yaron

[giladgray]

our proposal:

const userData: Record<string, IData>;
const contents: ITreeNode[];

<Tree contents={contents} onNodeClick={onClick} />

function onClick(node) {
    console.log(userData[node.id]);
}

your proposal:

const contents: ITreeNode[];

<Tree contents={contents} onNodeClick={onClick} />

function onClick(node) {
    console.log(node.userData as IData);
}

the difference is negligible. i don't see how this isn't already supported trivially.

followup: our proposal is inherently more type-safe as it avoids declaring an any type.

[yaron-lavi-deel]

Not at all. One is held in the tree and is integral part. The second is glueing both artificially. The difference is in concept and component design, not code per-se. If this is so troubling you guys, you can start by removing pros like node.path or node.depth which are mandatory when supplying nodes for some reason but seems useless... Thanks, Yaron

[izikorgad]

Hi,
In my opinion, the way @yaronlavi suggested gives a whole solution within the component itself.
Of curse, this can be implemented as external lookup data structure, but it would be much more robust to have it as part of a well-defined components props.

I have prior experience with other Tree-like controls, and that was the common interface there as well.

Thanks,
Izik

[msydoron]

Reading the above, @yaronlavi suggestion seems more like it. Up-vote from me!

[jkillian]

followup: our proposal is inherently more type-safe as it avoids declaring an any type.

I agree with @giladgray here, if we were to even consider userData, we'd need a way to make it typed. We'd have to propagate a generic type down from the Tree component.

This is clunky in React, I assume it'd have to be something like this:

blueprint/packages/select/src/components/select/select.tsx

Lines 117 to 119 in e2d8317

	public static ofType<T>() {
	return Select as new () => Select<T>;
	}

[adidahiya]

Yeah I agree with @jkillian, I'd prefer to use a generic type rather than any. We could do this in a non-breaking way with generic parameter defaults in TS 2.3. Tree and TreeNode would have to use the same type constraint, so ofType<T> should return both a Tree and TreeNode component. We should return userData in Tree's various onNode* event handlers and get rid of path: number[].

Lastly, I think we should rename userData to nodeData.

[adidahiya]

Open to this overall idea & goal, but requesting changes as noted above ^

[giladgray]

@yaronlavi status here? we're open to the overall idea and have left some comments with next steps.

[yaron-lavi-deel]

A generic type for nodeData is welcomed, although I didn't understand how to combine ofType with my solution.

1. Did you mean that instead of nodeData: any it'll be defined as nodeData<T>?
2. What if a user does not want to use nodeData at all? will he be forced to pass in a type T for the tree and tree node?

[adidahiya]

1. yes
2. you can set a default for the generic type parameter so that users who don't need it won't have to specify T. use nodeData: T where the interface is defined as ITreeNode<T = {}>, similar to queryList:

   blueprint/packages/select/src/components/query-list/queryList.tsx

   Line 15 in ff76c4a

   items: T[];

[giladgray]

closing this PR as stale.

@yaronlavi you know what to do when you're ready to revisit this!

[izikorgad]

Hi,
I've submit PR that implements the generic nodeData concept.
Please review #2132.