[@types/underscore] Collection and Array Tests - OOP, Chain, and Chunk by reubenrybnik · Pull Request #45304 · DefinitelyTyped/DefinitelyTyped

reubenrybnik · 2020-06-05T08:12:22Z

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/
Include tests for your changes

This PR is the first in a planned set that will add a standardized set of happy path tests for all Underscore Collection and Array functions (as well as OOP and Chain) and update the type definitions to pass those tests. Chunk is included in this PR because the return type for that function references Collection and it's important for that to not be the case sooner rather than later. The other changes are pretty straightforward for the most part, but I did remove _ChainSingle in favor of just using _Chain because Underscore provides several functions that it could be useful to call on a wrapped single value as part of a chain (bind/partial on functions, is* on anything, range on numbers, values on an object, etc.) to fix #7931. Because _Chain is defined as _Chain<T, V = T> and its value function is defined as value(): V;, replacing references to _ChainSingle<T> with _Chain<T> will provide an exact equivalent to _ChainSingle<T>'s value(): T; function.

An unpolished version of the full set of changes that I intend to make can be found in #45201, and the full set of PRs that I expect to create to accomplish this are below. I've prepared two additional branches for future PRs at this time (diff links are included below) but don't want to go further until I've had a chance to get feedback on these initial changes and the format of the tests in general.

Round 1 (a few base changes)

underscore-tests-chain-chunk (this PR)

Round 2 (new types that will be repeatedly referenced in other PRs)

underscore-tests-each-map-pluck-groupBy-flatten

Round 3 (the remaining Collection functions)

underscore-tests-reduce-reduceRight
underscore-tests-find-filter-reject
underscore-tests-where-findWhere
underscore-tests-every-some
underscore-tests-min-max
underscore-tests-sortBy-indexBy-countBy
underscore-tests-shuffle-sample
underscore-tests-toArray-size

Round 4 (the remaining Array functions)

underscore-tests-first-initial-last-rest
underscore-tests-without-partition
underscore-tests-union-intersection-difference-uniq
underscore-tests-compact-zip-unzip
underscore-tests-invoke-object
underscore-tests-indexOf-lastIndexOf-findIndex-lastIndex
underscore-tests-contains-sortedIndex

Update: After revisions, this ended up updating UnderscoreStatic.sample in a way that fixes #20440 (on that interface at least) because it redefined Collection<T> to not be an empty interface so T could be inferred from the provided collection.

…lists, one for dictionaries, and one for other values - that preserve the correct wrapped value types.

… using a disguised type assertion.

…ince several Underscore functions apply to single values (e.g. bind, partial, is*, and range).

… instead of any since that's very close to being the minimum supported version at this time.

typescript-bot · 2020-06-05T08:12:32Z

@reubenrybnik Thank you for submitting this PR! I see this is your first time submitting to DefinitelyTyped 👋 - keep an eye on this comment as I'll be updating it with information as things progress.

Code Reviews

Because you edited one package and updated the tests (👏), I can help you merge this PR once someone else signs off on it.

Status

✅ No merge conflicts
✅ Continuous integration tests have passed
✅ Most recent commit is approved by type definition owners or DT maintainers

All of the items on the list are green. To merge, you need to post a comment including the string "Ready to merge" to bring in your changes.

Diagnostic Information: What the bot saw about this PR

