search

josephg / node-foundationdb

Modern Node.js FoundationDB bindings
Other
115 stars 17 forks source link

Fix typings of `at` methods #79

Open keijokapp opened 2 months ago

keijokapp commented 2 months ago

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.

josephg commented 2 months ago

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 commented 2 months ago

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.