Lodash: Add missing use cases by aj-r · Pull Request #20795 · DefinitelyTyped/DefinitelyTyped

aj-r · 2017-10-21T19:16:14Z

Fixes #20741. I've added support for some subtle use cases that broke with #19356, specifically for differenceBy and flatMap.

For the context of these changes, see:

Please fill in this template.

Use a meaningful title for the pull request. Include the name of the package modified.
Test the change in your own code. (Compile and run.)
Follow the advice from the readme.
Avoid common mistakes.
Run npm run lint package-name (or tsc if no tslint.json is present).

If changing an existing definition:

Provide a URL to documentation or source code which provides context for the suggested changes: https://lodash.com/docs/4.17.4#differenceBy, https://lodash.com/docs/4.17.4#flatMap
Increase the version number in the header if appropriate. (not appropriate)
If you are making substantial changes, consider adding a tslint.json containing { "extends": "dtslint/dt.json" }. (already exists)

dt-bot · 2017-10-21T19:16:16Z

types/lodash/index.d.ts

to authors (@bczengel @chrootsu @stepancar @ericanderson @Ailrun @e-cloud). Could you review this PR?
👍 or 👎?

typescript-bot · 2017-10-21T22:51:01Z

@aj-r Please address comments from the code reviewers.

karptonite · 2017-10-24T16:57:57Z

Note that this problem also affects differenceWith, perhaps even more, since differenceWith takes a callback, and there is no reason to think that the callback couldn't take different types for the first and second arguments.

Comparators should accept any truthy/falsy value, not just true/false. Functions affected: differenceWith, intersectionBy, intersectionWith, pullAllBy, pullAllwith

aj-r · 2017-10-25T12:47:58Z

@Andy-MS Any idea how to fix the GC error in Travis? I get the same error when running npm run lint lodash locally but have no idea how to fix it. It always happens when testing with TS 2.3.

ghost · 2017-10-25T14:05:32Z

Looks like we've fixed some bugs since ts2.3. Also, tests will fail in ts2.3 because type inference has been improved. (I don't run out of memory when just running tsc on the command line.) You could just specify // TypeScript Version: 2.4 in the header.

aj-r · 2017-10-26T18:52:22Z

@bczengel @chrootsu @stepancar @ericanderson @Ailrun @e-cloud Checks are now passing. Could you review this PR?

aj-r · 2017-10-26T22:16:44Z

I guess @thorn0 could also review this, since he's an author now.

thorn0 · 2017-10-26T23:00:43Z

Reverting the changes one by one and repeatedly running lint, you can find what breaks TS 2.3. That's what I did with my PR.

thorn0 · 2017-10-26T23:08:49Z

types/lodash/index.d.ts

    type ValueKeyIteratee<T> = ((value: T, key: string) => any) | string | [string, any] | PartialDeep<T>;
-    type Comparator<T> = (a: T, b: T) => boolean;
+    type Comparator<T> = (a: T, b: T) => any;
+    type Comparator2<T1, T2> = (a: T1, b: T2) => any;


Why the return type is any?

Lodash looks for any truthy or falsy value in the return value, so we shouldn't restrict it to just booleans. For example, see this comparator: https://github.com/DefinitelyTyped/DefinitelyTyped/pull/20795/files#diff-6a82a43b7b79f07644334583c1add52aR671. boolean | undefined should be an allowed return type because lodash will treat undefined as a falsy value.

I don't buy that logic -- if something will be treated as a boolean it should be typed as a boolean. Math.abs("-2") will work at runtime but isn't allowed by TypeScript.

Returning a non-boolean value in callbacks is a common use case that people have opened issues for in the past - see #18956. If we force a boolean return value in comparators, then for consistency I think we should also force boolean return values in functions like filter, some, and every. But that would break #18956.

Let's break it. TypeScript's philosophy in regard to this kind of forgiveness and implicit conversions seems to be clear and simple.

What if we allow the return type to be boolean | null | undefined? That would support the use case in #18956, but also prevent the user from returning bad things like strings, objects, or functions.

Then again, maybe it's not a big deal to force users to put !! in front of anything that would otherwise be undefined.

