Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(tree-view): add nodesFlat prop #2069

Closed

Conversation

metonym
Copy link
Collaborator

@metonym metonym commented Nov 26, 2024

No description provided.

@brunnerh
Copy link
Contributor

I don't really see a reason to bloat TreeView with this, this could probably exist as a separate function or in user-land entirely.

<TreeView nodes={toHierarchy(nodeList)}>

@metonym
Copy link
Collaborator Author

metonym commented Nov 27, 2024

@brunnerh Agreed – I like the notion of a separate utility (tree-shakeable) that can be optionally used.

@bhavers
Copy link
Contributor

bhavers commented Nov 28, 2024

Shall i make this a separate function?
One downside from a DX perspective that i could see (not sure) is that you would loose easy reactivity with nodes=treeview.toHierarchy(flatdata) instead of nodesFlat=flatdata (i guess you would need an #key block for the former).

@metonym
Copy link
Collaborator Author

metonym commented Nov 30, 2024

@bhavers Yes. Let's make this a separate function. If I'm understanding correctly, I don't think reactivity would be a concern – if nodes changes, all downstream dependencies should be auto-updated.

  • Create a treeview.js file in the src/TreeView directory with the exported function to flatten the nodes. Moving the logic outside of the component avoid penalizing usage in which the function is not used (it should be tree-shaken).
  • Also create a treeview.d.ts file, which will be copied over to types as part of build process. A nice reference is breakpointObserver.d.ts

@bhavers
Copy link
Contributor

bhavers commented Nov 30, 2024

Ok, will do so some moment in the upcoming days. Thanks for the extra explanation.

@bhavers
Copy link
Contributor

bhavers commented Dec 1, 2024

@metonym I don't get it to work.
It keeps complaining that the function toHierarchy is not exported, but it is.

Uncaught (in promise) SyntaxError: The requested module '/@fs/Users/bramhavers/Documents/code/carbon-components-svelte/src/index.js?t=1733084657166' does not provide an export named 'toHierarchy' (at TreeViewFlatArray.svelte:2:22)

And i am also not clear why we would need those definition files. It seems quite double because the javascript files already contain JSDoc type declarations.
In breakpointobserver i also saw a index.d.ts which is identical to index.js, which makes little sense to me.
Hope you can provide a hint on what i am doing wrong here.

@bhavers
Copy link
Contributor

bhavers commented Dec 2, 2024

@metonym Never mind, a day later and a fresh quick look reveals that i was looking at the wrong index.js. Will make a new commit.

@bhavers
Copy link
Contributor

bhavers commented Dec 2, 2024

@metonym It works. Couple of questions:

  • toHierarchy (in treeview.js) didn't show up in the docs, so i created a proxy toHierarchy in TreeView.svelte. Is that ok, or does it bloat the component & prevent tree shaking? How would we otherwise get toHierarchy in the docs?
  • the test wouldn't run with toHierarchy in treeview.js; only when using toHierarchy from TreeView. I guess that has to do with the test runner not being linked to the new code? (i ran npm run test from the carbon root.
  • i added toHierarchy to src/index.js, but also to TreeView/index.js, and also added some definitions files. Seems all a bit double. Is that as designed or should i have only updated it in one place?

@metonym
Copy link
Collaborator Author

metonym commented Dec 8, 2024

@bhavers Great work! I've reviewed this and implemented the following changes. I opened #2072 since I was not able to push directly to the outside PR.

  • Avoid modifying the existing TreeView component since toHierarchy is designed to be used outside of it (formatting nodes prop as an input). This avoids any penalty to consumers that do not use this function, as toHierarchy can be tree-shaken.
  • Refactor toHierarchy to have a required second argument – a callback function to retrieve the parent ID (if applicable) on a given node. This avoids the need to hardcode the key of the parent ID. Any key (except id and nodes) can be used. As a user, I wouldn't assume "pid" to mean parent ID. This also saves a need to normalize the array before passing it to this function.
  • Refactor toHierarchy to avoid an extra loop to prune "empty" nodes. Instead, the parent existence is validated before child nodes are added.
  • Refactor toHierarchy TypeScript definitions to leverage generics. This allows for additional type safety, as it requires the parent ID to be a valid property and the supplied array of objects.
  • Finally, since the nodes prop of both TreeView and RecursiveList are the same, I made this utility even more generic for use with either component.
type NodeLike = {
  id: string | number;
  nodes?: NodeLike[];
  [key: string]: any;
};

export function toHierarchy<
  T extends NodeLike,
  K extends keyof Omit<T, "id" | "nodes">,
>(
  flatArray: T[] | readonly T[],
  /**
   * Function that returns the parent ID for a given node.
   * @example
   * toHierarchy(flatArray, (node) => node.parentId);
   */
  getParentId: (node: T) => T[K] | null,
): (T & { nodes?: (T & { nodes?: T[] })[] })[];
Screenshot 2024-12-08 at 1 54 18 PM

@bhavers
Copy link
Contributor

bhavers commented Dec 8, 2024

@metonym Thank you for the explanation. I went through your refactorings and it was really helpful to see how this is done. Although i don't think i fully the grasp the generics wizardry ;-)
Looking forward to using this new function.

@metonym metonym closed this in #2072 Dec 9, 2024
@metonym
Copy link
Collaborator Author

metonym commented Dec 10, 2024

Released in v0.87.0.

If using optimizeImports from carbon-preprocess-svelte, you'll need to upgrade to carbon-preprocess-svelte@0.11.9.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants