-
Notifications
You must be signed in to change notification settings - Fork 26
Breaking: prop #39
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Breaking: prop #39
Conversation
d6a7ce4 to
e6d81b1
Compare
test/prop.test.ts
Outdated
| // all numbers work as keys, no guarantee it will return a value | ||
| expectType<number>(prop(10)([1, 2, 3, 4])); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the behavior we want here? Should this return number | undefined instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. It should be number | undefined...
Although, I am thinking of removing array support altogether. nth already does this exact behavior and while prop does support arrays, it defers to nth internally
IMHO I feel that support is there for convenience. Typescript's stricter behavior should force users to strictly use prop or nth
path on the other hand, should continue to support both
This, however, would limit the support of the function in typescript only, and is something we should get community import on
New definition would look like this:
// prop(key)(obj)
export function prop<K extends PropertyKey>(prop: K extends Placeholder ? never : K): <U extends Record<K, any>>(obj: U) => U[K];
// prop(__, obj)(key)
export function prop<U>(__: Placeholder, obj: U): <K extends keyof U>(prop: K) => U[K];
// prop(key, obj)
export function prop<K extends keyof U, U>(prop: K, obj: U extends any[] ? never : U): U[K];There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose we could add support for arrays as separate definitions. Instead of K extends keyof U it would be K extends number
types/prop.d.ts
Outdated
| export function prop<_, P extends keyof never>(p: P): <T>(value: T) => Prop<T, P>; | ||
| export function prop<V>(p: keyof never): (value: unknown) => V; | ||
| // prop(key)(obj) | ||
| export function prop<K extends PropertyKey>(prop: K extends Placeholder ? never : K): <U extends any[] | Record<K, any>>(obj: U) => U extends (infer T)[] ? T : U extends Record<K, any> ? U[K] : never; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is PropertyKey a built in type that is equivalent to string | number | symbol?
I can't find any official documentation on this, but found a 3rd party explanation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it is built-in: https://github.com/microsoft/TypeScript/blob/main/src/lib/es5.d.ts#L90
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
documentation FTW!
|
I ended up removing support for arrays. The ramda documentation does has the usage with an array as an example, but the maybe type def |
|
I think the type change here is not correct. |
|
@perrin4869 That is intentional. It's the difference between working in javascript and typescript. In javascript we don't really care that the prop isn't "defined" on the object. We just try and access it and it returns undefined. The behavior of {}.x // undefined
R.prop('x', {}); //undefinedBut try that in typescript and you get a type error {}.x // Cannot find name 'x'By disallowing an The real problem this fixes is misspelling a prop. A problem that has led to thousands of lost hours by frontend devs back when all we had was notepad++ //let's say you had this object
type Person = {
name: string;
birthdate: Date;
};
// if you misspelled birthday, typescript would let you know
({} as Person).bithdate // Property 'bithdate' does not exist on type 'Person'.
// we want the same thing to happen when using `prop`
prop('bithdate', obj as Person);See this playground: https://tsplay.dev/NnLKBW |
|
hm... what if it is an optional property? type A = {
foo?: string
}
const a: A = {};
console.log(a.foo);This is valid typescript. However |
|
That's a bug! MR for fix: #75 |
|
Thanks!! That's awesome 😁 |
This update of the types for
propcorrects what I consider a major flaw in the previous typingThe previous version used a utility type
Propthat is defined inutil/tools.d.tsWhat it essentially did was ignore unions withundefined, such thatThis effectively side-steps the type safety that typescript gives us
Consider the following:
The above type safety is a key part of typescript and it is important to force an opt-out off it for convenience
There are 3 ways to get around this. First would be to have
strictNullChecks: falsein your tsconfig. This will make functions such as.find()not return with theundefinedunion.The other 2 would be done in code:
Using the
!non-null assertion operator is effectively an inline way to opt-in to the previous behaviorWhen using
compose/pipe, users may have to make updates such as this:While I am calling this change
Breaking. We have to keep our minor version locked withramda. So it will be a regular release