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

Compile static blocks without the intermediate priv field step #13297

Merged
merged 2 commits into from May 14, 2021

Conversation

nicolo-ribaudo
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo commented May 11, 2021

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

Consider this config:

{
  "presets": [
     {
       "plugins": ["proposal-class-static-block", "proposal-private-methods", "proposal-class-properties"]
     },
     "@babel/preset-env"
  ]
}

Until Babel 7.14.0, this was fine: the proposal-class-static-block was enabled before the other class features plugins, so that it could compile static blocks to private fields.

However, starting from Babel 7.14.0 @babel/preset-env automatically enabled the class fields and private methods plugins: the new effective order is

- proposal-private-methods
- proposal-class-properties
- proposal-class-static-block
- proposal-private-methods
- proposal-class-properties

This breaks, because the proposal-class-properties plugin runs before proposal-class-static-block (and the only way to workaround it is to change the presets order in the user config).

We can avoid this problem by transforming static blocks in Program:enter: it's safe, because the transform is quite self-contained so it doesn't cause problem if there are some proposals between the program node and the class node.

Regardless of the behavior change introduced in 7.14.0, this is something that makes it easier to configure Babel since it's one less thing our users have to worry about.

I hope to properly solve this problem with something like #13260.

@nicolo-ribaudo nicolo-ribaudo added the PR: Bug Fix 🐛 A type of pull request used for our changelog categories label May 11, 2021
// we are sure that static blocks have already been transformed regarless of the
// plugins order in the user config.
path.traverse({
Class(path: NodePath<Class>) {
Copy link
Member Author

Choose a reason for hiding this comment

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

This is just indented

@babel-bot
Copy link
Collaborator

babel-bot commented May 11, 2021

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

@codesandbox-ci
Copy link

codesandbox-ci bot commented May 11, 2021

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 6a7ab1b:

Sandbox Source
babel-repl-custom-plugin Configuration
babel-plugin-multi-config Configuration

@JLHwung
Copy link
Contributor

JLHwung commented May 12, 2021

Traversing the whole program to transform static block can be slow. And what if in the future we decide to compile some feature to Static Block? Can we apply the class transform on Class:exit? If so we can listen to StaticBlock:exit and their order constraints can be resolved, too.

Another thought may be introducing visitor.order: number, which can be considered as a generalization of start (Infinity) and exit (-Infinity), we can then allocate different orders to plugins so when they combine together, they work as we expected.

@nicolo-ribaudo
Copy link
Member Author

@JLHwung That would cause problems, because the private-in transform relies on the fact that private fields are transformed before visiting the class body to automatically disable itself. Another possibility is to just handle static blocks in the helper-class-features plugin.

@nicolo-ribaudo
Copy link
Member Author

I solved this in another way, that also makes the generated code smaller when compiling both static blocks and class fields.

Technically it's a new feature for the internal @babel/helper-create-class-features-plugin, but I think that it can go in a patch because from a user perspective it's just:

  • a relaxed plugin ordering constraint
  • a more optimized output

Comment on lines +12 to +14
Foo.foo = (_temp2 = _class2 = class {}, (() => {
_class2.bar = 42;
})(), _temp2).bar;
Copy link
Member Author

Choose a reason for hiding this comment

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

I think this is a bug (the super class is evaluated twice), but the bug was present also before this PR.

@nicolo-ribaudo nicolo-ribaudo added PR: Polish 💅 A type of pull request used for our changelog categories and removed PR: Bug Fix 🐛 A type of pull request used for our changelog categories labels May 12, 2021
@JLHwung JLHwung added PR: Output optimization 🔬 A type of pull request used for our changelog categories and removed PR: Polish 💅 A type of pull request used for our changelog categories labels May 12, 2021
@nicolo-ribaudo nicolo-ribaudo changed the title Remove ordering constraints for static-blocks plugin Compile static blocks without the intermediate priv field step May 13, 2021
@JLHwung JLHwung merged commit 8732dd3 into babel:main May 14, 2021
@nicolo-ribaudo nicolo-ribaudo deleted the static-blocks-order branch May 14, 2021 20:54
@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 Aug 14, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 14, 2021
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: Output optimization 🔬 A type of pull request used for our changelog categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: class-static-block works different on node 12 and 14+ after 7.14.0
4 participants