boolean | null | undefined looks like a good trade-off. In particular, there will still be an error if the user forgets to return a value (x => x vs x => { x }) due to the fact that TS makes a distinction between undefined and void.

In Java and the like, even conditions in if statements must be of boolean type, which was considered a step forward to type safety. Everyone seems to have been okay with this design decision.

This made me think, what if we change the type of ValueIteratee for functions like differenceBy from the current shape (value: T) => any to (value: T) => {} | undefined | null. If I'm not mistaken, {} | undefined | null means "any except void". And an iteratee callback that never returns a value is definitely a mistake.

thorn0 · 2017-10-27T16:23:19Z

types/lodash/index.d.ts

            iteratee: object
        ): boolean[];
    }
+    type T234 = number[][0];


Looks like debug leftovers.

Good catch, Thanks!

thorn0 · 2017-10-27T17:09:18Z

types/lodash/index.d.ts

    type ValueKeyIteratee<T> = ((value: T, key: string) => any) | string | [string, any] | PartialDeep<T>;
-    type Comparator<T> = (a: T, b: T) => boolean;
+    type Comparator<T> = (a: T, b: T) => any;
+    type Comparator2<T1, T2> = (a: T1, b: T2) => any;


Let's break it. TypeScript's philosophy in regard to this kind of forgiveness and implicit conversions seems to be clear and simple.

thorn0 · 2017-10-27T20:15:19Z

The rest of the PR looks good. Also you were going to fix isFunction, weren't you?

aj-r · 2017-10-28T12:55:53Z

Looks like someone else is fixing isFunction: #21015

aj-r · 2017-10-28T14:07:39Z

I've discovered that if you run:

node node_modules\dtslint\bin\index.js types lodash

instead of:

npm run lint lodash

then it will tell you the actual errors, instead of giving you the GC error.

aj-r · 2017-10-28T15:30:11Z

@thorn0 I have updated based on your comments. Please review again.

thorn0 · 2017-10-28T23:03:08Z

Great. Let's introduce an alias for boolean | null | undefined. How about Boolish?

aj-r · 2017-10-29T14:08:50Z

@thorn0 done

typescript-bot · 2017-10-29T23:56:20Z

Approved by a listed owner. PR ready to merge pending express review by a maintainer.

ghost · 2017-10-30T14:36:42Z

👎 for boolean | null | undefined , since the actual function does not look for null | undefined values -- it just treats the input as a boolean. Users who don't want to deal with nullness errors yet could just disable --strictNullChecks.

rbuckton

I'm concerned about the loss of type safety introduced with Comparator2 and the changes to the type parameters in many of the methods.

rbuckton · 2017-10-31T00:38:51Z

types/lodash/index.d.ts

