- Notifications
You must be signed in to change notification settings - Fork 26
[WIP] fix(propEq): improve propEq typings#73
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
base:develop
Are you sure you want to change the base?
Uh oh!
There was an error while loading. Please reload this page.
Conversation
Nemo108 commented Oct 15, 2023
- remove unnecessary constraint from propEq value: the function should receive any type values of object, no matter what parameter it received;
- add additional types to use with placeholder
Harris-Miller commented Jan 20, 2024
6f999a0 to f734c70CompareNemo108 commented Jan 21, 2024
@Harris-Miller done |
| str: string; | ||
| num: number; | ||
| int: number; | ||
| numLike: number| `${number}`; |
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.
${number} is a neat trick! Allows strings that only contain number characters, very cool
| }; | ||
| exportfunctionpropEq<KextendskeyofU,constU>(__: Placeholder,name: K,obj: U): (val: WidenLiterals<U[K]>)=>boolean; | ||
| exportfunctionpropEq<KextendskeyofU,constU>(val: WidenLiterals<U[K]>,__: Placeholder,obj: U): (name: K)=>boolean; | ||
| exportfunctionpropEq<KextendskeyofU,constU>(val: WidenLiterals<U[K]>,name: K,obj: U): boolean; |
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.
We don't want to WidenLiterals<U[K]> here. When we know the type of U, we want to constrain val to U[K]
typeObj={prop: 'foo'|'bar'};// we want to disallow strings other than `'foo'` and `'bar'` here for `val`propEq('biz','prop',obj);WidenLiterals<U[K]> is needed for the curried versions before of how typescript infers types
constdoesEqualFoo=propEq('foo','prop');// without `WidenLiterals<U[K]>`, the type would be `Record<'prop', 'foo'>`, but obj is `{prop: 'foo' | 'bar' }`, which is too wide for just `'foo'`.// with `WidenLiterals<U[K]>`, the type would be `Record<'prop', string>`, so `obj` is alloweddoesEqualFoo(obj)Typescript doesn't support currying like this that well in general, but we do the best we can with what we have
types/propEq.d.ts Outdated
| }; | ||
| exportfunctionpropEq<T,KextendsPropertyKey>(val: T,name: K): (obj: Record<K,T>)=>boolean; | ||
| exportfunctionpropEq<KextendskeyofU,U>(val: U[K],name: K,obj: U): boolean; | ||
| exportfunctionpropEq<KextendsPropertyKey>(__: Placeholder,name: K): Kextendsnumber ? { |
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.
Instead of the ternary, we do 2 overloads
exportfunctionpropEq<Kextendsnumber>(__: Placeholder,name: K): {// ...}exportfunctionpropEq<KextendsExclude<PropertyKey,number>>(__: Placeholder,name: K): {// ...}types/propEq.d.ts Outdated
| ? (array: Array<WidenLiterals<V>>)=>boolean | ||
| : (obj: Record<K,WidenLiterals<V>>)=>boolean; | ||
| <constU>(__: Placeholder,obj: U): (name: keyofU)=>boolean; | ||
| <KextendskeyofU,constU>(name: K,obj: U): boolean; |
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 don't think const U in needed here. U should be find.
You need to constrain U and K here though. If typeof U[K] as well. Turns out you don't need WidenLiterals here at all, in fact, it would actually break if U[K] was string | number but you passed a string to val: V.
See here: https://tsplay.dev/m3KbAm
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.
V extends U[K] ? U : never creates too strict constraint: it fires an error on any string I pass there
But I think I came up with a better solution
f734c70 to 895debcCompare* remove unnecessary constraint from propEq value: the function should receive any type values of object, no matter what parameter it received; * add additional types to use with placeholder
895debc to 1e22d85CompareHarris-Miller commented Feb 22, 2024
@Nemo108 I'm having to revert the other This MR is actually of of The additions made to #74 solved a different problem about |
| import{Placeholder}from'ramda'; | ||
| import{WidenLiterals}from'../util/tools'; |
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.
| import{Placeholder}from'ramda'; | |
| import{WidenLiterals}from'../util/tools'; | |
| import{Placeholder,WidenLiterals}from'../util/tools'; |
| exportfunctionpropEq(__: Placeholder): never; | ||
| exportfunctionpropEq<constV>(val: V): { | ||
| <Kextendsnumber>(name: K): <Uextendsany[]>(array: VextendsU[K] ? V[] : never)=>boolean; |
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.
Ah crap, I missed this when I left that other comment about this MR not being a problem
array: V extends U[K] ? V[] : never This is the thing we can't have. It breaks combining with other functions, egR.filter(R.propEq('type', 'something')). The predicate type signatures don't match because of the : never