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
Convert private methods to fields when they are supported #12250
Convert private methods to fields when they are supported #12250
Conversation
class B { | ||
#foo = 1; | ||
bar = 2; | ||
} | ||
|
||
let A = babelHelpers.decorate(null, function (_initialize) { | ||
"use strict"; | ||
|
||
class A { | ||
constructor() { | ||
_initialize(this); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Example of feature (1): the first class is not transformed.
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/33467/ |
|
||
export default declare((api, options) => { | ||
api.assertVersion(7); | ||
|
||
return createClassFeaturePlugin({ | ||
name: "proposal-private-methods", | ||
if (!api.targets || isRequired("proposal-class-properties", api.targets())) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@developit was proposing to add something like api.targets.support(name)
to the RFC, so that this line then becomes
if (api.targets.support("proposal-class-properties")) {
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 6a18821:
|
static #privateStaticFieldValue = Object.defineProperty({ | ||
t: this | ||
}, "_", { | ||
get: function () { | ||
return Cl.#PRIVATE_STATIC_FIELD; | ||
}, | ||
set: function (newValue) { | ||
Cl.#PRIVATE_STATIC_FIELD = `Updated: ${newValue}`; | ||
} | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The trick here is to rely on native getters/setters. By doing so we don't have to worry about replacing this.#foo
, this.#foo += 2
, etc. with complex function calls, since we can just replace this.#foo
with this.#foo._
.
used.fields = used.fields ?? elem; | ||
} | ||
|
||
if (elem.isPrivate() && elem.isProperty()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: consider merge this branch to
if (elem.isProperty()) {
used.fields = used.fields ?? elem;
}
...eatures-plugin/test/fixtures/plugin-proposal-class-properties/no-compile-method/options.json
Outdated
Show resolved
Hide resolved
packages/babel-plugin-proposal-private-methods/test/fixtures/accessors-modern/basic/exec.js
Outdated
Show resolved
Hide resolved
"highlightCode": false, | ||
"targets": { "node": 14 }, | ||
"plugins": [ | ||
["external-helpers", { "helperVersion": "7.1000.0" }], |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
46dae56
to
58a1e84
Compare
779a545
to
b6444ea
Compare
...vate-methods/test/fixtures/accessors-to-private-property/shadowed-nested-class-key/output.js
Outdated
Show resolved
Hide resolved
Can you rebase since #12251 was reverted? |
58a1e84
to
1f501e0
Compare
78451ad
to
61ec9b8
Compare
@JLHwung Wdyt about the last commit to reduce the output size for accessors? Using terser, the output size for the code in the original PR description is now 346 bytes: import o from"@babel/runtime/helpers/get";
import t from"@babel/runtime/helpers/getPrototypeOf";
var e,r;
export class A extends B{$incFoo=e||(e=function(){this.$foo+=o(t(A.prototype),"getIncrement",this).call(this)});$doubleFoo={__proto__:r||(r={get _(){return 2*this.t.$foo}}),t:this};$foo=2;$bar=3;run(o){return this.$incFoo(),this.$doubleFoo._}} |
t.objectProperty( | ||
t.identifier("get"), | ||
toMemoizedFunction(getter.node, path.scope, `get_${name}`), | ||
t.classMethod( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't it be t.objectMethod
?
1f501e0
to
7f6a7d8
Compare
4602035
to
35b4bff
Compare
CI error on Node.js 6 is related. |
085be04
to
7bb7925
Compare
0557d3c
to
6a18821
Compare
I was wondering if it would be better to implement it as a separate plugin, similarly to what we do for preset-env bugfixes. Or at least if we this plugin should have a |
This PR reduces the output size when targeting engines with native support for class fields, in two ways:
While (1) is just relaxing a check (but in the same package), (2) leverages the new
api.targets()
api, checking inside the private methods plugin if class fields are supported.This means that, when using
preset-env
and using the top-level targets option, the effective compat data forproposal-class-properties
change fromto
You can see here the output difference when targeting Node.js 12:
Input
Output (before this PR)
Terser output (before this PR): 667 bytes
Output (with this PR)
Terser output (with this PR): 370 bytes
(I had to replace
#
with$
, because terser doesn't support private fields yet)I don't how how to label this PR 😂 Maybe a new
[smaller output]
?