-            values?: List<T>,
-            comparator?: Comparator<T>
-        ): T[];
+        differenceWith<T1, T2>(


This seems like an odd choice if T1 and T2 are not comparable (e.g. when comparator is undefined). I would recommend you leave the original overload and add this one with both values and comparator as required, otherwise the entire method is much less type safe.

RyanCavanaugh · 2017-10-31T20:27:11Z

@aj-r Please fix the failures indicated in the Travis CI log.

…fied

RyanCavanaugh · 2017-11-03T02:46:53Z

Approved by a listed owner. PR ready to merge pending express review by a maintainer.

RyanCavanaugh · 2017-11-03T02:46:54Z

@rbuckton - Thanks for your review of this PR! Can you please look at the new code and update your review status if appropriate?

aj-r · 2017-11-10T01:13:46Z

@rbuckton Can you review this again now that I have made your requested changes?

typescript-bot · 2017-11-10T20:33:35Z

@rbuckton - Thanks for your review of this PR! Can you please look at the new code and update your review status if appropriate?

DanielRosenwasser · 2017-11-11T02:10:38Z

Thanks for being patient!

aj-r added 2 commits October 21, 2017 09:09

DefinitelyTyped#20741: added a missing use case for differenceBy

af5a9eb

Added a missing use case for flatMap

04246ca

typescript-bot added Popular package This PR affects a popular package (as counted by NPM download counts). The Travis CI build failed labels Oct 21, 2017

typescript-bot added The Travis CI build failed and removed The Travis CI build failed labels Oct 22, 2017

aj-r mentioned this pull request Oct 23, 2017

[lodash] Better chaining wrappers, overload clean-up #19356

Merged

8 tasks

sheetalkamat added the Has Merge Conflict This PR can't be merged because it has a merge conflict. The author needs to update it. label Oct 23, 2017

typescript-bot removed the Has Merge Conflict This PR can't be merged because it has a merge conflict. The author needs to update it. label Oct 23, 2017

aj-r added 2 commits October 24, 2017 21:36

Fixed more functions with the same issue as DefinitelyTyped#20741.

564d9f0

Comparators should accept any truthy/falsy value, not just true/false. Functions affected: differenceWith, intersectionBy, intersectionWith, pullAllBy, pullAllwith

Merge with upstream

6a21b9d

aj-r force-pushed the add-use-cases branch from ce55d83 to 6a21b9d Compare October 25, 2017 01:40

aj-r added 2 commits October 25, 2017 17:11

Update to TS 2.4 to fix tests

ebfac08

Fix lint

8839c14

typescript-bot removed the The Travis CI build failed label Oct 26, 2017

thorn0 reviewed Oct 26, 2017

View reviewed changes

thorn0 reviewed Oct 27, 2017

View reviewed changes

Fixed test errors in TS 2.3

c31000f

Force iterators and comparators to return a value (disallow void)

5cf22a5

Add type alias for boolean | null | undefined

5ed4c19

thorn0 approved these changes Oct 29, 2017

View reviewed changes

typescript-bot added Author Approved labels Oct 29, 2017

Use boolean instead of Boolish

62d3afb

aj-r mentioned this pull request Oct 30, 2017

Lodash definitions not working on TS 2.5 #18956

Closed

rbuckton requested changes Oct 31, 2017

View reviewed changes

typescript-bot added Revision needed This PR needs code changes before it can be merged. and removed Merge:Express labels Oct 31, 2017

Force lists to have the same type if iteratee/comparator is not speci…

13d697e

…fied

RyanCavanaugh added The Travis CI build failed and removed Revision needed This PR needs code changes before it can be merged. labels Nov 1, 2017

Fix lint

b416b57

RyanCavanaugh added Merge:Express and removed The Travis CI build failed labels Nov 3, 2017

DanielRosenwasser merged commit 67f4cca into DefinitelyTyped:master Nov 11, 2017

aj-r deleted the add-use-cases branch November 12, 2017 02:20

andnp mentioned this pull request Nov 13, 2017

[@types/lodash] _.filter types break when combined with _.get #21485

Closed

thorn0 mentioned this pull request Jun 20, 2018

[lodash] Error in findLast definition #26680

Closed

4 tasks

Conversation

aj-r commented Oct 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dt-bot commented Oct 21, 2017

Uh oh!

typescript-bot commented Oct 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

karptonite commented Oct 24, 2017

Uh oh!

aj-r commented Oct 25, 2017

Uh oh!

ghost commented Oct 25, 2017 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aj-r commented Oct 26, 2017

Uh oh!

aj-r commented Oct 26, 2017

Uh oh!

thorn0 commented Oct 26, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thorn0 Oct 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thorn0 Oct 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thorn0 commented Oct 27, 2017

Uh oh!

aj-r commented Oct 28, 2017

Uh oh!

aj-r commented Oct 28, 2017

Uh oh!

aj-r commented Oct 28, 2017

Uh oh!

thorn0 commented Oct 28, 2017

Uh oh!

aj-r commented Oct 29, 2017

Uh oh!

typescript-bot commented Oct 29, 2017

Uh oh!

ghost commented Oct 30, 2017

Uh oh!

rbuckton left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RyanCavanaugh commented Oct 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RyanCavanaugh commented Nov 3, 2017

Uh oh!

RyanCavanaugh commented Nov 3, 2017

Uh oh!

aj-r commented Oct 21, 2017 •

edited

Loading

typescript-bot commented Oct 21, 2017 •

edited

Loading

ghost commented Oct 25, 2017 •

edited by ghost

Loading

thorn0 Oct 27, 2017 •

edited

Loading

thorn0 Oct 27, 2017 •

edited

Loading

RyanCavanaugh commented Oct 31, 2017 •

edited

Loading