Remove explicit null value from JSDOC type definition for domain values

[rgieseke]

When using Layercake in a SvelteKit project with JSDoc or TypeScript type checking I am getting an error message for the [0, null] domain values, for example yDomain here:

 <LayerCake
    padding={{ top: 7, right: 10, bottom: 20, left: 25 }}
    x={xKey}
    y={yKey}
    z={zKey}
    yDomain={[0, null]}
    zScale={scaleOrdinal()}
    zRange={seriesColors}
    flatData={flatten(groupedData, 'values')}
    data={groupedData}
  >

This gives the error message (from npm run check):

Error: Type '[number, null]' is not assignable to type 'Function | number[] | string[] | [min: number, max: number] | undefined'.
  Type '[number, null]' is not assignable to type 'Function | number[] | string[] | [min: number, max: number]'.
    Type '[number, null]' is not assignable to type '[min: number, max: number]'.
      Type at position 1 in source is not compatible with type at position 1 in target.
        Type 'null' is not assignable to type 'number'. (js)

When I change the JSDOC definitions to remove `|null' it still seems to work fine, so maybe there are not neccessary?

JSDoc.app lists nullable types like this as well

A number or null. {?number}
This indicates that the type is either the specified type, or null. 

https://jsdoc.app/tags-type.html

[rgieseke]

I spoke too soon, this does not fix the problem 🙈

[rgieseke]

Also didn't see the previous discussion in #117

[mhkeller]

Hm – it would seem fairly basic to me that something could be null. Is it saying that it needs to have both types be the same? Do you get the same error if you set [null]?

[rgieseke]

Yes, seems to be the same error:

Error: Type '[null]' is not assignable to type 'Function | number[] | string[] | [min: number, max: number] | undefined'.
  Type '[null]' is not assignable to type 'Function | number[] | string[]'.
    Type '[null]' is not assignable to type 'string[]'.
      Type 'null' is not assignable to type 'string'. (js)

I fully agree, that something could be null seems fairly basic (and I like this as an easily understandable API).

[rgieseke]

FWIW, having the error is due to "strictNullChecks": true, (or strict) - without I don't get the error.

https://www.typescriptlang.org/tsconfig#strictNullChecks

[mhkeller]

Great find! So if we remove that and keep the JSDoc types as they are currently, that solves it? Or is it better to change the syntax as you describe above?

[rgieseke]

I don't know. The problem occurs because the default of a new SvelteKit project with JSDoc/Typescript includes strict: true.

Thus it might be better to change the syntax. I'll need to test that, i am slowly wrapping my head around where things go and when to build and package (hence my initial confusion).

[rgieseke]

I think the leading ? might be in the wrong position (at least for Vscode which uses Typescript's JSDoc extension as far as I understood):

There is no difference when they are left out:

Maybe they should go here:

But maybe we rather want this, only the second parameter being nullable?

The domains in the examples all seem to be [0, null] (or another array with specific years etc.).

When turning the example above in a little Test module I can import the component without problems and without any 'null' not assignable errors, but not yet when changing LayerCake's yDomain definition similarly.

I found these links helpful:
https://stackoverflow.com/questions/52893679/difference-between-union-types-with-null-and-nullable-object-declaration-in-type
https://www.typescriptlang.org/docs/handbook/2/objects.html#tuple-types

[rgieseke]

Got it I think, in LayerCake repo itself strictNullChecks or strict needs also to be true to avoid these error messages.

Nullable types only have meaning if strictNullChecks is on:

/**
 * @type {?number}
 * With strictNullChecks: true  -- number | null
 * With strictNullChecks: false -- number
 */
var nullable;

https://www.typescriptlang.org/docs/handbook/jsdoc-supported-types.html#unsupported-patterns

One can see the difference in dist/LayerCake.svelte.d.ts:
In the export default class LayerCake extends SvelteComponentTyped block with strict: true:

    yDomain?: Function | string[] | number[] | [min: number, max: number | null] | undefined;

Without strict

    yDomain?: Function | string[] | number[] | [min: number, max: number];

In the props block further below in LayerCake.svelte.d.ts they appear with nulls in both settings which had confused me before.

Remains the question, what is the desired configuration?

[min: Number, max: Number|null]?

[null, null] would be allowed from [min: Number|null, max: Number|null] I think but wouldn't really make sense.
Not sure about [null, 0].

[rgieseke]

I switched to the [min: Number|null, max: Number|null] variant and tested this locally.

In partialDomain.js both values could be null and [null, 0] can be used for data going from negative values to 0.

https://github.com/mhkeller/layercake/blob/main/src/lib/utils/partialDomain.js

[mhkeller]

Wow thanks a bunch for diving in and doing all of this great research! I'll merge this and put it as a patch release, which I should be able to do later today.

[rgieseke]

Thank you! Very glad to be able to give back a tiny bit to LayerCake, it's awesome!

[mhkeller]

Are there any changes that should be made to the starter template https://github.com/mhkeller/layercake-template?

[mhkeller]

This is published as 7.6.1!

[rgieseke]

Are there any changes that should be made to the starter template https://github.com/mhkeller/layercake-template?

I thought about that, too. I'll take a look.