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

d3.tree won't accept a hierarchy where children are an empty list #144

Closed
Malex opened this issue May 10, 2019 · 6 comments
Closed

d3.tree won't accept a hierarchy where children are an empty list #144

Malex opened this issue May 10, 2019 · 6 comments

Comments

@Malex
Copy link

Malex commented May 10, 2019

It turns out this line https://github.com/d3/d3-hierarchy/blob/master/src/tree.js#L150 breaks if the passed hierarchy has an empty list of children (due to the fact that children[0] will be undefined
TypeError: Cannot read property 'z' of undefined
at firstWalk (tree.js:150)
at TreeNode.eachAfter (eachAfter.js:10)
at GraphService.tree (tree.js:110)
where GraphService is my code handling my hierarchy graph).

Solving the bug in user code is of course really simple (just need to check length and put undefined or null instead), but I do think d3.tree should check it as well and treat empty arrays just like it treat the null/undefined case.

PS Just to clarify, of course it was not d3.hierarchy to put the list to empty, it was user code accessing a node.children property to filter it.
Sadly there's no API to filter a hierarchy after it has been computed (other than using .each and setting children manually), but computing it from scratch was not feasible in that scenario.
I think a filter API would be welcome; handling empty list cases would suffice otherwise (please note that in the documentation nothing says children lists cant be empty, and the tree function could be called even with non-d3 (but compatible) data structures, so assuming the non-emptiness of a list doenst feel right)

@Fil
Copy link
Member

Fil commented May 11, 2019

I think this is a case that would be covered by #140 (comment)

@Malex
Copy link
Author

Malex commented May 24, 2019

Not exactly. In my case the children are arrays, just empty arrays. d3.hierarcly (probably for optimination sake) makes leaves have children undefined, while it will throw error if emtpy.

@mbostock
Copy link
Member

The contract of d3.hierarchy currently is that node.children is undefined for leaf nodes. Thus, if node.children is defined, it must be a non-empty array. We could redesign that in a future major version, but that’s the current contract.

@Malex
Copy link
Author

Malex commented May 25, 2019 via email

@Fil
Copy link
Member

Fil commented May 25, 2019

See https://github.com/d3/d3-hierarchy#hierarchy

  • The specified children accessor (…) must return an array of data representing the children, or null if the current datum has no children.
  • The returned node and each descendant has the following properties: (…) node.children — an array of child nodes, if any; undefined for leaf nodes

maybe we should explicitly add "non-empty": must return a non-empty array of data representing the children?

@mbostock
Copy link
Member

Leaf nodes by definition have no children, so it’s already documented that node.children is undefined in this case. I don’t think the API reference needs any further editing but it will help when we publish extended tutorials and examples for d3-hierarchy.

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

No branches or pull requests

3 participants