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

Fix: class transform should not drop method definition when key contains non-BMP characters #14897

Merged
merged 5 commits into from Sep 3, 2022

Conversation

JLHwung
Copy link
Contributor

@JLHwung JLHwung commented Aug 31, 2022

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

The PR starts from polishing the typings/docs of @babel/helper-function-name and then the type checker catches a bug of the class transform.

@JLHwung JLHwung added the PR: Bug Fix 🐛 A type of pull request used for our changelog categories label Aug 31, 2022
@babel-bot
Copy link
Collaborator

babel-bot commented Aug 31, 2022

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

@JLHwung
Copy link
Contributor Author

JLHwung commented Aug 31, 2022

image

I am so disappointed that the GitHub Action log still does not support non-BMP characters decades after they were introduced.

@liuxingbaoyu
Copy link
Member

I didn't even notice this failure due to the failed CI.😰

Comment on lines 203 to 215
* @template N The unamed expression type
* @param {({
* node: N;
* parent?: NodePath<N>["parent"];
* scope: Scope;
* id?: t.LVal | t.StringLiteral | t.NumericLiteral | t.BigIntLiteral;
* })} {
* node,
* parent,
* scope,
* id,
* } `node`, `parent` and `scope` are generally extracted from a given NodePath. `id` is the fallback naming source when the helper
* can not infer the function name from the AST
Copy link
Member

Choose a reason for hiding this comment

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

JSDoc does not support destructuring, but we can do something like this:

Suggested change
* @template N The unamed expression type
* @param {({
* node: N;
* parent?: NodePath<N>["parent"];
* scope: Scope;
* id?: t.LVal | t.StringLiteral | t.NumericLiteral | t.BigIntLiteral;
* })} {
* node,
* parent,
* scope,
* id,
* } `node`, `parent` and `scope` are generally extracted from a given NodePath. `id` is the fallback naming source when the helper
* can not infer the function name from the AST
* @param {NodePath<t.FunctionExpression | t.Class>} The function or class to name.

Also, id is not a fallback: if there is already an id we exit early, and the fallback behavior is to infer it.

Copy link
Contributor Author

@JLHwung JLHwung Aug 31, 2022

Choose a reason for hiding this comment

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

If we have node.id, then node is a named function/class expression, we exit early.
Then the helper tries to determine id from the AST, and if it can't determine and the fallback id is not provided, we exit early.
Then the helper determine a name from id, exit early if the name can't be determined.

In this way the id is the fallback naming source. And because id does not exist in a NodePath, we can't put NodePath here.

Comment on lines +8 to +10
babelHelpers.createClass(o, [{
key: "\uD842\uDFB7\u91CE\u5BB6",
value: function 𠮷野家() {}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In current main it is transformed to

  babelHelpers.createClass(o, [{
    key: "\uD842\uDFB7\u91CE\u5BB6"
  }]

here the value is dropped because nameFunction returns undefined.

@JLHwung JLHwung force-pushed the improve-helper-function-name-typings branch from 96f90d9 to 880f117 Compare August 31, 2022 18:51
@JLHwung JLHwung merged commit 0e0f123 into babel:main Sep 3, 2022
@JLHwung JLHwung deleted the improve-helper-function-name-typings branch September 3, 2022 00:55
@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 Dec 3, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 3, 2022
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.

None yet

4 participants