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 static/proto initializers when there aren't class fields #14335

Merged
merged 3 commits into from Mar 8, 2022

Conversation

JLHwung
Copy link
Contributor

@JLHwung JLHwung commented Mar 7, 2022

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

The helper behaviour is now strictly aligned with the transformer:

if (kind !== FIELD) {
if (isStatic) {
requiresStaticInit = true;
} else {
requiresProtoInit = true;
}
}

Also added a script per #14334 (comment). Here is an an example output when we have uncommitted changes after build

Please re-run "make build" and checkout the following changes to git
HEAD detached at pull/14335/merge
Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
	modified:   packages/babel-helpers/src/helpers-generated.ts

no changes added to commit (use "git add" and/or "git commit -a")

/cc @wvq Please try if this PR fixes the issues you found when using decorators. Thank you!

@JLHwung JLHwung added PR: Bug Fix 🐛 A type of pull request used for our changelog categories Spec: Decorators labels Mar 7, 2022
@babel-bot
Copy link
Collaborator

babel-bot commented Mar 7, 2022

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

@JLHwung JLHwung force-pushed the fix-14242 branch 3 times, most recently from 313330f to 7a76b0b Compare March 7, 2022 16:42
Comment on lines +450 to +452
if (kind !== 0 /* FIELD */) {
staticInitializers = staticInitializers || [];
initializers = staticInitializers;
Copy link
Member

Choose a reason for hiding this comment

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

Nit, but this probably produces a smaller output: initializers = staticInitializers = staticInitializers || [].

Copy link
Member

Choose a reason for hiding this comment

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

Ah no it's ok, the minifier step already produces this code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

initializers = staticInitializers = staticInitializers || []

This is the exact output in helpers-generated by terser, so I don't bother to manually minify here.

Copy link
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.

Code looks good, let's see if the new CI check already catches some missing files 😬

@nicolo-ribaudo nicolo-ribaudo reopened this Mar 7, 2022
@JLHwung JLHwung force-pushed the fix-14242 branch 3 times, most recently from 5f57e57 to 965a1a6 Compare March 7, 2022 16:58
@wvq
Copy link

wvq commented Mar 8, 2022

:) It's work's now.

@nicolo-ribaudo
Copy link
Member

I slightly updated the PR title so that it fits in a commit message even with (#14335) at the end :)

@nicolo-ribaudo nicolo-ribaudo changed the title Initialize static/proto Initializers when we seen non-field class members Fix static/proto initializers when there aren't class fields Mar 8, 2022
@nicolo-ribaudo nicolo-ribaudo merged commit 0951316 into babel:main Mar 8, 2022
@nicolo-ribaudo nicolo-ribaudo deleted the fix-14242 branch March 8, 2022 17:27
@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 Jun 8, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 8, 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 Spec: Decorators
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants