Skip to content

Conversation

Fil
Copy link
Contributor

@Fil Fil commented Mar 1, 2023

I'm not sure this is the right way to do it, but I can't find another way… (we certainly don't want to create a normal array, peek at the values, then convert to typed array).

closes #1297

@Fil Fil requested a review from mbostock March 1, 2023 10:09
@mbostock
Copy link
Member

mbostock commented Mar 1, 2023

I think what we do instead is split the current map function into map and typedMap, where map always returns an Array, and typedMap always returns a given TypedArray. Then, in the typedMap case we compose the given accessor with Number to safely coerce rather than relying on the implicit version which errors. (We could also short-circuit the additional number coercion if the input array is already a TypedArray.)

@Fil
Copy link
Contributor Author

Fil commented Mar 1, 2023

0d2e2d3 keeps a single entry point (map). Is it enough?

@Fil Fil marked this pull request as ready for review March 1, 2023 15:40
@mbostock
Copy link
Member

mbostock commented Mar 1, 2023

a single entry point

My thinking was we can easily look for all places that are passing a TypedArray to map and change those to typedMap. That way we don’t need the type === Array test (which also isn’t strictly accurate, since you can technically subclass Array or TypedArray, so we’d have to use isTypedArray instead).

@Fil Fil mentioned this pull request Mar 2, 2023
@Fil
Copy link
Contributor Author

Fil commented Mar 2, 2023

See #1305 for an exploration of typedMap vs. map.

@Fil
Copy link
Contributor Author

Fil commented Mar 2, 2023

I'll add more unit tests tomorrow and merge.

@mbostock
Copy link
Member

mbostock commented Mar 3, 2023

I added some more tests. 👍 Feel free to add more. I feel confident in this change.

I spent some time thinking we could bypass the coerceNumber call for the integer array types (e.g., Uint8Array) since these types have no way of representing NaN, and hence you can’t avoid mapping null to 0. But I was forgetting that the other reason we want coerceNumber is to avoid the error coercing a BigInt to a number, which was the title of this PR, so I threw the work away. 😅

@Fil Fil merged commit 6172d64 into main Mar 3, 2023
@Fil Fil deleted the fil/bigint-map branch March 3, 2023 08:20
@Fil Fil changed the title more support for bigint more support for bigint / refactor Mar 3, 2023
chaichontat pushed a commit to chaichontat/plot that referenced this pull request Jan 14, 2024
Co-authored-by: Mike Bostock <mbostock@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The stack transform should support BigInt
2 participants