[lodash] Better chaining wrappers, overload clean-up #19356
Conversation
|
types/lodash/index.d.ts to authors (@bczengel @chrootsu @stepancar @ericanderson @Ailrun). Could you review this PR? Checklist
|
|
@aj-r Please fix the failures indicated in the Travis CI log. |
|
Checks are now passing @bczengel @chrootsu @stepancar @ericanderson @Ailrun |
acd53ee to
48d2b05
Compare
|
Thanks! |
|
Ouch, why were the generic type parameters for |
|
|
After this change I am getting ‘Index signature is missing’ when i do _.flatMap({a:1 b:2}); Is this intended? |
|
@alan-agius4 No, that was not indended. However, this code works for me in TS 2.4 and 2.2. What version are you using, and can you provide more context?
|
|
@aj-r I am using |
|
I am having issues with the definition for omit, typescript version 2.5.3 Using redux, we have a reducer state definition of: Trying to use omit to remove 'deleted' keys in the reducer now gets rejected by the compiler. For the deletion action the reducer returns: Result is now the compile error for the use of omit:
Given that the keys are variable anyway, the actual end result for omit in this case is actually still |
|
For now we can solve this by adding an overload for omit<T>(
object: Dictionary<T>,
...paths: Many<string | number>[]
): Dictionary<T>; |
|
I’m terribly confused. Why aren’t we using type Diff<T extends string, U extends string> = ({[P in T]: P } & {[P in U]: never } & { [x: string]: never })[T]; |
|
At least because |
|
Ah. Nm. I didn’t read the above code sample. |
|
@dawnmist ActivitiesByRef is probably typed wrong. Your definition says that no matter what I pass in as the key I will get back an Activity. In reality, I presume, you may also get an undefined. If you update your index definition to reflect this your compiler error will go away. |
|
In general, everything associated with those deep property paths is really difficult to type in a correct and/or practical way. For instance, I tried to improve types for declare function zipObjectDeep<T>(
paths: Array<string | number | Array<string | number>>,
values: T[]
): NestedDictionary<T>;
interface NestedDictionary<T> {
[ key: string ]: T | NestedDictionary<T>;
}But how would we use a value of the const a = zipObjectDeep(['foo'], [1]); // number | NestedDictionary<number>What can be done with Then I realized that interface NestedDictionary<T> {
[ key: string ]: T | NestedDictionary<T> | Array<T | NestedDictionary<T>>;
}Right? No, because arrays can contain other arrays ( The situation with |
|
@dawnmist I think the best solution in your case would be to change your ActivitiesByRef` definition to this: Since it's impossible for every key in That said, I think @thorn0 's overload makes a lot of sense, too, and we should add it, provided that it doesn't break any existing use cases: |
|
@ericanderson If you pass in a key that exists in the object, you are guaranteed an Activity in return. It is only when the key itself doesn't exist in the object that you'd get undefined. And in that case, it is undefined because it is not there, not because it is stored as an undefined value. So the object type should be right - but access functions would have the undefined as a possible output for invalid keys. That is a standard Redux pattern - storing a list of items in an object with a unique key to identify each item so you can update/remove/add/retrieve items to the list via the key. At no time should a key that exists in the object point to anything except a list item (in this case, an Activity). Otherwise, what is the point of a mapped type if the type can never be properly mapped? What you are saying is that a typescript declaration of is never a valid definition - despite this being a typical example from the Typescript Handbook. |
|
@dawnmist @ericanderson That's discussed in microsoft/TypeScript#13778 and microsoft/TypeScript#11238 (comment). TL;DR: those who really want it should add |
|
@alan-agius4 For that particular use case, you should use Unfortunately, I can't find a good way to handle this that doesn't involve an index signature. The best I can do is this: I'll add this overload. |
|
In my code bases, we set index values to be | undefined because code that access them need to do the check and people may forget otherwise. I think here in the non-nested case, the code is actually helping prevent bugs. The returned type is no longer valid even your type says “all keys have a defined value”. I’m the case where you might Object.keys, you can always add the exclamation point. There has long been a discussion about undefined as a value and undefined as in there is no key. This has come up a lot with things like Partial as optionality implies | undefined. |
|
@ericanderson I'm a fan of putting |
|
By the way, this pull request breaks either
|
|
|
|
Agreed. I'll fix this in #20795. |
Fixes #14337, Fixes #19228, Fixes #14748, Fixes #18956, Fixes #14282
Apologies for the massive PR. The fix for #14337 required me to modify the chaining wrappers for every single function, so I figured I should clean up some 8000 of the messy overloads along the way (which also fixed several other issues).
Unfortunately, this will be a breaking change in terms of what type arguments are accepted. So if anyone was explicitly passing type arguments (e.g.
_.chain([]).find<_.LoDashExplicitObjectWrapper<any>>('a')), their code may be broken. But I think in the long run this change should be more helpful than harmful, as it removes the need to explicitly pass type arguments in almost all cases.Summary of changes:
LoDashImplicitArrayWrapper,LoDashImplicitObjectWrapper,LoDashImplicitNumberArrayWrapper, etc.), there are now only 2 wrapper types:LoDashImplicitWrapperandLoDashExplicitWrapper.headandfind).iterateearguments more consistent - now they all use one of the newIterateetypes, unless they have a good reason not to.ObjectandFunctiontypes with more appropriate typesDictionary<T>inputs to just take plain objects, so an index signature is not requiredPlease fill out this template:
npm run lint package-name(ortscif notslint.jsonis present).Select one of these and delete the others:
If changing an existing definition:
tslint.jsoncontaining{ "extends": "dtslint/dt.json" }.