Skip to content
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

Support indices.groups when compiling named groups in regexps #15092

Merged
merged 1 commit into from
Oct 31, 2022
Merged

Support indices.groups when compiling named groups in regexps #15092

merged 1 commit into from
Oct 31, 2022

Conversation

ptomato
Copy link
Contributor

@ptomato ptomato commented Oct 28, 2022

Q                       A
Fixed Issues? None opened
Patch: Bug Fix? No
Major: Breaking Change? No
Minor: New Feature? Yes (?)
Tests Added + Pass? Yes
Documentation PR Link No
Any Dependency Changes? No
License MIT

If a RegExp has the /d flag (hasIndices), match objects should have an extra property .indices with an array of the character indices that each subgroup matched. The indices array itself has a property .groups giving the matched character indices (or undefined) for each named capturing group.

This doesn't do anything to enable support for the /d flag, the .hasIndices property, or the returned indices object, if the environment does not support it, but it will populate .indices.groups in _wrapRegExp if .indices is present, so that the transform-named-capturing-groups-regex and proposal-duplicate-named-capturing-groups-regex plugins can take advantage of that support if it exists.

@babel-bot
Copy link
Collaborator

babel-bot commented Oct 28, 2022

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

@nicolo-ribaudo nicolo-ribaudo added the PR: Bug Fix 🐛 A type of pull request used for our changelog categories label Oct 29, 2022
@nicolo-ribaudo
Copy link
Member

I'm marking this as a bugfix because it just adds a missing piece of a proposal we already implemented (duplicate named groups), and it only works in browsers that already support /d. It would be a new feature if it implemented full support for /d, which we don't have yet.

@nicolo-ribaudo
Copy link
Member

nicolo-ribaudo commented Oct 31, 2022

For the failing tests: Node.js 14 and older don't support /d, so they throw. You can fix it by:

If a RegExp has the /d flag (hasIndices), match objects should have an
extra property .indices with an array of the character indices that each
subgroup matched. The indices array itself has a property .groups giving
the matched character indices (or undefined) for each named capturing
group.

This doesn't do anything to enable support for the /d flag, the
hasIndices property, or the returned indices object, if the environment
does not support it, but it will populate .indices.groups in _wrapRegExp
if .indices is present, so that the
transform-named-capturing-groups-regex and
proposal-duplicate-named-capturing-groups-regex plugins can take
advantage of that support if it exists.
@nicolo-ribaudo nicolo-ribaudo changed the title Add support for indices.groups Support indices.groups when compiling named groups in regexps Oct 31, 2022
@nicolo-ribaudo nicolo-ribaudo merged commit d6fbba5 into babel:main Oct 31, 2022
@ptomato ptomato deleted the ncg-indices branch October 31, 2022 20:47
@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Jan 31, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 31, 2023
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants