[underscore] Fixes sortBy on chained array wrapper#40417
[underscore] Fixes sortBy on chained array wrapper#40417sheetalkamat merged 4 commits intoDefinitelyTyped:masterfrom
Conversation
…round Z[] claimed to return a Z. e.g. _.chain([1, 2, 3]).sortBy(x => -x).value() would claim to be of type number, not number[].
…rtBy on chained, wrapped array.
|
@ojhall 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 If no reviewer appears after a week, a DefinitelyTyped maintainer will review the PR instead. |
That's its implementation behaviour, even if not provided with an array to begin with.
|
👋 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. |
jgonggrijp
left a comment
There was a problem hiding this comment.
I'm just asking for clarification below, but I chose "Request changes" to inform the bot that there should be another round of feedback.
types/underscore/index.d.ts
Outdated
| * @see _.sortBy | ||
| **/ | ||
| sortBy(iterator?: _.ListIterator<T, any>, context?: any): _Chain<T>; | ||
| sortBy<TSort>(iterator?: _.ListIterator<T, TSort>, context?: any): _Chain<T, T[]>; |
There was a problem hiding this comment.
Is the new TSort type parameter really necessary, and if so, what for? I wonder whether the _Chain<T, T[]> return type wouldn't already be a sufficient solution by itself.
There was a problem hiding this comment.
You're right as far as I'm able to see - I cargo-culted it from the other sortBy implementation further up. I'll change that back. Thanks!
There was a problem hiding this comment.
My codebase is happy with the updated typings. 👍
|
@ojhall 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! |
|
🔔 @jgonggrijp - Thanks for your review of this PR! Can you please look at the new code and update your review status if appropriate? |
jgonggrijp
left a comment
There was a problem hiding this comment.
Looks completely sensible to me now. 👍
|
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! |
|
I just published |
Please fill in this template.
npm test.)npm run lint package-name(ortscif notslint.jsonis present).Select one of these and delete the others:
If changing an existing definition:
Calling
_([1, 2, 3]).chain()correctly returns a_Chain<number, number[]>type, due to:However, a subsequent
sortBycall then returns a_Chain<number, number>type, due to:As a result,
_([1, 2, 3]).chain().sortBy(x => -x).value()returns anumbertype instead ofnumber[], which doesn't match the implementation.This PR sorts that out by trying to more closely match the other
sortBytype specifications, specifically returning a_Chain<T, T[]>.