{
  "type": "info",
  "now": "-",
  "pr_number": 45304,
  "author": "reubenrybnik",
  "owners": [
    "borisyankov",
    "jbaldwin",
    "ccurrens",
    "confususs",
    "jgonggrijp",
    "ffflorian",
    "regevbr",
    "peterblazejewicz"
  ],
  "dangerLevel": "ScopedAndTested",
  "headCommitAbbrOid": "3835c33",
  "headCommitOid": "3835c33848aed8f30912d06e75e9074c51cf5d6d",
  "mergeIsRequested": true,
  "stalenessInDays": 0,
  "lastCommitDate": "2020-06-18T04:46:45.000Z",
  "lastCommentDate": "2020-06-18T21:08:31.000Z",
  "reviewLink": "https://github.com/DefinitelyTyped/DefinitelyTyped/pull/45304/files",
  "hasMergeConflict": false,
  "authorIsOwner": false,
  "isFirstContribution": true,
  "popularityLevel": "Popular",
  "anyPackageIsNew": false,
  "packages": [
    "underscore"
  ],
  "files": [
    {
      "filePath": "types/underscore/index.d.ts",
      "kind": "definition",
      "package": "underscore"
    },
    {
      "filePath": "types/underscore/underscore-tests.ts",
      "kind": "test",
      "package": "underscore"
    }
  ],
  "hasDismissedReview": false,
  "ciResult": "pass",
  "lastReviewDate": "2020-06-18T17:54:01.000Z",
  "reviewersWithStaleReviews": [],
  "approvalFlags": 2,
  "isChangesRequested": false
}

typescript-bot · 2020-06-05T08:12:33Z

🔔 @borisyankov @jbaldwin @ccurrens @confususs @jgonggrijp @ffflorian @regevbr @peterblazejewicz - 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.

types/underscore/index.d.ts

typescript-bot · 2020-06-05T09:07:04Z

👋 Hi there! I’ve run some quick measurements against master and your PR. These metrics should help the humans reviewing this PR gauge whether it might negatively affect compile times or editor responsiveness for users who install these typings.

Let’s review the numbers, shall we?

	master	#45304	diff
Batch compilation
Memory usage (MiB)	48.0	56.2	+17.1%
Type count	9178	10074	+10%
Assignability cache size	1422	1807	+27%

Language service
Samples taken	1844	2363	+28%
Identifiers in tests	1844	2363	+28%
`getCompletionsAtPosition`
Mean duration (ms)	145.4	149.6	+2.9%
Mean CV	12.2%	12.5%
Worst duration (ms)	228.3	421.0	+84.4% 🔸
Worst identifier	tap	simpleNumber
`getQuickInfoAtPosition`
Mean duration (ms)	137.8	141.3	+2.6%
Mean CV	12.1%	12.3%	+2.0%
Worst duration (ms)	180.9	405.4	+124.1% 🚨
Worst identifier	a	_

Looks like there were a couple significant differences—take a look at worst-case duration for getting completions at a position and worst-case duration for getting quick info at a position to make sure everything looks ok.

types/underscore/index.d.ts

types/underscore/underscore-tests.ts

reubenrybnik · 2020-06-05T22:48:34Z

Oh yeah, this should fix #7931 (added a mention of that to the description).

jgonggrijp

I have some comments I'd like to see addressed, though not necessarily by code changes.

I definitely think that you fixed some problems with chunk and I'm happy for _ChainSingle to go.

types/underscore/index.d.ts

types/underscore/underscore-tests.ts

typescript-bot · 2020-06-08T13:14:35Z

@reubenrybnik 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. If you disagree with the reviewer's comments, you can "dismiss" the review using GitHub's review UI. Thank you!

…ed any features from higher TS versions in this set of revisions since any is perferable to unknown.

…pended.

reubenrybnik · 2020-06-16T14:50:49Z

...I'm very confused now about what precisely the benchmark check is measuring...but I'll take the Perf: Same label 😄

jgonggrijp · 2020-06-16T16:04:07Z

I'm starting to like this more and more, @reubenrybnik. Please let me know when you want to try again whether I'll hit "approve".

reubenrybnik · 2020-06-17T18:02:20Z

@jgonggrijp sorry to have left this sitting for a bit, I think I have this in a reasonable state for a merge at this point if you don't mind merging a few tests that may get replaced later. I'll work on putting together either a direct switch or transition plan to a different testing strategy for chain as you had requested before putting up more PRs, but I can also put this in a draft state for a bit to get it off the board while I try to work that out if you prefer.

jgonggrijp · 2020-06-17T20:55:00Z

@reubenrybnik As far as you're concerned, is the current state of the current PR an improvement over the master branch (even though transitional)?

reubenrybnik · 2020-06-18T03:01:24Z

