Skip to content

remove all children - #276

Merged
mbostock merged 3 commits into
mainfrom
children-remove
Jun 7, 2021
Merged

remove all children#276
mbostock merged 3 commits into
mainfrom
children-remove

Conversation

@Fil

@Fil Fil commented Feb 2, 2021

Copy link
Copy Markdown
Member

fixes #275

@curran curran left a comment

Copy link
Copy Markdown
Contributor

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

mbostock commented Jun 6, 2021

Copy link
Copy Markdown
Member

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

mbostock commented Jun 6, 2021

Copy link
Copy Markdown
Member

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

Development

Successfully merging this pull request may close these issues.

selection.selectChildren().remove() does not remove all children

3 participants