d3/d3-array

Simplify return of `extent` when non-comparable values are found

axelboc opened this issue · 4 comments

Currently, d3.extent(iterator) returns [undefined, undefined] when the iterator contains values that cannot be compared.

I suggest changing this return to undefined, since the min and max value are always both defined or undefined together. The full return type would therefore become [number, number] | undefined.

In Typescript, the current return is particularly painful to deal with. One has to check both the min and max value for undefined before TypeScript can infer that the domain is of type [number, number] and not [undefined, undefined].

The problem with returning undefined is that it causes this common pattern to crash:

const [min, max] = d3.extent();

And similarly this will crash rather than giving you a degenerate domain:

scale.domain(d3.extent())

Now, maybe you want a crash in this case, but this would certainly not be backwards-compatible and would need to be deferred until d3@7 (since d3-array@2 is already part of d3@6 which is not released yet).

Fil commented

I can see pros and cons, and it seems easy enough to wrap with a test of e[0] !== undefined. I wouldn't want to change this just for the sake of change.

Closing the issue to clean up, but it's not meant as a "final call"; feel free to comment or reopen if you have strong views about this.

felds commented

@mbostock

It's really easy to fix these problems by being explicit about the fallback values:

// using the old OR expression
const [min, max] = d3.extent() || [0, 0];

// using the new null coalescing operator
scale.domain(d3.extent() ?? [0, 0]);

As I see it, having a truthy return value makes it harder to set fallback values.

In d3 v7, it looks like the return type for .extent() is [undefined, undefined] | [T | T], so you can't do the fallback value approach stated above. I had to do this:

  const [xMin = 0, xMax = 100] = d3.extent(data, (d) => d.total);
  const xScale = d3.scaleLinear().domain([xMin, xMax]).range([width, 0]);