@jgonggrijp I believe it is, yes, though it's only the first part of what I'd like to achieve 😄

It fixes Underscore: chained reduce returns wrong type #7931, and while that does come at the expense at least for now of folks being allowed to make potentially less-than-useful chain calls it also allows them to make several useful ones that were not previously readily available to them
It provides more accurate overloads for the creation of underscore and chain wrappers that can be put to good use in future PRs
It updates each of the three Underscore interfaces to provide accurate and consistent results for chunk
It introduces tests that may not all be the most ideal in the long run but that I think are simple and straightforward and that are likely to help future authors not mistakenly make changes that break existing functionality

reubenrybnik · 2020-06-18T03:09:35Z

Wait, but I can make it slightly better! I can add the previously-failing example for #7931 as a test actually, which will be nice too because it's the kind of chain test you were hoping for @jgonggrijp 😄

… it works when switching from _ChainSingle to _Chain.

types/underscore/underscore-tests.ts

…_Chain since several Underscore functions apply to single values (e.g. bind, partial, is*, and range)." This reverts commit 5daf47b.

…m types when given values that are not collection types.

types/underscore/index.d.ts

reubenrybnik · 2020-06-18T21:01:13Z

I did one more sanity check of building my company's repository with the updates I made in my most recent set of changes, and the result was one new legitimate build error where we're passing an object to toArray that isn't a List or Dictionary, which the new definition of Collection<T> catches. It is worth noting that the object in question falls into the type of object mentioned here where it has a type that has properties which are technically all of a single type, but since it is possible for the actual JS object to be a larger type with more properties that just happens to conform to a reduced interface, I still think it is better to expect a Dictionary type that explicitly requires all object members to be of a single type through a declared index signature (and folks can assert that as needed if they happen to know that the object they're dealing with does meet those requirements).

reubenrybnik · 2020-06-18T21:05:56Z

Thanks a lot @jgonggrijp for all of your feedback and help in this PR! Your input definitely resulted in valuable improvements to the overall result and a good amount of food for thought for me, and I look forward to looking into tests for chain that will hopefully make these types more easily maintainable in the long term 😄.

reubenrybnik · 2020-06-18T21:08:31Z

Ready to merge

jgonggrijp · 2020-06-19T08:33:30Z

My pleasure!

@reubenrybnik

…rray Tests - OOP, Chain, and Chunk by @reubenrybnik * Updating both underscore and chain to have three overloads - one for lists, one for dictionaries, and one for other values - that preserve the correct wrapped value types. * Updating Underscore.value to return the wrapped value type instead of using a disguised type assertion. * Updating _Chain.chain to continue chaining the wrapped value type. * Removing the _ChainSingle interface in favor of always using _Chain since several Underscore functions apply to single values (e.g. bind, partial, is*, and range). * Adding tests for OOP Style and Chaining. * Adding myself to the set of Underscore type definition authors. * Adding a newline back to the end of the tests file. * Moving the minimum supported TS version to 3.0 so unknown can be used instead of any since that's very close to being the minimum supported version at this time. * Making some updates to summary comments. * Removing extra lines in tests. * Updating chunk. * Adding tests for chunk. * Changing the minimum TS version back to 2.8 since I probably won't need any features from higher TS versions in this set of revisions since any is perferable to unknown. * Reordering tests so new ones are appended to the file rather than prepended. * Adding a newline to the end of underscore-tests.ts. * Removing tests that explicitly specify generics and moving $ExpectType assertions to the ends of the lines that they're testing. * Adding tests for more specific list and dictionary types, switching to only using $ExpectType for result type testing, and getting rid of some extra scoping braces now that they're not needed. * Fixing incorrect JSDoc terminators for chunk functions. * Adding the example from DefinitelyTyped#7931 as a test to verify that it works when switching from _ChainSingle to _Chain. * Revert "Removing the _ChainSingle interface in favor of always using _Chain since several Underscore functions apply to single values (e.g. bind, partial, is*, and range)." This reverts commit 5daf47b. * Updating the Collection type to be more accurate and useful. * Updating _ChainSingle to be an alias to _Chain<TypeOfCollection<V>, V>. * Also updating the Underscore and chain functions to use never for item types when given values that are not collection types. * Adding a few more tests around ChainSingle results. Co-authored-by: Mike Ness <mness@clearcompany.com>

