Alias types

[aslak01]

Hey, I just noticed the typescript cleanup PRs being merged and I wanted to make a suggestion about using type aliases to make complex types more easy to reason about.

Also the type Function seems to provide very little benefit, I also added a type that replaces it in the Scale case for consideration.

[mhkeller]

This is much needed, thanks for putting it together!

We merged the other PR to start getting everything on the same branch but the types are still definitely a work in progress so any help on this or other PRs is greatly appreciated. It's a big lift to port this all over to Svelte 5. I left a couple of questions above.

[aslak01]

Yeah no problem, I actually started this last year, but then I got busy moving / renovating house and forgot about it until now. I think you'll find that it's a lot easier to work with types when they are shared instead of inlined.

The Svelte 5 move seems like a massive undertaking, it's basically a fundamental rewrite I guess?

[mhkeller]

No worries I understand! I have another project that has been taking up a lot of my attention I get it. The collective effort helps! I've also learned a lot more TS than I did a year ago.

In terms of a re-write, there's obviously the components, which are outside of the library and then the library itself. There's also the layout components but those are pretty easy. We're staring with the components and things that are public facing. I think it will be a question down the road if we stick with stores or move to $state. Moving to $state would require an API change while sticking with stores may be confusing for people coming to Svelte since the time of runes and may find the $ syntax weird.

In short, though, the components can be updated to Svelte 5 / runes for now and the question on stores vs $state can be punted for now.

[rgieseke]

That looks very useful! I wonder whether it would be possible to import something from @types/d3-scale and re-use that?

[aslak01]

Yeah it could be done like this:

import type { 
  ScaleContinuousNumeric,
  ScaleOrdinal, 
  ScaleTime,
  ScaleLinear,
  ScalePower,
  ScaleLogarithmic,
  ScaleSymlog,
  ScaleQuantile,
  ScaleQuantize,
  ScaleThreshold,
  ScaleSequential,
  ScaleDiverging
} from 'd3-scale';

type AnyD3Scale = 
  | ScaleContinuousNumeric<any, any>
  | ScaleOrdinal<any, any>
  | ScaleTime<any, any>
  | ScaleLinear<any, any>
  | ScalePower<any, any>
  | ScaleLogarithmic<any, any>
  | ScaleSymlog<any, any>
  | ScaleQuantile<any, any>
  | ScaleQuantize<any, any>
  | ScaleThreshold<any, any>
  | ScaleSequential<any, any>
  | ScaleDiverging<any, any>;

Potentially adding D3ScaleLike to support people implementing their own scale functions. However I think that requires a .ts file that would be compiled by tsc due to the import statement, not just a .d.ts file.

[mhkeller]

My two cents is I'm not sure anyone is implementing their own scale functions and setting the type to the D3 definitions feels like the better route to me.

[mhkeller]

@aslak01 or anyone else, if you want to keep plugging away on this that would be great. We're getting pretty close to the next release where I'm going to roll in https://github.com/mhkeller/layercake/pull/271, https://github.com/mhkeller/layercake/pull/241 and https://github.com/mhkeller/layercake/pull/243. I had to make some breaking changes to the layout component props so it'll be a major version release.

[aslak01]

I've changed the Scale type to use the @types/d3 types, but using my linter there is a further problem stemming from the mix of null and undefined in the codebase, I get a lot of these errors

Because the values are initialised as undefined but the type only allows for null in the empty case. Personally I think it's a good idea try to only use undefined as JS won't let you escape that, but nulls could potentially only arrive from other libraries.

I like this piece on null and undefined in js: https://medium.com/javascript-scene/handling-null-and-undefined-in-javascript-1500c65d51ae

[rgieseke]

I think it would be super nice to have something more meaningful than just Function.

I checked out the PR to see what one gets in the editor tooltip. What is the 'OrConstructor' part of D3ScaleOrConstructor for? The LayerCake comment states

The D3 scale that should be used for the x-dimension. Pass in an instantiated D3 scale if you want to override the default or you want to extra options.

So from that perspective maybe just having D3Scale would be sufficient?

(As an aside, the comments for yScale, zScale. rScale refer to the "x-dimension", maybe they could be fixed in this PR as well.)

[aslak01]

I checked out the PR to see what one gets in the editor tooltip. What is the 'OrConstructor' part of D3ScaleOrConstructor for? The LayerCake comment states

The D3 scale that should be used for the x-dimension. Pass in an instantiated D3 scale if you want to override the default or you want to extra options.

So from that perspective maybe just having D3Scale would be sufficient?

I'm not sure why it's set up like that, but the src/lib/settings/defaultScales.js is passing the scales uninstantiated

https://github.com/mhkeller/layercake/blob/d38dcd8595ea683458f09a5ee052da8c4604e330/src/lib/settings/defaultScales.js#L3-L8

so the type needs to support both. It seems to me like getting rid of the OrConstructor would be beneficial, but I'm not familiar enough with the project to have an opinion on whether or not that makes sense.

(As an aside, the comments for yScale, zScale. rScale refer to the "x-dimension", maybe they could be fixed in this PR as well.)

Sure, fixed.

[rgieseke]

Got it, thanks for the explanation!