-
Notifications
You must be signed in to change notification settings - Fork 13.2k
Add Array.prototype.findLast() and Array.prototype.findLastIndex() #49636
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Thanks for the PR! It looks like you've changed the TSServer protocol in some way. Please ensure that any changes here don't break consumers of the current TSServer API. For some extra review, we'll ping @sheetalkamat, @amcasey, @mjbvz, @minestarks for you. Feel free to loop in other consumers/maintainers if necessary |
|
|
|
@DanielRosenwasser Has there been any changes to the CLA bot? I believe it used to accept contributions from the Bloomberg fork automatically. Edit: Fixed now - thanks! |
sandersn
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good except that TransformFlags ran out of space. @rbuckton, do you have any ideas for how to move any other members out of it?
src/compiler/types.ts
Outdated
| ContainsES2016 = 1 << 10, | ||
| ContainsES2015 = 1 << 11, | ||
| ContainsGenerator = 1 << 12, | ||
| ContainsDestructuringAssignment = 1 << 13, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this now overlaps with ContainsTypeScriptClassSyntax = 1 << 13, which means we're officially out of space in TransformFlags. =(
Paging @rbuckton for advice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review! I've assumed the overlap was intentional from the ES2022 PR but apparently it was fixed later by shifting Markers. There is no more space for that now though.
tests/cases/compiler/findLast.ts
Outdated
| @@ -0,0 +1,27 @@ | |||
| // @target: es2023, esnext | |||
|
|
|||
| [0].findLast((item) => item === 0); | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a test for the generic overload of findLast?
src/compiler/types.ts
Outdated
| ES2020 = 7, | ||
| ES2021 = 8, | ||
| ES2022 = 9, | ||
| ES2023 = 10, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DanielRosenwasser: Do we normally add a new script target before that edition is ratified, or do we put everything in esnext until there's an official spec?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's okay to put it into 2023 since we're expecting ratification before the stable release of TS5.0.
|
I believe we wait for ratification because the target isn't stable. |
|
I'd vote to wait to add es2023 until that edition is ratified. It could go into esnext for now. |
|
I removed ES2023 target and left ES2023 lib as @DanielRosenwasser suggested. Could you take one more look @sandersn @rbuckton please? Thanks! |
sandersn
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Can you merge from main so before I merge it?
5b524d5 to
c7518db
Compare
|
@sandersn done! Rebased on main. |
|
(I am not a member of TypeScript; I just reviewed based on my experience. Do point out me if I made something wrong.) |
|
Current status: @m-basov is waiting on #50450 by @graphemecluster to be merged. Once that lands he will rebase this PR. |
|
@robpalme Thanks, I've just resolved the conflicts of my PRs. |
|
Can we just implement this based on the existing signatures of |
|
@DanielRosenwasser I suppose the only difference is whether or not the |
|
Sure, I will rebase my changes on the latest main branch. UPD. The branch is rebased now but CLA isn't working for some reason. |
c7518db to
a6a6e74
Compare
4cd2162 to
95fab91
Compare
|
Looks like we have some merge conflicts. Would you be able to fix those up? Also, @rbuckton would you be able to take a second look? |
|
@typescript-bot pack this |
|
Heya @DanielRosenwasser, I've started to run the tarball bundle task on this PR at 95fab91. You can monitor the build here. |
|
Heya @DanielRosenwasser, I've started to run the diff-based top-repos suite on this PR at 95fab91. You can monitor the build here. Update: The results are in! |
|
Heya @DanielRosenwasser, I've started to run the parallelized Definitely Typed test suite on this PR at 95fab91. You can monitor the build here. |
|
Hey @DanielRosenwasser, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running There is also a playground for this build and an npm module you can use via |
|
@DanielRosenwasser Here are the results of running the top-repos suite comparing Everything looks good! |
* Add .findLast(), .findLastIndex(), and es2023 target Signed-off-by: mbasov2 <[email protected]> * Update baselines Signed-off-by: mbasov2 <[email protected]> Remove ES2023 target (#75) Signed-off-by: mbasov2 <[email protected]>
Signed-off-by: mbasov2 <[email protected]>
95fab91 to
9bf9ee1
Compare
|
@DanielRosenwasser thanks for review. I've fixed the merge conflicts. @graphemecluster thanks for spotting the issue with |
|
@microsoft-github-policy-service agree company="Bloomberg" |
rbuckton
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit wary about adding an es2023 lib given that the committee has not yet ratified ES2023, but given that Array find from last is at Stage 4, this is probably fine.
Describe your changes
Add type definitions for
Array.prototype.findLast()andArray.prototype.findLastIndex()methods. Both methods were added to thees2023lib but the lib didn't exist so I've created it. The definitions are identical to.find()and.findIndex()but with updated comments.Testing performed
Added
findLast.tstest file where all Array interfaces try to call.findLast()and.findLastIndex().Also, tested it manually by running:
tscwith thees5target andlibs: []tscwith thees5target andlibs: ["es2023"]tscwith thees5target andlibs: ["esnext"]Additional context
The
es2023target were added using this PR #46291 as an example.Fixes #48829
Credits to Bloomberg colleagues who helped with reviews @acutmore @dragomirtitian @mkubilayk.