Skip to content

Adopt InternMap. - #185

Merged
mbostock merged 6 commits into
masterfrom
internmap
Jan 22, 2021
Merged

mbostock merged 6 commits into
masterfrom
internmap

Conversation

@mbostock

Copy link
Copy Markdown
Member

@mbostock
mbostock requested a review from Fil January 21, 2021 20:47
Comment thread src/index.js
@Fil

Fil commented Jan 22, 2021

Copy link
Copy Markdown
Member

Documentation at https://observablehq.com/@d3/d3-group will need an update when this lands. (I've also added a section on d3.index, if you want to proofread it.)

Note that the paragraphs about key equality are defensive, and we'll need to mention the version number which solves this.

It's worth noting that while InternMap can index on an array, by passing a key function, there is no way to pass a key function to d3.group (and rollup and index).

When the key is on the first level it's easy though to upgrade to an InternMap:

@Fil

Fil commented Jan 22, 2021

Copy link
Copy Markdown
Member

I've added documentation in #186.

@mbostock

Copy link
Copy Markdown
Member Author

I thought about exposing the configurable key function through d3.group et al. but that feels like overkill to start. (I mainly think configurable keys are useful for compound keys; compound keys aren’t typically needed with d3.group because it’s already nested, and furthermore JSON.stringify is slow.) If we do need to expose it in the future, it could be something like d3.group.key(JSON.stringify)(data, d => d.foo), or d3.groupKey etc.

Thanks for writing docs! I’ll check ’em out now.

@mbostock
mbostock merged commit 5a453a6 into master Jan 22, 2021
@mbostock
mbostock deleted the internmap branch January 22, 2021 18:16
@Fil Fil mentioned this pull request Jan 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

Adopt InternMap for d3.group et al.?

3 participants