[@types/underscore] Fix #36308 - underscore has bad chain typings fix only for function 'map' (#36319) by Radullus · Pull Request #36510 · DefinitelyTyped/DefinitelyTyped

Radullus · 2019-06-28T06:45:42Z

Fix typing in 'underscore' for function 'map'

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.)
Add or edit tests to reflect the change. (Run with npm test.)
Follow the advice from the readme.
Avoid common mistakes.
Run npm run lint package-name (or tsc if no tslint.json is present).
Provide a URL to documentation or source code which provides context for the suggested changes: https://underscorejs.org/#map
https://underscorejs.org/#chain
Increase the version number in the header if appropriate.
If you are making substantial changes, consider adding a tslint.json containing { "extends": "dtslint/dt.json" }.

… only for function 'map' (#36319)

Radullus · 2019-06-28T06:51:14Z

@jgonggrijp @regevbr @renjfk Can you please review?

typescript-bot · 2019-06-28T07:22:40Z

@Radullus Thank you for submitting this PR!

🔔 @borisyankov @jbaldwin @ccurrens @confususs @jgonggrijp @ffflorian @regevbr - please review this PR in the next few days. Be sure to explicitly select Approve or Request Changes in the GitHub UI so I know what's going on.

If no reviewer appears after a week, a DefinitelyTyped maintainer will review the PR instead.

typescript-bot · 2019-06-28T07:42:11Z

👋 Hi there! I’ve run some quick performance metrics against master and your PR. This is still an experiment, so don’t panic if I say something crazy! I’m still learning how to interpret these metrics.

Let’s review the numbers, shall we?

Comparison details 📊

	master	#36510	diff
Batch compilation
Memory usage	51492152.0	56392808.0	+9.5%
Type count	8871	8869	0.0%
Assignability cache size	1383	1372	-0.8%
Subtype cache size	525	517	-1.5%
Identity cache size	0	0

Language service
Samples taken	1742	1755	+0.7%
Identifiers in tests	1742	1755	+0.7%
`getCompletionsAtPosition`
Mean duration (ms)	182.8	180.6	-1.2%
Median duration (ms)	178.9	176.3	-1.4%
Mean CV	16.8%	17.5%	+4.6%
Worst duration (ms)	275.5	270.0	-2.0%
Worst identifier	clone	map
`getQuickInfoAtPosition`
Mean duration (ms)	168.7	168.1	-0.4%
Median duration (ms)	166.6	166.3	-0.2%
Mean CV	16.6%	17.9%	+8.3%
Worst duration (ms)	258.8	246.8	-4.7%
Worst identifier	partial	max

It looks like nothing changed too much. I’m pretty lenient since I’m still an experiment, so take a look anyways and make sure nothing looks out of place.

If you have any questions or comments about me, you can ping @andrewbranch. Have a nice day!

jgonggrijp · 2019-06-28T11:13:26Z

I defer this one to @regevbr because he is currently working on a complete fix for the chain typings. I'm not sure whether the fix in the current PR will combine well with his work, but I think the unittest would be a welcome addition in any case.

regevbr

@Radullus Thanks for the PR!

The T in _Chain<T,V> represents the type of the object and V represents the object.
So for example, string[] should be _Chain<string,string[]> and _.Dictionary<number> should be _Chain<number, _.Dictionary<number>

The lines you changed made your test pass, but they would cause other tests, that should be there but are not :), to fail.

Please see the following suggestion to fix the issue:

DefinitelyTyped/types/underscore/index.d.ts

Line 5103 in d200340

    
           map<TResult>(iterator: _.ListIterator<T, TResult>, context?: any): _Chain<TResult>;

should be changed to

 map<TResult>(iterator: _.ListIterator<T, TResult>, context?: any): _Chain<TResult, TResult[]>;

DefinitelyTyped/types/underscore/index.d.ts

Line 5115 in d200340

    
           map<TResult>(iterator: _.ObjectIterator<T, TResult>, context?: any): _Chain<TResult>;

should be changed to

 map<TResult>(iterator: _.ObjectIterator<T, TResult>, context?: any): _Chain<TResult, _.Dictionary<TResult>>;

Can you please try it (after reverting your change) and see if the test passes? Also I agree with @jgonggrijp that more tests are always welcome, so can you please also add a test for map operation on an object?

typescript-bot · 2019-06-28T12:22:53Z

@Radullus One or more reviewers has requested changes. Please address their comments. I'll be back once they sign off or you've pushed new commits or comments. Thank you!

