-
Notifications
You must be signed in to change notification settings - Fork 315
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
Comments
I think this is a case that would be covered by #140 (comment) |
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. |
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. |
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>
.
|
See https://github.com/d3/d3-hierarchy#hierarchy
maybe we should explicitly add "non-empty": must return a non-empty array of data representing the children? |
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. |
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)
The text was updated successfully, but these errors were encountered: