Skip to content

Ensure proposal-unicode-property-regex runs before transform-unicode-regex#10347

Closed
meowtec wants to merge 2 commits intobabel:masterfrom
meowtec:fix-regex-property
Closed

Ensure proposal-unicode-property-regex runs before transform-unicode-regex#10347
meowtec wants to merge 2 commits intobabel:masterfrom
meowtec:fix-regex-property

Conversation

@meowtec
Copy link
Copy Markdown

@meowtec meowtec commented Aug 16, 2019 •
edited
Loading

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

base case:

"devDependencies": {
  "@babel/cli": "^7.5.5",
  "@babel/core": "^7.5.5",
  "@babel/preset-env": "^7.5.5"
},

babelrc:

{
  "presets": [
    ["@babel/preset-env", {
    }]
  ]
}

input:

/^\p{Unified_Ideograph}$/u;

throw with:

SyntaxError: Expected atom at position 3
    ^\p{Unified_I

input:

/^[\p{Unified_Ideograph}]+$/u;

output:

/^[IU_ad-in-pr\{\}]+$/;

@babel-bot
Copy link
Copy Markdown
Collaborator

babel-bot commented Aug 16, 2019 •
edited
Loading

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

JLHwung
JLHwung reviewed Aug 16, 2019
View reviewed changes
packages/babel-preset-env/test/fixtures/sanity/regex-unicode-property/exec.js Outdated
@@ -0,0 +1 @@
expect(/^\p{Unified_Ideograph}+$/u.test('中')).toBe(true);
Copy link
Copy Markdown
Contributor

@JLHwung JLHwung Aug 16, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Could you check if the /s flag works with unicode property escape?

expect(/\p{Unified_Ideograph}./su.test('中\n')).toBe(true);

Copy link
Copy Markdown
Author

@meowtec meowtec Aug 17, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

It got SyntaxError Expected atom ^\p{Unified_I because transform-dotall-regex runs before proposal-unicode-property-regex.

However, if I moved proposal-unicode-property-regex before transform-dotall-regex, it would get a wrong result. see mathiasbynens/regexpu-core#26

Copy link
Copy Markdown
Contributor

@JLHwung JLHwung Aug 17, 2019

Choose a reason for hiding this comment

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

When proposal-unicode-property-regex is applied before transform-dotall-regex, the . token is transformed to equivalent code points given the default dotAll: false, rendered the subsequent transform-dotall-regex inactive.

babel/packages/babel-plugin-proposal-unicode-property-regex/src/index.js

Lines 22 to 25 in eb3767d

node.pattern = rewritePattern(node.pattern, node.flags, {
unicodePropertyEscape: true,
useUnicodeFlag,
});

Preferably regexpu-core could offer a preserveDot: boolean option so that . token can be consumed by transform-dotall-regex after unicode-property transforms.

Note that we may workaround this by plug in dotAllFlags in proposal-unicode-property-regex,

rewritePattern(node.pattern, node.flags, {
  dotAllFlags: node.flags.includes("s")
}

/cc @mathiasbynens

@JLHwung JLHwung added area: plugin ordering PR: Bug Fix 🐛 A type of pull request used for our changelog categories labels Aug 16, 2019
@JLHwung JLHwung mentioned this pull request Aug 17, 2019
Closed
@meowtec
Move proposal-unicode-property-regex before transform-dotall-regex an…
3e40cee 
…d enable `dotAllFlag` in proposal-unicode-property-regex
@nicolo-ribaudo
Copy link
Copy Markdown
Member

nicolo-ribaudo commented Aug 19, 2019 •
edited
Loading

I would be in favor of creating a single plugin which delegates to regexpu, with options to toggle parsing/transform for the various feature. I will propose it to the team.

@mathiasbynens
Copy link
Copy Markdown
Contributor

mathiasbynens commented Aug 19, 2019

FWIW, I'm happy to defer to y'all for making the call on what's best for Babel. I'm open to changes on the regexpu-core side as well, if that would be helpful.

@JLHwung
Copy link
Copy Markdown
Contributor

JLHwung commented Aug 30, 2019

I would be in favor of creating a single plugin which delegates to regexpu, with options to toggle parsing/transform for the various feature. I will propose it to the team.

#10367 also justifies the approach to create a single regexpu plugin since namedGroups is also supported in regexpu.

@nicolo-ribaudo nicolo-ribaudo mentioned this pull request Sep 9, 2019
Closed
7 tasks
@JLHwung JLHwung mentioned this pull request Sep 16, 2019
Merged
1 task
@lock lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Jan 29, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Jan 29, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@JLHwung JLHwung JLHwung left review comments

Assignees

No one assigned

Labels

area: plugin ordering 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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@meowtec @babel-bot @nicolo-ribaudo @mathiasbynens @JLHwung