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.

Sign up for GitHub

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

Jump to bottom

Fix unicode handling in generated function names #14047

Merged
merged 4 commits into from Dec 20, 2021
Merged

Fix unicode handling in generated function names #14047

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
673c623
Fix: unicode encoding function name handling
The-x-Theorist Dec 13, 2021
af6dcf7
supportUnicodeId currently not working
The-x-Theorist Dec 15, 2021
70c508f
unicode encoding with transform-function-name working
The-x-Theorist Dec 17, 2021
feaed13
unicode-encoding test removed
The-x-Theorist Dec 20, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions packages/babel-helper-function-name/src/index.ts
View file
Open in desktop
Expand Up @@ -223,6 +223,8 @@ export default function (
name = id.name;
}

if (isFunction(node) && /[\u{10000}-\u{10ffff}]/u.test(name)) return;
Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious: won't this get transpiled into an unnecessarily complex regex without unicode flag?
I mean, /[\uD800-\uDFFF]/.test(name) would work as well, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nicolo-ribaudo suggested me to use this #14047 in this issue.

Copy link
Member

Choose a reason for hiding this comment

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

We transpile our code targeting Node.js 6, so we don't transpile the u flag. However yes, /[\uD800-\uDFFF]/.test(name) would work too.

Copy link
Contributor

Choose a reason for hiding this comment

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

BTW /[\uD800-\uDFFF]/ are not equivalent to /[\u{10000}-\u{10ffff}]/u because lone surrogate can be a valid string literal. 🤦

var o = {
  "\uD800"() {}
}

Copy link
Member

Choose a reason for hiding this comment

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

Ok so /[\uD800-\uDFFF]/ is the correct one in this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I will change it to /[\uD800-\uDFFF]/.


if (name === undefined) {
return;
}
Expand Down
3 changes: 3 additions & 0 deletions ...abel-plugin-transform-function-name/test/fixtures/function-name/unicode-encoding/input.js
View file
Open in desktop
@@ -0,0 +1,3 @@
var o = {
"\uD835\uDC9C"() {}
Copy link
Contributor

Choose a reason for hiding this comment

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

The new test cases use shorthand-properties which is not supported by function-name. We can either plug in transform-shorthand-properties or just use ObjectProperty instead.

var o = {
  "\uD835\uDC9C": function () {}
}

When unicode escapes are supported, the output should be

var o = {
  "\uD835\uDC9C": function 𝒜() {}
};

otherwise, the output should be

var o = {
  "\uD835\uDC9C": function () {}
};

In this case the plugin does nothing.

};
4 changes: 4 additions & 0 deletions ...-plugin-transform-function-name/test/fixtures/function-name/unicode-encoding/options.json
View file
Open in desktop
@@ -0,0 +1,4 @@
{
The-x-Theorist marked this conversation as resolved.
Show resolved Hide resolved
"sourceType": "unambiguous",
Copy link
Member

Choose a reason for hiding this comment

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

No need to specify it; the default (script) is ok to reproduce the bug.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I will change it.

"presets": ["env"]
Copy link
Member

Choose a reason for hiding this comment

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

The plugin enabled by preset env will not always be the same. Can you enable just this plugin instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I will enable only the plugin.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we don't use preset "env" it doesn't throw the error.

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe the plugin refers to the transform-function-name here. There is an options.json already, so it suffices to remove presets: ["env"].

}
3 changes: 3 additions & 0 deletions ...bel-plugin-transform-function-name/test/fixtures/function-name/unicode-encoding/output.js
View file
Open in desktop
@@ -0,0 +1,3 @@
var o = {
"\uD835\uDC9C": function () {}
};