[Ramda]: fixed pick and omit type signatures to reflect projections#24936
Conversation
ProofOfKeags
changed the title
typescript-bot
added
the
Popular package
|
@CaptJakk Thank you for submitting this PR! 🔔 @donnut @mdekrey @mrdziuban @sbking @afharo @teves-castro @1M0reBug @hojberg @charlespwd @samsonkeung @angeloocana @raynerd @googol @moshensky @ethanresnick @leighman - please review this PR in the next few days. Be sure to explicitly select If no reviewer appears after a week, a DefinitelyTyped maintainer will review the PR instead. |
|
@CaptJakk The Travis CI build failed! Please review the logs for more information. Once you've pushed the fixes, the build will automatically re-run. Thanks! |
|
Build error fixed by #24942 |
|
There are some problems with the Diff and Omit types. Maybe it would be better to bump the minimum TS version to 2.8 and use Exclude? |
|
Is there a page where the problems are described? I'd be interested |
|
I did not use the "standard" diff and omit types. I modified them to properly exclude things that should be excluded by omit and pick respectively. Can you clarify what you mean by "problems with the Diff and Omit" ? |
|
Can I get a response on this? |
|
I did bot mean to booxk the progress on this PR, sorry! It were just a quick thought but if you have modified and tested the types for this exact purpose I'm sure they work as expected. |
types/ramda/index.d.ts
Outdated
| omit<T>(names: ReadonlyArray<string>, obj: T): T; | ||
| omit(names: ReadonlyArray<string>): <T>(obj: T) => T; | ||
| omit<T, K extends keyof T>(names: ReadonlyArray<K>, obj: T): Omit<T, K>; | ||
| omit<K extends keyof { [index: string]: any }>(names: ReadonlyArray<K>): <T>(obj: T) => Omit<T, K>; |
There was a problem hiding this comment.
Isn't keyof { [index: string]: any } just string?
There was a problem hiding this comment.
yes. Good call.
There was a problem hiding this comment.
Actually this won't work. When you change it to string it ruins the conditional typing that constrains the structure of the return value to which we bind the projection. The trouble is not that we want all strings, but that we want the set of indices present in the specific type the pick and omit functions are used on. If we don't do this, we can't guarantee how the projections behave when this library is used.
Unfortunately the tradeoff here is the readability of the type definitions vs the accuracy of the types that these functions produce. Since these definitions serve to aid the users I think that having a more complicated types that exclude more incorrect programs while including correct ones should be the tradeoff we make.
The desired change you requested would appear like the following:
omit(names: ReadonlyArray<string>): <T>(obj: T) => Omit<T, ???>As you can see, we can't actually describe the set of keys that should be missing in the return type. By making it the way it is, we can preserve not just the fact that the keys being excluded are strings, but are in fact the exact strings that are reflected in the array. This only works because the strings are themselves types in typescript.
There was a problem hiding this comment.
Upon further inspection it appears that I misunderstood what you were asking. We can simply change the following:
K extends keyof { [index: string]: any }to
K extends stringThere was a problem hiding this comment.
Yeah sorry for the unclear message earlier, you got exactly what I mean in the end
|
Additionally, after reviewing the documentation here: https://blogs.msdn.microsoft.com/typescript/2018/03/27/announcing-typescript-2-8/ It does not appear that the Exclude/Extract types will have the behavior desired here. This simply means that types that are assignable to one are excluded in another. This is particularly useful for sum types but will not work for projections, which is inherently what |
|
I have made the requested changes to reduce the verbosity of type signatures without sacrificing accuracy of the types. |
|
Could you add the same simplification to your Omit type, changing the Regarding TS2.8, Pick and Exclude can be combined to write omit: microsoft/TypeScript#21847 But I'm not sure if the minimum TS version should be updated yet. We can easily change the Omit type later on anyway. |
…akk/DefinitelyTyped into feature/pick-omit-type-correctness
|
Just added the update to the Omit type signature and rebased with master since my other change went through. This should be good to go now. |
|
A definition author has approved this PR ⭐️. A maintainer will merge this PR shortly. If it shouldn't be merged yet, please leave a comment saying so and we'll wait. Thank you for your contribution to DefinitelyTyped! |
5 participants
Please fill in this template.
npm test.)npm run lint package-name(ortscif notslint.jsonis present).*NOTE: There are unrelated linting/compile errors in this module that have arisen due to typescript@next. I can include fixes for them in this PR if you would like but I figured I'd keep it separate as to not clutter what was being changed with this PR.
Select one of these and delete the others:
If changing an existing definition:
tslint.jsoncontaining{ "extends": "dtslint/dt.json" }.SUMMARY: We have encountered situations where we used one of the projection functions (omit, pick) and trimmed out some properties of an object only to have the compiler not catch when we later try to reference that property. This fix makes Typescript aware of what properties exist on the projections of the original Object type. I tried to break it in numerous ways and it holds up. Feedback is welcome.