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

Question

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

Malex opened this issue 5 years ago · 6 comments

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)

Answer 1 · 2019-05-11T20:34:58.000Z

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

Answer 2 · 2019-05-24T13:41:11.000Z

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.

Answer 3 · 2019-05-24T14:35:47.000Z

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.

Answer 4 · 2019-05-25T08:46:46.000Z

Such contract should be documented though. Documentation exists for a reason, that reason includes explicit declaring the contracts Il giorno ven 24 mag 2019, 16:35 Mike Bostock <notifications@github.com> ha scritto:

…

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. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#144?email_source=notifications&email_token=AAAZEDDENX52BLYJJN5Y4X3PW74MLA5CNFSM4HMDYJBKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODWFSAUA#issuecomment-495657040>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAZEDBBQFQSM3SNI44MLNDPW74MLANCNFSM4HMDYJBA> .

Answer 5 · 2019-05-25T10:05:38.000Z

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?

Answer 6 · 2019-05-25T14:13:51.000Z

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.