[@types/underscore] More comprehensive tests for Underscore collection and array functions + definition changes as appropriate#45201
Conversation
… objects to also accept an ObjectIterator and combining similar definitions for map and collect to be consistent with these and other existing definitions like find and detect.
…yield Dictionaries instead of Lists since _(dict).each(...) and _(dict).forEach(...) both yield the original dict object that was passed in.
…core methods + converting tabs to spaces.
…I just realized that the technically incorrect _Chain<T> return type actually worked exactly like _ChainSingle<T> from the perspective of a .value() call, which means that adding undefined would technically be a breaking change of a specific call chain. Also removing some copy pasta in a few test namespaces.
…above the List version since otherwise List will always win.
…assing an array in will yield an array.
… or chain what they are wrapping rather than T[].
… be more accurate.
…foldr can take a MemoObjectIterator as well as a MemoIterator.
… aren't already and updating them to refer to the correct primary function on UnderscoreStatic.
…her Underscore invoke functions.
…opping overloads that aren't super useful since any covers everything.
|
@jgonggrijp I've prepped the Round 3c changes you requested for
These parameters are definitely always supplied by Underscore, so I don't think making them optional in the type used by Underscore functions would be the right move since it would require the addition of unnecessary type guards when trying to use their values. I guess it would probably be best to have them return a full |
Wait, I'm not following you anymore. Where would these typeguards have to be added if we were to make the second and third argument of
By "them", do you mean |
In the callback code of any function that uses interface CollectionIterator<T extends TypeOfCollection<V>, TResult, V = Collection<T>> {
(element: T, key?: CollectionKey<V>, collection?: V): TResult;
}
// admittedly not the best example code
_.find({ a: 'one', b: 'two' }, (element, key, collection) => {
if (key.length > 3) { // error: key is possibly 'undefined'
return true;
}
if (collection.a === element) { // error: collection is possibly 'undefined'
return true;
}
return false;
});
It would not reflect reality as well as it could, but it would make it more future-proof. I'm fine with making a separate one-parameter or optional-parameter version of |
|
How did you find out that the type checker would behave in the way that you described in your code example? I find it very counterintuitive. I would expect the As for |
Sorry if I'm not understanding your question here. I strongly suspected that that would be the behavior, so to verify I wrote exactly that code and observed the errors that occurred (I did change the text slightly to more specifically refer to the variable that the error was associated with). I admittedly relied on the IDE I was using rather than verifying through linting that that error did actually occur when compiled, but VS has been pretty reliable so far and running through the linter just now did provide the same errors 😄
As shown by the red on the right of that screenshot, this change makes a lot of tests unhappy.
There's some additional explanation around this in Do's and Don'ts that might be helpful here. The find signature does restrict what types of functions are allowed to be passed, but it also informs those functions that they can expect values for all three of the parameters. If Underscore were written in TS, the callback signature would also require the caller to provide all three parameters and error out if they didn't, while marking them as optional would allow Underscore to not supply them in some cases but also require the caller to check whether they were supplied, which I think makes sense. With regards to the signature restricting the types of functions, as noted in that Do's and Don'ts documentation one can always write a callback that accepts fewer arguments than the caller will supply without marking the parameters themselves as optional since in JS a function can always be given more arguments than it actually has parameters for without any errors occurring (and before rest parameters at least that was often common practice for things that needed to accept a theoretically infinite number of parameters).
Yup, that is definitely the case. That being said, if |
|
Thanks, it’s landing now. You are right that making the last two parameters of I need to think longer about this, so perhaps I should just let it rest until it manifests in an actual review. I do need to mention one caveat in advance: not all functions are actually guaranteed to call the iteratee with three arguments. The counter-example that comes to mind is that |
|
Good to know, thanks! I should probably include a fix for |
|
We did it @jgonggrijp! The full set of PRs minus cleanup are below. Introduction of testing strategy (Round 1) Introduction of commonly used types (Round 2) Collections (Round 3a) Arrays (Round 3b) Select Objects Functions (Round 3c) The items I have planned for cleanup are as follows, where each main bullet point will be a PR:
Let me know if you'd like to see anything added or moved around in that list, and thanks in advance for continuing this journey with me a little further! |
|
If I'm right, this is the plan we agreed about earlier, so there is nothing more I want to add. I look forward to the last bits! |
|
There are admittedly a few items I mentioned over the course of these PRs that I've become a little meh on:
|
|
Still working on the last PR, progress so far can be observed in this diff: master...reubenrybnik:underscore-tests-usage-tests-cleanup |
|
Sorry this has taken a while. I've made all the changes I want to make in the last PR at this point, but I'm running into an issue with the linter that I can't seem to hack my way around, so I may need to put together an issue in https://github.com/Microsoft/dtslint for further investigation. |
|
A nuance that escaped me here is that it seems like different versions of TS are expecting different results, which is why updating the $ExpectType declarations to match the original failures causes different failures - the run Initial errors: Errors when $ExpectType declarations are updated to use |
…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.
…ay Tests - Usage Tests Cleanup by @reubenrybnik (cherry picked from commit 61deea7)
|
The final set of PRs for this project are as follows: Introduction of testing strategy (Round 1) Introduction of commonly used types (Round 2) Collections (Round 3a) Arrays (Round 3b) Select Objects Functions (Round 3c) Cleanup The final diff for this PR contains changes from a few additional PRs (#45894, #46650, #47539) that were not technically part of this project but took place in this timeframe and worked toward the same ends as well as a contribution from @ProdigySim (#45842). Tons of thanks to @jgonggrijp, who was a tireless reviewer who made major contributions to the success of this project. Additional closing remarks for this project can be found in #47543. |
npm test.)npm run lint package-name(ortscif notslint.jsonis present).tslint.jsoncontaining{ "extends": "dtslint/dt.json" }. If for reason the any rule need to be disabled, disable it for that line using// tslint:disable-next-line [ruleName]and not for whole package so that the need for disabling can be reviewed.Changes in this PR include:
Changes I'm considering including in this PR but am not certain of include:
Additional potentially breaking changes that I'm considering making in future PRs that I could instead work on including in this one if that would be preferable include:
I work for a company that uses underscore fairly extensively in its codebase. Recently we looked into upgrading from 1.8.1 definitions to 1.9.4 but ran into some issues similar to those fixed in #36319, #36510, and #40417. I had originally planned on just fixing the additional issues as well, but when adding tests for those fixes I thought it might be more useful to add tests for more functions than just those that I was changing, and while adding tests I found several more changes that it seemed useful to make. I had originally hoped to create an initial PR that didn't include breaking changes (except for fixing inaccuracies that would have required definition consumers to use an assertion to get correctly running JS) and a subsequent one that did, but unfortunately that didn't end up being feasible. If it would be useful for me to split this up or reduce the set of changes made in some way, let me know.
@regevbr it looks like you may have been working on similar changes a while back as indicated by comments in #36319 and #36510 (which admittedly I wish I would have looked more closely at before today); if it would make more sense to do so, feel free to either incorporate some or all of these changes and/or tests into the changes you were planning to make instead or have me decline this PR in favor of similar changes that may be coming soon.
This includes several breaking changes to existing signatures, but I'm not sure whether it would be best to change the version here to 1.10 or 2.0 or fork these to a v2 of definitions or even possibly do nothing. 1.10 is at least a valid version of Underscore and doesn't appear to have added new functionality that would involve changes to typings, so it's probably the best option if these shouldn't be forked.
From the handbook, I did update code to use "any" in a few places where the return value of a callback parameter isn't useful to the parameter or return types of the function (e.g. sorting an array or determining uniqueness of items using the returned value of a callback) and also continued to use it in a few return types (see e.g. commit 8480687). I would have used "unknown" instead, but at this time I don't need to push the supported version up to 3.0. I'd personally prefer to keep it at or below the current lowest supported version of 2.9, but I'd be happy to bump it up if others feel differently.