Expansion of the `metadata` object

[Pearce-Ropion]

Describe the solution you'd like

As per my comment here, I think we can improve/expand on the metadata object that was recently added to tree nodes.

In general, metadata is used to provide additional static properties that can be used to provide unique rendering or functionality to the given node. In many (similar) libraries, we can also use this metadata object to pass mutable data between rerenders. With this in mind, the current scope of the metadata object is too limited. Therefore, I propose that we do 2 things:

1. Do not clone metadata in the flattenTree function
2. Allow any object (keys/properties) as metadata

1. Do not clone metadata

As a user, I would expect that metadata object that I pass into a node would be the same object before and after the flattenTree call. In most javascript/typescript libraries, it is generally expected that if you pass an object to a function, then that object is mutable. If you (the user) don't want your data to be mutable, then you (the user) should handle the immutability yourself.

Therefore, we should not be doing a shallow clone of the metadata object as this breaks the expected way in which javascript objects should be handled

2. Allow any object (in the types)

The only reason we currently only allow a non-nested object is so that flattenTree can do a shallow clone. However, users who build their INode trees manually, either through async data calls or through manual data mapping, do not use flattenTree and therefore should not be limited in how metadata is handled in their nodes.

If we are no longer cloning the metadata object (see my first point), then there is nothing stopping us from supporting any object as metadata. This would allow us to pass anything as metadata including nested objects, functions or even an instances of a class.

For example, you could pass a helper function to determine which color to use, rather than having to pass a hard-coded color to the metadata object:

const metadata = {
  defaultColor: 'red',
  getNodeColor: (rendererProps: INodeRendererProps): string => {
    return rendererProps.isSelected
      ? someComplexWayToCalculateColor()
      : renderProps.element.metadata.defaultColor;
  },
}

If there are changes to the interfaces, please describe them here:

1. Rename IFlatMetadata to IMetadata
2. Don't use the Record<> helper type. The metadata object is completely user controlled and operated on, we don't do anything with it except detecting if it exists. Therefore, the type of metadata should be unknown.

export type IMetadata = unknown;

We should also allow users to set the type of the metadata when defining the props of their tree. Therefore, we should add generics for metadata to any interface or function that contains a reference to the INode type so that user defined metadata correctly propagates to all internal functions. This includes:

ITreeViewProps
ITreeViewOnLoadDataProps
ITreeViewOnExpandProps
ITreeViewOnNodeSelectProps
ITreeViewOnSelectProps
INodeRendererProps
INodeProps
INodeGroupProps

usePreviousData
isBranchNode
getParent
getAncestors
getDescendants
getChildren
getSibling
getLastAccessible
getPreviousAccessible
getNextAccessible
propagateSelectChange
getAccessibleRange
propagatedIds
isBranchSelectedAndHasSelectedDescendants
isBranchNotSelectedAndHasAllSelectedDescendants
isBranchNotSelectedAndHasOnlySelectedChild
isBranchSelectedAndHasOnlySelectedChild
getTreeParent
getTreeNode
validateTreeViewData

Node
NodeGroup

Describe alternatives you've considered

I'm not sure this applicable here.

Additional context

I would be happy to submit a PR to handle this.

[dgreene1]

@Pearce-Ropion we took over this library since it was abandoned. We’re trying to add new life to it, but we don’t know it’s history. So I can’t answer why it used clone in the first place. As to why it’s not deep cloned, I don’t think it’s appropriate to have widget bring in a deep clone library due to the size of such libs, so when I PR reviewed the metadata enhancement the contributor and I decided shallow clone was enough.

Anyway, if you can PR this and allow a nested object on metadata and not break the tests, then go for it.

@mellis481 or @kpustakhod do you know why the shallow clone was there in the first place?

[Pearce-Ropion]

Anyway, if you can PR this and allow a nested object on metadata and not break the tests, then go for it.

Thanks. I went ahead and created #130.

As to why it’s not deep cloned, I don’t think it’s appropriate to have widget bring in a deep clone library due to the size of such libs, so when I PR reviewed the metadata enhancement the contributor and I decided shallow clone was enough.

Hmm, not sure if something was lost in translation here. I am not suggesting that we deep clone metadata. I am suggested that we don't clone it at all.

[dgreene1]

Yes, I agree that no cloning would be ideal if it is possible to do so. But I am assuming that cloning was necessary so to the original library creator adding it into the logic.

Anyway, I have type changes that I’ve requested on your PR.

[donaldpipowitch]

Would be awesome to see a solution. Right now IFlatMetadata is also disallowing booleans to be used.

[dgreene1]

@donaldpipowitch we’ll accept a PR for the Boolean enhancement. However, I’m less sure of the nested object enhancement since the application code can store the list of nested objects and then you can just have a reference in the state of the tree view. I know I’m not explaining the workaround well, but I don’t see why your widget would have to keep all of your data. But a Boolean, sure. So send us a PR if you’d like for that.

[Pearce-Ropion]

@donaldpipowitch we’ll accept a PR for the Boolean enhancement. However, I’m less sure of the nested object enhancement since the application code can store the list of nested objects and then you can just have a reference in the state of the tree view. I know I’m not explaining the workaround well, but I don’t see why your widget would have to keep all of your data. But a Boolean, sure. So send us a PR if you’d like for that.

In the case you've described, there is no point to even having the metadata property in the first place since you could apply the same logic for anything you can currently store in metadata. The idea with the metadata object is that you don't have to do a nested search through the data in the application because you are already iterating through it in order to render the tree. Therefore, the metadata itself should be treated as a quality of life improvement.

[dgreene1]

@Pearce-Ropion i understand your perspective, but at this point if I’m my comments can’t be resolved as requested, I honestly don’t have more time to spend on this.

I will absolutely accept a pr that allows booleans. But this nested metadata stuff is more than I think I want to support “in my free time.” I’m considering locking this specific issue as won’t do. It’s a matter of our small team not being able to support all cases for all apps. My workaround above is reasonable and any consumer should be able to utilize linked lists or a hash map to do this in such a way that performance is not a problem.

This repository is meant to focus on the accessibility needs and that’s where our focus will stay. If you want a tree that can handle tons of scenarios, go look at behemoths like KendoUI.

I won’t be replying for a few weeks as I must focus on an important work deadline. In the meantime, if you can make the change without a breaking change, without type casting, without the any keyword, and the tests pass, we will consider it. But I just want to be fair to you to communicate that this is not a high priority feature and therefore our focus is elsewhere. I hope that helps you with prioritizing your own work.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.