@reubenrybnik

…rray Tests - OOP, Chain, and Chunk by @reubenrybnik * Updating both underscore and chain to have three overloads - one for lists, one for dictionaries, and one for other values - that preserve the correct wrapped value types. * Updating Underscore.value to return the wrapped value type instead of using a disguised type assertion. * Updating _Chain.chain to continue chaining the wrapped value type. * Removing the _ChainSingle interface in favor of always using _Chain since several Underscore functions apply to single values (e.g. bind, partial, is*, and range). * Adding tests for OOP Style and Chaining. * Adding myself to the set of Underscore type definition authors. * Adding a newline back to the end of the tests file. * Moving the minimum supported TS version to 3.0 so unknown can be used instead of any since that's very close to being the minimum supported version at this time. * Making some updates to summary comments. * Removing extra lines in tests. * Updating chunk. * Adding tests for chunk. * Changing the minimum TS version back to 2.8 since I probably won't need any features from higher TS versions in this set of revisions since any is perferable to unknown. * Reordering tests so new ones are appended to the file rather than prepended. * Adding a newline to the end of underscore-tests.ts. * Removing tests that explicitly specify generics and moving $ExpectType assertions to the ends of the lines that they're testing. * Adding tests for more specific list and dictionary types, switching to only using $ExpectType for result type testing, and getting rid of some extra scoping braces now that they're not needed. * Fixing incorrect JSDoc terminators for chunk functions. * Adding the example from DefinitelyTyped#7931 as a test to verify that it works when switching from _ChainSingle to _Chain. * Revert "Removing the _ChainSingle interface in favor of always using _Chain since several Underscore functions apply to single values (e.g. bind, partial, is*, and range)." This reverts commit 5daf47b. * Updating the Collection type to be more accurate and useful. * Updating _ChainSingle to be an alias to _Chain<TypeOfCollection<V>, V>. * Also updating the Underscore and chain functions to use never for item types when given values that are not collection types. * Adding a few more tests around ChainSingle results. Co-authored-by: Mike Ness <mness@clearcompany.com>

mness-cc added 12 commits June 4, 2020 19:31

Updating both underscore and chain to have three overloads - one for …

6402c69

…lists, one for dictionaries, and one for other values - that preserve the correct wrapped value types.

Updating Underscore.value to return the wrapped value type instead of…

4e0e575

… using a disguised type assertion.

Updating _Chain.chain to continue chaining the wrapped value type.

b559905

Removing the _ChainSingle interface in favor of always using _Chain s…

5daf47b

…ince several Underscore functions apply to single values (e.g. bind, partial, is*, and range).

Adding tests for OOP Style and Chaining.

4335411

Adding myself to the set of Underscore type definition authors.

2159f89

Adding a newline back to the end of the tests file.

6014702

Moving the minimum supported TS version to 3.0 so unknown can be used…

f1ab657

… instead of any since that's very close to being the minimum supported version at this time.

Making some updates to summary comments.

70f6bf3

Removing extra lines in tests.

78a3270

Updating chunk.

a32ea9b

Adding tests for chunk.

21c1724

reubenrybnik requested a review from borisyankov as a code owner June 5, 2020 08:12

typescript-bot added Where is GH Actions? GH Actions didn't give a response to this PR Popular package This PR affects a popular package (as counted by NPM download counts). labels Jun 5, 2020

typescript-bot removed the Where is GH Actions? GH Actions didn't give a response to this PR label Jun 5, 2020

reubenrybnik commented Jun 5, 2020

View reviewed changes

types/underscore/index.d.ts Outdated Show resolved Hide resolved

typescript-bot added the Perf: Worse typescript-bot determined that this PR has a negative impact on compilation performance. label Jun 5, 2020

reubenrybnik commented Jun 5, 2020

