Skip to content

Conversation

keijokapp
Copy link

The at method overloads do not handle all cases correctly. Both key and value transformers can independently have type undefined, Transformer or Transformer | undefined. In the later case, the return value would need to be a union type. That's total of 9 combinations (in addition to GetSubspace case).

Example:

const subspace: Subspace<'KeyType1', 'KeyType1'> = new Subspace('');

const subspace1: Subspace<'KeyType2', 'KeyType2'> = subspace.at(null, undefined)
const subspace2: Subspace<'KeyType2', 'KeyType2'> = subspace.at(null, undefined as Transformer<'KeyType2', 'KeyType2'> | undefined)

That code would pass the type checking with the current overloads but is actually invalid.

@keijokapp keijokapp force-pushed the fix-subspacing-types branch from d6516f2 to 4ec83f5 Compare May 9, 2024 11:29
@josephg
Copy link
Owner

josephg commented May 9, 2024

That code would pass the type checking with the current overloads but is actually invalid.

Why would you want to pass null and undefined to subspace.at like that? Is there a case where being less strict here actually causes real problems? I mean, your proposed fix adds a lot of typing noise. Is there a benefit to fixing this beyond making it more mathematically pure?

@keijokapp
Copy link
Author

Is there a case where being less strict here actually causes real problems?

I don't think it's about strictness. The innocent-looking code ends up with a wrong type rather than an imprecise type. Alternatively, the methods could return Subspace<unknown, unknown, unknown, unknown> or Subspace<any, any, any, any> which would be less strict but probably better than a wrong type.

Why would you want to pass null and undefined to subspace.at like that?

This is a minimal example. In reality, we often don't have the luxury to know what values (or even types in the case of generics) we get. Passing in a constant undefined is probably not a real use case (except maybe if there's some magic with generics going on in the caller). Having undefined | Transformer is a common case.

I mean, your proposed fix adds a lot of typing noise.

I think having 9 overloads instead of 4 is a small price to pay. I think these 9 overloads are also easier to comprehend since they handle all cases systematically and correctly. The pattern is fairly clear. (Though I'm obviously biased because I wrote that.) Sadly, I haven't found a less bloated way.

@keijokapp keijokapp force-pushed the fix-subspacing-types branch 2 times, most recently from 05b3a77 to 1d9a130 Compare May 10, 2024 11:30
@keijokapp keijokapp force-pushed the fix-subspacing-types branch from 1d9a130 to 5e0949c Compare May 28, 2024 20:55
@keijokapp keijokapp force-pushed the fix-subspacing-types branch from e13d6c5 to cd6e044 Compare July 8, 2024 07:45
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.

2 participants