selfrefactor/rambda

Possible currying glitch

eugenezastrogin opened this issue · 10 comments

For some reason rambda typings fail in this tuple array example with curried prop function. If we use full form, TS infers proper types:
https://stackblitz.com/edit/typescript-frjsyt?file=index.ts

I couldn't quite understand if it is TS limitation or simply a missing overload :(

I did the same example with Ramda and I received one more error, which means that there is no error in Rambda typings.

In your example, you receive error when you use prop(0) but R.prop doesn't accept number as property input. It comes from Ramda typings and Rambda often follows their definitions.

So while R.prop(0) works fine in plain Javascript, but from Typescript point of view this is incorrect. I can change the typing of R.prop but am unsure that will wise.

I am closing this, as I haven't receive any further response.

Sorry for the late reply, didn't have time to investigate further.

I am by no means an advanced TS user, so I don't have a ready solution, but would like to share my findings.

It seems that original ramda's prop was made to accept numbers as property input to better follow JS behavior (which seems reasonable)

And indeed, a special handling of numbers for prop was added.

I see that rambda's implementation have array indexing ootb support as well.

Now, this code works in both libraries:

const t = prop(0, [100]); //=> 100

Here it compiles with the following overload matching:

export function prop<P extends keyof T, T>(propToFind: P, obj: T): T[P];

And indeed, keyof number[] = number | "length" | ....

So it seems that no harm is done by using array indexing via prop - it is supported by both JS and TS implementation, mentioned in the docs and is in there by design.

Now, getting back to curried version.

While aforementioned uncurried type overload allows for arbitrary P extending keyof T, curried versions don't, they expect strings only:

export function prop<P extends string>(p: P): <T>(obj: Record<P, T>) => T;
export function prop<P extends string, T>(p: P): (obj: Record<P, T>) => T;

Something along the lines of this would be great to have:

export function prop<P extends keyof T, T>(p: P): (propToFind: P) => T[P]; 

But this overload only works with explicit types provided (this seems to be an inference limitation):

const myAccessor = prop<keyof MyObjType, MyObjType>(0);

Seems repeating, and ugly?

So, just adding number might not be the worst solution for the time being, and it certainly tracks usage and implementation better:

prop<P extends string | number>(p: P): <T>(propToFind: Record<P, T>) => T;
prop<P extends string | number, T>(p: P): (propToFind: Record<P, T>) => T;

Ramda typings leave much to be desired, both old and new, it might be worth it to deviate from them with good reasons.

Thank you for the long comment. I am reopening this issue and will get back to you with suggestion. You make a good point that maybe I don't need to follow @types/ramda so closely, but without personally experiencing TS problems, it is hard to spot them just by when looking at them.

I applied your suggestion. You can look at it to confirm that it is as expected.

I applied your suggestion. You can look at it to confirm that it is as expected.

Hmm, I cannot see any prop type changes in linked commit index.d.ts, just batch export keyword removals.

Sorry, I forgot to run the build script and I also forgot that I have made other changes not related to this issue. Please check the other linked commit.

Overall LGTM 👍🏻, but while checking how it works I began wondering if we can pull off this (probably can):

<P extends keyof T, T>(propToFind: P, obj: T): T[P];
<P extends string | number>(p: P): <T>(propToFind: Record<P, T>) => T;
<P extends keyof T, T>(p: P): (propToFind: Record<P, T>) => T;

and I think I found another bug in curried typings (maybe for another issue though):
Record<P, T> narrows type too much, this causes an assignability error in legit cases:

const test = prop('e')({ e: 'test1', d: 'test2' })

Argument of type '{ e: string; d: string; }' is not assignable to parameter of type 'Record<"e", string>'.

And indeed, this is akin to:

type M = Record<'e', string>
const test: M = { e: 'test1', d: 'test2' }

It shouldn't be a strict Record in the typings, it should extend Record.
I'll try to research this further.

I will try to apply your suggestion and I will get back to you.

Closing this as changes are applied with version 6.4.0.