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

remove all children #276

Merged
merged 3 commits into from
Jun 7, 2021
Merged

remove all children #276

merged 3 commits into from
Jun 7, 2021

Conversation

Fil
Copy link
Member

@Fil Fil commented Feb 2, 2021

fixes #275

Copy link
Contributor

@curran curran left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

Tracing this all the way back to the original problem that selectChildren solves, described in #63, it does seem to make sense to make it compatible with .remove() in this way.

@mbostock
Copy link
Member

mbostock commented Jun 6, 2021

This only fixes selection.selectChildren, but does not fix other cases where you pass a “live” HTMLCollection to d3.selectAll or return an HTMLCollection from the function in selection.selectAll. Both of those cases allow an HTMLCollection to be considered an array here:

export default function(x) {
return typeof x === "object" && "length" in x
? x // Array, TypedArray, NodeList, array-like
: Array.from(x); // Map, Set, iterable, string, or anything else
}

I think there are two ways we could go here.

One would be for the array function to test for HTMLCollection more explicitly and force the conversion to a non-live array.

The other (and it’s not exclusive) would be for selection.remove to run in reverse order, so that the last element is removed first, and thus selection.remove will remove all the elements even if the collection is live.

My inclination is that we should do the first, and that if we do the first, we don’t really need to do the second. D3 selections are intended to be immutable (at least with respect to the elements they contain—obviously the elements themselves are mutable). That said, I’m not sure how best to detect HTMLCollection yet. We could do an instanceof check, but that might be overly precise.

@mbostock
Copy link
Member

mbostock commented Jun 6, 2021

Oops, apparently NodeList can be live too, e.g. d3.selectAll(document.body.childNodes). So, copying an HTMLCollection into an Array solves only some of the problem, and there’s no (practical) way to distinguish between a NodeList that is live (e.g., element.childNodes) and a NodeList this is static (e.g., document.querySelectorAll). That said, we could still have a special path that avoids a copy for the common case of selection.selectAll(selector).

@mbostock mbostock merged commit 690dd20 into main Jun 7, 2021
@mbostock mbostock deleted the children-remove branch June 7, 2021 17:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 participants