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

Skip public class property transform when possible #12849

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

JLHwung
Copy link
Contributor

@JLHwung JLHwung commented Feb 22, 2021

Q                       A
Fixed Issues? Skip properties transform under certain circumstances (see below)
Patch: Bug Fix?
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes?
License MIT

In this PR we skip proposal-class-properties transform when all of the following conditions are satisfied

  • The targets has native support of class properties
  • There is no private element or static block inside class body
  • The class and its elements are not decorated

I think we should discuss about where we should put the granular compat data. In this case it is the public_class_fields used only in proposal-class-properties transform. I put this table in helper-create-features-plugin, which is how a third-party plugin author would have done if he/she wants to implement granular control on the plugin output given different api.targets().

@JLHwung JLHwung added the PR: Output optimization 🔬 A type of pull request used for our changelog categories label Feb 22, 2021
@babel-bot
Copy link
Collaborator

babel-bot commented Feb 22, 2021

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

@codesandbox-ci
Copy link

codesandbox-ci bot commented Feb 22, 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 e912525:

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

@JLHwung JLHwung force-pushed the skip-public-class-property-transform-when-possible branch 4 times, most recently from ad66f65 to 3657f4d Compare February 23, 2021 22:12
@nicolo-ribaudo
Copy link
Member

I think it's easier for us to keep all our own data in @babel/compat-data, but we can still keep the "granular data" in a different file from the main plugins data (similarly to what we do for bugfixes).

Fyi, I did something like this in #12899 where I re-used existing @babel/compat-data data.

@@ -19,6 +19,8 @@ import {
FEATURES,
isLoose,
} from "./features";
import { isRequired } from "@babel/helper-compilation-targets";
import compatData from "../data/compat.json";
Copy link
Member

Choose a reason for hiding this comment

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

Please don't directly import .json files, since it won't work when moving to native ESM 🙏

Ref #12759

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

What do you think about splitting the private and public transforms instead? If a class contains both private and public fields, we could still leave public fields intact.

@JLHwung
Copy link
Contributor Author

JLHwung commented Feb 25, 2021

If a class contains both private and public fields, we could still leave public fields intact.

I don't think we can leave public fields intact, for example when private elements are referenced in the initializer.

class C {
  #a = 1;
  b = this.#a;
}

@JLHwung JLHwung force-pushed the skip-public-class-property-transform-when-possible branch from 3657f4d to a196bc8 Compare February 25, 2021 14:25
@nicolo-ribaudo
Copy link
Member

Well that can be compiled to

class C {
  b = (INITIALIZE(this, #a, 1), this.#a);
}

which means

class C {
  b = (_a.set(this, { writable: true, value: 1 }), _classPrivateFieldGet(this, _a))
}

anyway, this PR is already a good step in the correct direction.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

None yet

3 participants