-
Notifications
You must be signed in to change notification settings - Fork 13.2k
ES2020: fix String.prototype.matchAll type and description #62873
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
|
This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise. |
1 similar comment
|
This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise. |
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.
Copilot wasn't able to review any files in this pull request.
|
The TypeScript team hasn't accepted the linked issue #55843. If you can get it accepted, this PR will have a better chance of being reviewed. |
fa72d1b to
7decdc8
Compare
|
v6 will be in TS, v7 will be the Go-based implementation. |
|
phew, thanks :-) |
|
Will this show up in the nightly v6 build the same day it's merged? |
|
Yes. For safety... @typescript-bot test it |
|
Hey @jakebailey, the results of running the DT tests are ready. Everything looks the same! |
|
@jakebailey Here are the results of running the user tests with tsc comparing There were infrastructure failures potentially unrelated to your change:
Otherwise... Everything looks good! |
|
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@jakebailey Here are the results of running the top 400 repos with tsc comparing Everything looks good! |
|
I'm not sure this actually resolves the linked issue -- the linked issue talks about passing custom objects that implement the |
|
ah, that's true - it's more like the proper resolution to the incorrectly-closed #47310. |
|
That's critical context. If I had known this was actually for that issue, I would not have approved and merged this PR. It's well known that our lib types can be slightly more strict than "the spec technically allows this type because of implicit conversions". I could argue then that this function should accept number because that could be stringified, or something. |
|
#47310 was correctly closed and this shouldn't have been merged. This PR allows const someStr = "(";
"foo".matchAll(someStr)which throws a runtime error! It's super important that people understand that the argument is being regexified here |
|
Then that's a major implementation bug - the spec does not say to do that. https://tc39.es/ecma262/#sec-string.prototype.matchall |
|
I will file implementation bugs for every affected engine, but matchAll absolutely should not coerce to a regexp. |
|
@ljharb What is step 5 doing if not creating a regexp? |
|
shit, you're right. clearly my memory is incorrect, and i'm astonished it went in this way. Please do revert this PR, then, and I apologize for the confusion. |
|
FWIW, this would only have brought |
|
Yeah, if we have a string somewhere else, that is probably a mistake |
|
The more I've thought about it, the more I've realized that that was indeed the reason this horrific design choice was made - to be consistent with So yes, I think either both should allow a string, or neither should, and I'm perfectly content for it to be "neither". (same with all the other regex-taking string methods) |

Fixes #55843
(it would be very ideal to get this bugfix into v5, assuming v6 is the go implementation)