… only for function 'map' (#36319) Add more test and applay changes form review

Radullus · 2019-07-01T08:47:19Z

Hi @regevbr

I added more test, for mapping 'Dictionary'.
I apply charges suggested for Line 5103 in line DefinitelyTyped/types/underscore/index.d.ts d200340
I modified Line 5115 DefinitelyTyped/types/underscore/index.d.ts d200340.
Map function for 'chain' object don't return 'Dictionary' but return 'Array'. You can see some of my tested https://jsfiddle.net/Radullus/fsmj2rxa/37/

regevbr · 2019-07-01T08:43:36Z

types/underscore/index.d.ts

        * @see _.map
        **/
-        map<TArray>(iterator: _.ListIterator<T, TArray[]>, context?: any): _ChainOfArrays<TArray>;
+        map<TArray>(iterator: _.ListIterator<T, TArray>, context?: any): _ChainOfArrays<TArray>;


this should be reverted as current behavior is ok

done.
I have add test for this definition of 'map', but I have to comment part of the 'testing' witch check output because it will cause the test to fail.

regevbr · 2019-07-01T08:43:42Z

types/underscore/index.d.ts

        * @see _.map
        **/
-        map<TArray>(iterator: _.ObjectIterator<T, TArray[]>, context?: any): _ChainOfArrays<TArray>;
+        map<TArray>(iterator: _.ObjectIterator<T, TArray>, context?: any): _ChainOfArrays<TArray>;


this should be reverted as current behavior is ok

done.
I have add test for this definition of 'map', but I have to comment part of the 'testing' witch check output because it will cause the test to fail.

regevbr · 2019-07-01T08:52:32Z

types/underscore/underscore-tests.ts

+    'fake id': { name: 'curly', age: 60 },
+};
+
+let youngPeopleId: string[] = _.chain(usersData)


This test should actually fail, as performing filter on a a Dictionary returns T[], thus performing map on it will need the following iterator:

(value: T, index: number, list: T[]): TResult

The reason this test passes now is because of missing signature of filter for _Chain which should be:

filter(iterator: _.ObjectIterator<T, boolean>, context?: any): _Chain<T, T[]>;