View reviewed changes

types/underscore/index.d.ts Show resolved Hide resolved

reubenrybnik commented Jun 5, 2020

View reviewed changes

types/underscore/underscore-tests.ts Outdated Show resolved Hide resolved

jgonggrijp suggested changes Jun 8, 2020

View reviewed changes

typescript-bot added the Revision needed This PR needs code changes before it can be merged. label Jun 8, 2020

mness-cc added 2 commits June 9, 2020 03:04

Changing the minimum TS version back to 2.8 since I probably won't ne…

934b3e6

…ed any features from higher TS versions in this set of revisions since any is perferable to unknown.

Reordering tests so new ones are appended to the file rather than pre…

5071964

…pended.

typescript-bot removed the Revision needed This PR needs code changes before it can be merged. label Jun 9, 2020

typescript-bot added the Perf: Same typescript-bot determined that this PR will not significantly impact compilation performance. label Jun 16, 2020

mness-cc added 2 commits June 17, 2020 20:14

Fixing incorrect JSDoc terminators for chunk functions.

83a553f

Adding the example from DefinitelyTyped#7931 as a test to verify that…

93fcb9c

… it works when switching from _ChainSingle to _Chain.

reubenrybnik commented Jun 18, 2020

View reviewed changes

types/underscore/underscore-tests.ts Show resolved Hide resolved

mness-cc and others added 5 commits June 17, 2020 20:59

Revert "Removing the _ChainSingle interface in favor of always using …

c0d7c8c

…_Chain since several Underscore functions apply to single values (e.g. bind, partial, is*, and range)." This reverts commit 5daf47b.

Updating the Collection type to be more accurate and useful.

8f291ae

Updating _ChainSingle to be an alias to _Chain<TypeOfCollection<V>, V>.

f21a05c

Also updating the Underscore and chain functions to use never for ite…

735461e

…m types when given values that are not collection types.

Adding a few more tests around ChainSingle results.

3835c33

jgonggrijp reviewed Jun 18, 2020

View reviewed changes

types/underscore/index.d.ts Show resolved Hide resolved

jgonggrijp approved these changes Jun 18, 2020

View reviewed changes

typescript-bot added Owner Approved A listed owner of this package signed off on the pull request. Self Merge This PR can now be self-merged by the PR author or an owner labels Jun 18, 2020

typescript-bot merged commit 5a9e6a5 into DefinitelyTyped:master Jun 18, 2020

This was referenced Jul 19, 2020

[@types/underscore] Collection and Array Tests - Each, ToArray, and Partition #46120

Merged

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

Closed

This was referenced Aug 15, 2020

[@types/underscore] Collection and Array Tests - Shortening Cleanup #46799

Merged

Underscore: chained reduce returns wrong type #7931

Closed

Conversation

reubenrybnik commented Jun 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Code Reviews

Status

Uh oh!

typescript-bot commented Jun 5, 2020

Uh oh!

Uh oh!

typescript-bot commented Jun 5, 2020

Uh oh!

Uh oh!

Uh oh!

reubenrybnik commented Jun 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jgonggrijp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

typescript-bot commented Jun 8, 2020

Uh oh!

reubenrybnik commented Jun 16, 2020

Uh oh!

jgonggrijp commented Jun 16, 2020

Uh oh!

reubenrybnik commented Jun 17, 2020

Uh oh!

jgonggrijp commented Jun 17, 2020

Uh oh!

reubenrybnik commented Jun 18, 2020

Uh oh!

reubenrybnik commented Jun 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

reubenrybnik commented Jun 18, 2020

Uh oh!

reubenrybnik commented Jun 18, 2020

Uh oh!

reubenrybnik commented Jun 18, 2020

Uh oh!

jgonggrijp commented Jun 19, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

reubenrybnik commented Jun 5, 2020 •

edited

Loading

typescript-bot commented Jun 5, 2020 •

edited

Loading

reubenrybnik commented Jun 5, 2020 •

edited

Loading

reubenrybnik commented Jun 18, 2020 •

edited

Loading