Skip to content

fix capturing group for matchAll#10136

Merged
jridgewell merged 3 commits intobabel:masterfrom
tanhauhau:tanhauhau/fix/match-all-capturing-regex
Jul 26, 2019
Merged

fix capturing group for matchAll#10136
jridgewell merged 3 commits intobabel:masterfrom
tanhauhau:tanhauhau/fix/match-all-capturing-regex

Conversation

@tanhauhau
Copy link
Copy Markdown
Member

@tanhauhau tanhauhau commented Jun 27, 2019

Q                       A
Fixed Issues? Fixes #10134
Patch: Bug Fix? 👍
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes?
License MIT

According to spec (https://github.com/tc39/proposal-string-matchall/blob/master/spec.md#stringprototypematchall--regexp-), when calling String.matchAll, the regex is recreated again with 'g' flag, and the current BabelRegExp constructor will take the 'g' flag as group descriptor.

I see there's no String.prototype.matchAll support in node.js, not sure how to test this

@babel-bot
Copy link
Copy Markdown
Collaborator

babel-bot commented Jun 27, 2019

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/11199/

@nicolo-ribaudo
Copy link
Copy Markdown
Member

In node 12 it is supported: you can use minNodeVersion in the test options.
Also, we should have another fixture to test it loading the core-js/features/string/match-all polyfill.

@tanhauhau
Copy link
Copy Markdown
Member Author

@nicolo-ribaudo apparently we dont have node 12 for the test env, https://github.com/babel/babel/blob/master/.travis.yml#L8 . somehow, it's supposed to be added here 80a5a2e, but the node env for travis is node 11, not node 12

@jridgewell
Copy link
Copy Markdown
Member

We can add node 12 to the Travis yaml

@existentialism
Copy link
Copy Markdown
Member

@tanhauhau we test node 12 via circle:

https://github.com/babel/babel/blob/master/.circleci/config.yml#L37

@tanhauhau
Copy link
Copy Markdown
Member Author

@nicolo-ribaudo how do i add core-js/features/string/match-all polyfill into the fixture?

@nicolo-ribaudo
Copy link
Copy Markdown
Member

Doesn't adding it to the dev dependencies and then requireing it work?

@mrtnzlml
Copy link
Copy Markdown

Hi, thanks so much for taking care of my reported issue. Is there anything I can do to help with moving it forward?

@tanhauhau tanhauhau force-pushed the tanhauhau/fix/match-all-capturing-regex branch from cef8fb4 to c727a1c Compare July 10, 2019 16:56
@tanhauhau
Copy link
Copy Markdown
Member Author

@mrtnzlml thanks for nudging, @nicolo-ribaudo i've added another fixture that test with core-js@v3 String.matchAll polyfill

"repository": "https://github.com/babel/babel/tree/master/packages/babel-plugin-transform-named-capturing-groups-regex",
"bugs": "https://github.com/babel/babel/issues",
"dependencies": {
"core-js-pure": "^3.0.0",
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be a devDependency?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops.. updated @existentialism

@tanhauhau tanhauhau force-pushed the tanhauhau/fix/match-all-capturing-regex branch from c727a1c to 1dacdd5 Compare July 11, 2019 00:41
@nicolo-ribaudo
Copy link
Copy Markdown
Member

I tried to use GitHub's "delete file" feature, thinking that it would have reverted the changes to a single file. I messed it up 😅

Copy link
Copy Markdown
Member

@nicolo-ribaudo nicolo-ribaudo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apart for that, this PR looks good

@mrtnzlml
Copy link
Copy Markdown

Hi @nicolo-ribaudo, me again... 😊 Would you please consider merging this PR into master?

@nicolo-ribaudo
Copy link
Copy Markdown
Member

@mrtnzlml We have a two-:heavy_check_mark:s policy for merging PRs; I'll try to find some reviewers!

@nicolo-ribaudo nicolo-ribaudo added PR: Bug Fix 🐛 A type of pull request used for our changelog categories PR: Needs Review A pull request awaiting more approvals labels Jul 19, 2019
@existentialism
Copy link
Copy Markdown
Member

I was just waiting on the revert of the package.json to +1

@tanhauhau tanhauhau force-pushed the tanhauhau/fix/match-all-capturing-regex branch from 857c9d2 to e4f9369 Compare July 20, 2019 15:25
@tanhauhau
Copy link
Copy Markdown
Member Author

Sorry my bad. fixed the wrong changes and reverts on package.json

@tanhauhau tanhauhau force-pushed the tanhauhau/fix/match-all-capturing-regex branch from e4f9369 to 2677f95 Compare July 20, 2019 15:27
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

outdated A closed issue/PR that is archived due to age. Recommended to make a new issue PR: Bug Fix 🐛 A type of pull request used for our changelog categories PR: Needs Review A pull request awaiting more approvals

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Broken string.matchAll behavior after compilation

8 participants