Definitelytyped: [d3-scale] Scales return `T | undefined` ?

[m-rutter]

Is something wrong with CI or publishing infrastructure?

• Check the Infrastructure status updates thread for known issues.
• Leave a message on the DefinitelyTyped Gitter.

If you know how to fix the issue, make a pull request instead.

• [x] I tried using the @types/xxxx package and had problems.
• [x] I tried using the latest stable version of tsc. https://www.npmjs.com/package/typescript
• [x] I have a question that is inappropriate for StackOverflow. (Please ask any appropriate questions there).
• [x] [Mention](https://github.com/blog/821-mention-somebody-they-re-notified) the authors (see Definitions by: in index.d.ts) so they can respond.
  
  
  
  • Authors: @Methuselah96
  
  

I just pulled the latest definitions for d3-scale that includes this merged PR:
https://github.com/DefinitelyTyped/DefinitelyTyped/pull/48095

I'm curious why all of the scale functions now have this signature:

    // ordinal:
    (x: Domain): Range | undefined;
   // continuous
   (value: NumberValue): Output | undefined;

Is this because its possible that the Range array could be empty? Reading the d3-scale docs I don't see undefined being described as a possible output value, and I've never had undefined returned in practice as far as I'm aware.

[Methuselah96]

The reasoning was that unknown was introduced in 2.2 which returns undefined for certain unknown values like NaN. See https://github.com/d3/d3-scale/issues/97 for more details about the functionality.

[Methuselah96]

@m-rutter Would you like me to revert the change? While it is defined behavior, it does seem a bit of an edge case and could end up being more annoying than helpful.

[m-rutter]

@Methuselah96

I didn't know about unknown, thanks for the link.

I'm not sure. I suppose it would be nice to have a UknownProvidedContiniousScaleType<T> and friends that returns T and not T | undefined, but that is probably difficult to implement?

I've had to add assertions in a number of places to deal with the new change because I've not had to time to figure out fallback values.

In the case of ScaleOrdinal is that still strictly required? The documentation says that if an unknown value is provided that it adds it to the domain gives and gives it mapping to the range array. I suppose there is always the case of the range being empty, which case there is no value to provide and the unknown fallback gets invoked?

[Methuselah96]

@m-rutter You're right, I overlooked ScaleOrdinal, thanks for pointing that out. I made a PR to fix that.

[danvk]

@Methuselah96 After updating @types/d3-scale in our project, we're running into all sorts of errors because of this. I do think it would be more sensible to exclude this possibility from the types unless you opt-in, maybe something like

const scale = scaleLinear().range([0, 100]);
const x = scale(0.5);  // type is number

vs.

const scale = scaleLinear().range([0, 100]) as SafeScaleLinear<number, number>;
const x = scale(0.5);  // type is number | undefined

or add it as a third generic parameter:

const scale = scaleLinear().range([0, 100]);
const x = scaleSafe(0.5);  // type is number

const scaleSafe = scaleLinear<number, number, true>().range([0, 100]);
const xSafe = scaleSafe(0.5);  // type is number | undefined

There's always a balance between "correct" and "convenient to use" in type declarations, and I think this goes a little too far to the "correct" side. See https://github.com/microsoft/TypeScript/pull/39560 for a similar debate about making indexing into an array return T | undefined instead of T.

[Methuselah96]

@danvk No objections from me to revert that part back. Do you think that scaleBand and scalePoint should still return | undefined? Those scales returned | undefined before my changes in https://github.com/DefinitelyTyped/DefinitelyTyped/pull/48095. The documentation for scaleBand and scalePoint more explicitly note that if the value is not in the scale's domain, that it returns undefined.

[danvk]

@Methuselah96 Thanks. Let's leave those methods as they were before.

Another option for modeling scale.unknown would be to have it change the return type only when you explicitly call it. For example:

const scale = scaleLinear().range([0, 100]);
const x = scaleSafe(0.5);  // type is number

const scaleWithUnknown = scaleLinear().range([0, 100]).unknown('error');
const xUnknown = scaleWithUnknown(0.5);  // type is number | string

and then you could call .unknown(undefined) if you really wanted to model this in the type system:

const scaleSafe = scaleLinear().range([0, 100]).unknown(undefined);
const xSafe = scaleSafe(0.5);  // type is number | undefined

[Methuselah96]

@danvk https://github.com/DefinitelyTyped/DefinitelyTyped/pull/49196 should resolve it.