[@types/underscore] Collection and Array Tests - Map, Pluck, Filter, GroupBy, and Flatten#45763
Conversation
…rscore and chain functions to a single overload.
…tely instead of using Collection since using the latter behaves oddly with JQuery objects.
…n and removing _ChainOfArrays and IterateeMatcherShorthand.
… issue with the linter that seems to affect TS 3.0 specifically.
This reverts commit 1955fc1.
…flatten # Conflicts: # types/underscore/index.d.ts # types/underscore/underscore-tests.ts
|
@reubenrybnik Thank you for submitting this PR! This is a live comment which I will keep updated. Code ReviewsBecause you edited one package and updated the tests (👏), I can help you merge this PR once someone else signs off on it. Status
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": 45763,
"author": "reubenrybnik",
"owners": [
"borisyankov",
"jbaldwin",
"ccurrens",
"confususs",
"jgonggrijp",
"ffflorian",
"regevbr",
"peterblazejewicz",
"reubenrybnik"
],
"dangerLevel": "ScopedAndTested",
"headCommitAbbrOid": "6bd9cb7",
"headCommitOid": "6bd9cb75d38e5ba10fcfcf81dbd3792c918bccf6",
"mergeIsRequested": true,
"stalenessInDays": 0,
"lastCommitDate": "2020-07-04T22:34:09.000Z",
"reopenedDate": "2020-06-30T16:35:01.000Z",
"lastCommentDate": "2020-07-05T00:29:03.000Z",
"reviewLink": "https://github.com/DefinitelyTyped/DefinitelyTyped/pull/45763/files",
"hasMergeConflict": false,
"authorIsOwner": true,
"isFirstContribution": false,
"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-07-05T00:24:12.000Z",
"reviewersWithStaleReviews": [],
"approvalFlags": 2,
"isChangesRequested": false
} |
|
🔔 @borisyankov @jbaldwin @ccurrens @confususs @jgonggrijp @ffflorian @regevbr @peterblazejewicz - please review this PR in the next few days. Be sure to explicitly select |
|
@reubenrybnik The CI build failed! Please review the logs for more information. Once you've pushed the fixes, the build will automatically re-run. Thanks! |
|
This PR is dependent on the changes in #45748 for its CI check to succeed, but it was useful to be able to show the CI check failing, which is why I opened it early. I'm going to convert it to a draft for now, but reviewers may still feel free to start reviewing it if they'd like since aside from that check failure this PR is ready for review. |
|
Hey @reubenrybnik, I'll be happy to review this one as well, but I'll wait what happens in #45748 first. |
|
Sounds good, thanks for following up @jgonggrijp! |
|
👋 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? Comparison details 📊
It looks like nothing changed too much. I won’t post performance data again unless it gets worse. |
|
Thanks for making this PR super extra excellent with the addition of As I mentioned, I expected this to be the most complicated PR of the set; hopefully it will all be downhill from here 😄 |
|
Hmm, darn, I plugged this into my company's solution to sanity check all these changes and a pair of unpleasant things happened that I need to look into.
|
…atee function since that's really all that matters from its perspective to fix issues with using function iteratees on values of type any.
…ions as their wrapped values.
After applying these fixes, my company's solution now has four new and IMO correct errors when these changes are applied. One is a |
jgonggrijp
left a comment
There was a problem hiding this comment.
Thanks for making this PR super extra excellent with the addition of
IterateeandIterateeResult@jgonggrijp!
It was a pleasure! Though you should give me credit only for IterateeResult and the forthcoming reduction in overloads. Iteratee was your idea and it solves a problem I didn't foresee.
The only change I made in this last round of commits that you might not have expected is that I updated
string | numberconstraints in the iteratee types to the built-inPropertyKeytype ofstring | number | symbol. I tried out using symbols as iteratees in JS to sanity check that Underscore works with them before doing so and didn't run into any issues.
Symbols happen to work as iteratees, but I'm not comfortable with encouraging that. Currently, symbols are not actually supported in Underscore; all Underscore functions that proactively extract property names (through _.keys or _.allKeys) restrict themselves to enumerable keys, and properties with symbol keys are never enumerable. If you tell Underscore where to look, like when passing a symbol as iteratee, it will of course find the property, but this is an unfortunate coincidence rather than something you're supposed to do. This has confused people before, for example in jashkenas/underscore#2789.
Having a name for the key type union is useful though. Maybe you can set type EnumerableKey = string | number and then substitute that everywhere for PropertyKey.
Whether or not I made any changes that you might not approve of, though, DT still requires the most recent commit to be approved so I'm still going to need you to give this one more quick click to be able to merge it.
Oops, of course. Next time I expect more changes, I'll save myself the effort of approving ahead of time.
|
@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! |
Sounds reasonable to me @jgonggrijp, especially since one can still use a function iteratee if one wants to reference symbol properties. I've made the requested change. |
|
Also just to make sure you had seen them @jgonggrijp after my original message I did end up making two other changes outside of the scope of originally discussed changes that aren't super huge but that you might want to take a quick peek at; see this comment for more details. |
jgonggrijp
left a comment
There was a problem hiding this comment.
Also just to make sure you had seen them @jgonggrijp after my original message I did end up making two other changes outside of the scope of originally discussed changes that aren't super huge but that you might want to take a quick peek at; see this comment for more details.
Yes I saw them and I'm not opposed.
Good to go, I think!
|
Ready to merge |
|
I just published |
…rray Tests - Map, Pluck, Filter, GroupBy, and Flatten by @reubenrybnik * Adding type extractors and updating existing tests to use them. * Improving handling of mixed-iterability types by simplifying the Underscore and chain functions to a single overload. * Updating TypeOfCollection to check against List and Dictionary separately instead of using Collection since using the latter behaves oddly with JQuery objects. * Renaming a variable to something slightly more friendly. * Updating type definitions for map, pluck, filter, groupBy, and flatten and removing _ChainOfArrays and IterateeMatcherShorthand. * Adding tests for map, pluck, filter, groupBy, and flatten. * Fixing a test for @types/parse that breaks with these changes. * Updating more places to use returns instead of return because I want to. * Revising identifiers in tests. * Switch from $ExpectType to assignability checks for now due to an odd issue with the linter that seems to affect TS 3.0 specifically. * Updating a word in a comment that I forgot to update. * Revert "Fixing a test for @types/parse that breaks with these changes." This reverts commit 1955fc1. * Fixing a typo in a variable name. * Dropping KeysOfUnion and TypesOfUnionProperty for now in favor of a safer and simpler keyof T + string approach. * Reducing the dimensions of a deep flatten and changing the names of the resulting types. * Dropping array tests for most functions since testing lists should be sufficient to also cover arrays. * Adding a comment to every test group. * Update functions that take an iteratee to use an Iteratee type and updating map to use an IterateeResult type. * Shortening the names of nested list variables. * Also collapsing pluck to a single overload. * Switching groupBy back to allowing any rather than attempting to constrain to typical valid key types. * Dropping unnecessary `keyof T` constraints and updating `string` and `string | number` constraints to `PropertyKey` since from experimentation Underscore works just fine with symbols as well. * Wrapping return values for multi-line funciton declarations to their own line. * Updating UnderscoreStatic functions that don't technically need the collection type to use it anyway for consistency with those that do. * Renaming some mixed-dimension array variables one more time. * Adding property path tests for groupBy. * Changing "iterator" to "iteratee" in test group comments. * Adding property path tests for filter. * Updating IterateeResult to only care about the return type of an iteratee function since that's really all that matters from its perspective to fix issues with using function iteratees on values of type any. * Working around some _Chain return types incorrectly using non-collections as their wrapped values. * Using `T` rather than `TypeOfCollection<V>` in `Underscore.map` since I have it. * Applying a shorter variable name that I had missed copying over from DefinitelyTyped#45201. * Fixing consistency issues with two more test variable names. * Adding EnumerableKey and using that intstead of PropertyKey.
…rray Tests - Map, Pluck, Filter, GroupBy, and Flatten by @reubenrybnik * Adding type extractors and updating existing tests to use them. * Improving handling of mixed-iterability types by simplifying the Underscore and chain functions to a single overload. * Updating TypeOfCollection to check against List and Dictionary separately instead of using Collection since using the latter behaves oddly with JQuery objects. * Renaming a variable to something slightly more friendly. * Updating type definitions for map, pluck, filter, groupBy, and flatten and removing _ChainOfArrays and IterateeMatcherShorthand. * Adding tests for map, pluck, filter, groupBy, and flatten. * Fixing a test for @types/parse that breaks with these changes. * Updating more places to use returns instead of return because I want to. * Revising identifiers in tests. * Switch from $ExpectType to assignability checks for now due to an odd issue with the linter that seems to affect TS 3.0 specifically. * Updating a word in a comment that I forgot to update. * Revert "Fixing a test for @types/parse that breaks with these changes." This reverts commit 1955fc1. * Fixing a typo in a variable name. * Dropping KeysOfUnion and TypesOfUnionProperty for now in favor of a safer and simpler keyof T + string approach. * Reducing the dimensions of a deep flatten and changing the names of the resulting types. * Dropping array tests for most functions since testing lists should be sufficient to also cover arrays. * Adding a comment to every test group. * Update functions that take an iteratee to use an Iteratee type and updating map to use an IterateeResult type. * Shortening the names of nested list variables. * Also collapsing pluck to a single overload. * Switching groupBy back to allowing any rather than attempting to constrain to typical valid key types. * Dropping unnecessary `keyof T` constraints and updating `string` and `string | number` constraints to `PropertyKey` since from experimentation Underscore works just fine with symbols as well. * Wrapping return values for multi-line funciton declarations to their own line. * Updating UnderscoreStatic functions that don't technically need the collection type to use it anyway for consistency with those that do. * Renaming some mixed-dimension array variables one more time. * Adding property path tests for groupBy. * Changing "iterator" to "iteratee" in test group comments. * Adding property path tests for filter. * Updating IterateeResult to only care about the return type of an iteratee function since that's really all that matters from its perspective to fix issues with using function iteratees on values of type any. * Working around some _Chain return types incorrectly using non-collections as their wrapped values. * Using `T` rather than `TypeOfCollection<V>` in `Underscore.map` since I have it. * Applying a shorter variable name that I had missed copying over from DefinitelyTyped#45201. * Fixing consistency issues with two more test variable names. * Adding EnumerableKey and using that intstead of PropertyKey.
npm test.)npm run lint package-name(ortscif notslint.jsonis present).This PR continues the planned set that will together add up to #45201 and includes the following changes:
map,pluck,filter,groupBy, andflatten.Iterateetype that handles all the cases Underscore allows for iteratees and drops the matcher shorthand ofDictionary<T>formapsince that isn't accurate (it should be and now isPartial<T>).maptoIterateeResultto provide more specific return types for non-function iteratees where possible.map,filter, andgroupByto useIteratee.pluckto result in a specific type rather thananyif it is able to do so while still accepting unknown strings to handle type unions with differing properties and to avoid re-introducing [@types/underscore] v1.8.10 throws error on basic pluck usage. #33479._Chain.filter,_Chain.groupBy, and special cases of_Chain.flattento use the correct wrapped value typeVto partially fix @types/underscore error TS2322 for_.chainafter upgrade to v1.9 #36308.pluckandgroupByto work with both Lists and Dictionaries to partially fix Underscore collections functions should take objects #20623.flattento use conditional types to be able to provide correctly flattened shallow and deep return types for arrays of up to two dimensions and deprecates_ChainOfArrays(also removing that interface as a return type formapandgroupBy).collectoverloads with a reference tomapoverloads and allselectoverloads with a reference tofilteroverloads.Changes to
filterand_Chain.tapare included in this PR because some existing chain tests that involve those functions followed bymapbecame unhappy without updates to their return types. This is expected to be the largest and most complicated of the PRs in this set.