But fixing it will not even help as the entire _Chain interface is malformed :-( and I'm working on a wide fix for it.
I would rather you only add tests that use the fixed map method

You have right. I will stick in this test to 'map' function. I interface definition I was trying to test.

regevbr · 2019-07-01T08:58:56Z

Thanks @Radullus!
The _Chain interface is completely messed up currently :(
I added some comments for you to review

typescript-bot · 2019-07-01T09:24:57Z

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

typescript-bot · 2019-07-01T09:25:35Z

Updated numbers for you here from f3f7a2f:

Comparison details 📊

	master	#36510	diff
Batch compilation
Memory usage	51629480.0	52528944.0	+1.7%
Type count	8871	8940	+0.8%
Assignability cache size	1383	1389	+0.4%
Subtype cache size	525	525	0.0%
Identity cache size	0	0

Language service
Samples taken	1742	1796	+3.1%
Identifiers in tests	1742	1796	+3.1%
`getCompletionsAtPosition`
Mean duration (ms)	180.6	178.2	-1.4%
Median duration (ms)	177.7	174.8	-1.6%
Mean CV	17.1%	16.8%	-1.8%
Worst duration (ms)	278.2	260.9	-6.2%
Worst identifier	templateSettings	chain
`getQuickInfoAtPosition`
Mean duration (ms)	167.9	164.3	-2.2%
Median duration (ms)	165.5	162.7	-1.7%
Mean CV	16.9%	16.2%	-4.4%
Worst duration (ms)	238.4	233.0	-2.3%
Worst identifier	partial	value

It looks like nothing changed too much. I’m pretty lenient since I’m still an experiment, so take a look anyways and make sure nothing looks out of place.

… only for function 'map' (#36319) - Add more test - revert part of typings for function

typescript-bot · 2019-07-01T13:08:15Z

Updated numbers for you here from c42c111:

Comparison details 📊

	master	#36510	diff
Batch compilation
Memory usage	51005080.0	53628304.0	+5.1%
Type count	8871	9005	+1.5%
Assignability cache size	1383	1403	+1.4%
Subtype cache size	525	551	+5.0%
Identity cache size	0	0

Language service
Samples taken	1742	1825	+4.8%
Identifiers in tests	1742	1825	+4.8%
`getCompletionsAtPosition`
Mean duration (ms)	183.9	185.1	+0.7%
Median duration (ms)	180.1	181.6	+0.8%
Mean CV	17.1%	17.4%	+1.6%
Worst duration (ms)	271.4	287.2	+5.8%
Worst identifier	object	templateSettings
`getQuickInfoAtPosition`
Mean duration (ms)	169.2	168.8	-0.2%
Median duration (ms)	167.4	166.0	-0.8%
Mean CV	17.0%	15.9%	-6.8%
Worst duration (ms)	257.6	246.2	-4.5%
Worst identifier	a	score

It looks like nothing changed too much. I’m pretty lenient since I’m still an experiment, so take a look anyways and make sure nothing looks out of place.

regevbr · 2019-07-01T13:35:44Z

types/underscore/underscore-tests.ts

+    .value();
+
+// Test map function with _ChainOfArrays<>
+let usersTable_2 /*: { age: number; name: string; id: string }[][]*/ = _.chain(usersData)


why did you remove the type here?
It is best to use the flatten() method here and then get the value

because the type by the underscore should be like this one in the comment, But with current typescript definition, There is missing one level of array nest.
{ age: number; name: string; id: string }[][] // type return by underscore
{ age: number; name: string; id: string }[] // type with is calculate base of current definitions
It would require more changes in the underscore typescript definition. And I'm not certain what should be changed?
interface _ChainOfArrays<T> extends _Chain<T[]>
to
interface _ChainOfArrays<T> extends _Chain<T[], T[][]>
it will not match with data return from function 'groupBy'

You are correct. Lets leave it be for now

regevbr · 2019-07-01T13:36:12Z

types/underscore/underscore-tests.ts

+    })
+    .value();
+
+let usersTable_3 /*: { score: number; fullName: string; login: string }[][]*/ = _.chain(usersTable)


same here - why did you remove the type?

typescript-bot · 2019-07-01T14:25:05Z

A definition owner 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!

typescript-bot · 2019-07-01T18:14:41Z

I just published @types/underscore@1.9.2 to npm.

…ain typi… (DefinitelyTyped#36510) * [@types/underscore] Fix DefinitelyTyped#36308 - underscore has bad chain typings fix only for function 'map' (DefinitelyTyped#36319) * [@types/underscore] Fix DefinitelyTyped#36308 - underscore has bad chain typings fix only for function 'map' (DefinitelyTyped#36319) Add more test and applay changes form review * [@types/underscore] Fix DefinitelyTyped#36308 - underscore has bad chain typings fix only for function 'map' (DefinitelyTyped#36319) - Add more test - revert part of typings for function * Comment part of test with currently didn't pass

[@types/underscore] Fix #36308 - underscore has bad chain typings fix…

0dbf774

… only for function 'map' (#36319)

Radullus requested a review from borisyankov as a code owner June 28, 2019 06:45

typescript-bot added the Popular package This PR affects a popular package (as counted by NPM download counts). label Jun 28, 2019

typescript-bot added the Awaiting reviewer feedback label Jun 28, 2019

regevbr suggested changes Jun 28, 2019

View reviewed changes

typescript-bot added Revision needed This PR needs code changes before it can be merged. and removed Awaiting reviewer feedback labels Jun 28, 2019

[@types/underscore] Fix #36308 - underscore has bad chain typings fix…

c1db64c

… only for function 'map' (#36319) Add more test and applay changes form review

regevbr reviewed Jul 1, 2019

View reviewed changes

Radullus added 2 commits July 1, 2019 13:54

[@types/underscore] Fix #36308 - underscore has bad chain typings fix…

d87f8e2

… only for function 'map' (#36319) - Add more test - revert part of typings for function

Comment part of test with currently didn't pass

f197216

regevbr reviewed Jul 1, 2019

View reviewed changes

regevbr approved these changes Jul 1, 2019

View reviewed changes

typescript-bot added Owner Approved A listed owner of this package signed off on the pull request. Merge:Express labels Jul 1, 2019

RyanCavanaugh merged commit 020e04a into DefinitelyTyped:master Jul 1, 2019

MichaelGatesDev mentioned this pull request Jul 15, 2019

Chain sorting an array does not return an array (Typescript) jashkenas/underscore#2812

Closed

reubenrybnik mentioned this pull request May 31, 2020

[@types/underscore] More comprehensive tests for Underscore collection and array functions + definition changes as appropriate #45201

Closed

10 tasks

reubenrybnik mentioned this pull request Jun 12, 2020

[@types/underscore] Collection and Array Tests - OOP, Chain, and Chunk #45304

Merged

8 tasks

Conversation

Radullus commented Jun 28, 2019

Uh oh!

Radullus commented Jun 28, 2019

Uh oh!

typescript-bot commented Jun 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

typescript-bot commented Jun 28, 2019

Uh oh!

jgonggrijp commented Jun 28, 2019

Uh oh!

regevbr left a comment • 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!

typescript-bot commented Jun 28, 2019

Uh oh!

Radullus commented Jul 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

Radullus Jul 1, 2019 • 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!

regevbr commented Jul 1, 2019

Uh oh!

typescript-bot commented Jul 1, 2019

Uh oh!

typescript-bot commented Jul 1, 2019

Uh oh!

typescript-bot commented Jul 1, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Radullus Jul 1, 2019 • 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!

typescript-bot commented Jul 1, 2019

Uh oh!

typescript-bot commented Jul 1, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

typescript-bot commented Jun 28, 2019 •

edited

Loading

regevbr left a comment •

edited

Loading

Radullus commented Jul 1, 2019 •

edited

Loading

Radullus Jul 1, 2019 •

edited

Loading

Radullus Jul 1, 2019 •

edited

Loading