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
Add new decorators transform #14004
Add new decorators transform #14004
Conversation
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/50906/ |
e292ecc
to
28087b7
Compare
28087b7
to
ee06a98
Compare
a438971
to
8f2bed6
Compare
a0f9d4d
to
f922636
Compare
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.
I only reviewed the explainer and part of the helper so far.
@@ -0,0 +1,741 @@ | |||
/* @minVersion 7.16.6 */ | |||
|
|||
var getOwnPropertyDescriptor = Object.getOwnPropertyDescriptor; |
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.
Using non-hoistable top-level code can cause problems with circular dependencies. For example (disclaimer: I didn't read the transform yet so this output does not match it, but it shows the problem):
// a.js
import "./b.js";
var getOwnPropertyDescriptor = Object.getOwnPropertyDescriptor;
function _applyDecorators(Class, decs) {
getOwnPropertyDescriptor(Class, "foo");
}
export function getClass() {
class _Class {};
let Class = _applyDecorators(_Class, []);
return Class;
}
// b.js
import { getClass } "./a.js";
getClass();
When running node a.js
, getOwnPropertyDescriptor(Class, "foo");
will be run before getOwnPropertyDescriptor = Object.getOwnPropertyDescriptor
so it will throw that getOwnPropertyDescriptor
is not a function.
Since Babel assumes that built-in functions behave as in the spec and are not modified, we don't need to extract all these Object.*
and Array.*
methods. For the remaining variables, we can just inline them (with comments like /* CONSTRUCTOR */ 0
to explain what the inlined value is).
let ret = applyDecs( | ||
this, | ||
elementDecs, | ||
[dec] | ||
); | ||
|
||
initA = ret[0]; | ||
callB = ret[1]; | ||
initC = ret[2]; | ||
getC = ret[3]; | ||
setC = ret[4]; | ||
initInstance = ret[5]; | ||
Class = ret[6]; | ||
initClass = ret[7]; |
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.
We can probably get a slightly smaller output by doing
({
0: initA,
1: callB,
2: initC,
3: getC,
4: setC,
5: initInstance,
6: Class,
7: initClass,
} = applyDecs(
this,
elementDecs,
[dec]
));
1. `static #b`: This is the method itself, which being a private method we | ||
cannot overwrite with `defineProperty`. We also can't convert it into a | ||
private field because that would change its semantics (would make it | ||
writable). So, we instead have it proxy to the locally scoped `callB` | ||
variable, which will be populated with the fully decorated method. |
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.
Should this code log true
or false
?
function x() {}
function replace() {
return x;
}
class C {
@replace
static #m() {}
static getM() {
return this.#m;
}
}
console.log(C.getM() === x);
if it should log true
, then this proxy does not work correctly. We might decide that it's ok and it's a known limitation, but we could make it work by converting #m
to a field and replacing obj.#m = val
assignments with obj, val, _readOnlyError("#m")
calls (_readOnlyError
is an existing helper).
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.
This code should return true, but it definitely is not a common use case so it would be alright if we did not support this. I think the larger issue is that it seems private fields cannot be accessed prior to initialization, so for instance:
let getA;
class Foo {
#a = ((this.#b = (v) => v), this.#b(123));
#b;
getA() {
return this.#a;
}
}
(new Foo()).getA()
This code does not work. With a true private method, it would work (tested in Chrome at least), and I think this is generally a more common pattern so I think we'll still want to do the proxy method here.
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.
Uhm right, private methods would need to be "hoisted" before all the private fields when converting them.
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.
hmm, yeah I think we can do that and that should work actually, will update
@@ -7,6 +7,7 @@ import { | |||
FEATURES, | |||
} from "@babel/helper-create-class-features-plugin"; | |||
import legacyVisitor from "./transformer-legacy"; | |||
import transformer2020_09 from "./transformer-2020-09"; |
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: We can rename this to 2021-12
, since it's when it was last presented (without @init
).
wrapped with decorator code/logic. They then `delete` this temporary property, | ||
which is necessary because no additional elements should be added to a class | ||
definition. |
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.
Is this deleted property somewhere observable? i.e. can decorators access _Class
before it's deleted?
We might consider storing it as a private method (which then doesn't need to be deleted), and pass an additional array of "private accessors" to _applyDecorators
, like
applyDecs(
this,
elementDecs,
classDecs,
[o => o.#original_b]
);
If we end up using the "private method to fields" design suggested in the above comment, then we only need to do
static #b = function () {
console.log('foo');
}
and then pass it like
applyDecs(
this,
elementDecs,
classDecs,
[this.#b]
);
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.
So I came up with this before I realized that we could use static blocks (and in fact needed to in order to match the spec), and I just tested it out, we can actually refer to private names in static blocks. So we can fully extract private methods to the static block and avoid the delete in these cases 😄 I'll think about the best way to do this and update the code of applyDecs
accordingly, no need to add private class fields.
which the decorator will get via `Object.getOwnPropertyDescriptor`. They will be | ||
deleted from the class fully, so again the name is not important here, just |
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.
Also here, if possible I'd prefer to pass them in a new last param of applyDecs
rather than deleting.
{ | ||
"plugins": [ | ||
[ | ||
"proposal-decorators", | ||
{ "version": "2020-09", "decoratorsBeforeExport": false } | ||
], | ||
"proposal-class-properties", | ||
"proposal-private-methods", | ||
"proposal-class-static-block" | ||
] | ||
} |
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.
Can you copy all these tests to also see how the transform behaves when only compiling decorators?
6a86175
to
04b7188
Compare
packages/babel-plugin-proposal-decorators/src/transformer-2021-12.ts
Outdated
Show resolved
Hide resolved
packages/babel-plugin-proposal-decorators/src/transformer-2021-12.ts
Outdated
Show resolved
Hide resolved
} | ||
|
||
path.traverse({ | ||
AssignmentExpression(path) { |
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.
Edge cases: A member expression can also appears in a UpdateExpression or a destructuring assignment target:
this.#x++;
[...this.#x] = [];
for (this.#x of []);
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.
hmm, in those cases I don't believe that the readOnlyError
helper will really work, it'll be a syntax error. Should we just throw a compiler error if we find any assignments to the method? This may also be a capability we add in the future (see: tc39/proposal-decorators#438) so we could throw an error for now, and in the future if we end up supporting setting to decorated methods just remove that error.
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.
We already handle that case poorly (https://babeljs.io/repl#?browsers=chrome%2080&build=&builtIns=false&corejs=3.6&spec=false&loose=false&code_lz=MYGwhgzhAECC0G8BQ1oGIAeAKAlIgvitALYCmALgBYD2AJrokagNoB07VAlhK5gLrQAvNGZ8A3EUKEkAO1IB3OLlZkqdXEA&debug=false&forceAllTransforms=false&shippedProposals=false&circleciRepo=&evaluate=false&fileSize=false&timeTravel=false&sourceType=module&lineWrap=true&presets=env&prettier=false&targets=&version=7.16.6&externalPlugins=&assumptions=%7B%7D is wrong), so a compile-time error is fine for now.
04b7188
to
b75a922
Compare
} | ||
|
||
/** | ||
* Generates a new element name that is unique to the given class. This can be |
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.
"unique to the given class" might not be enough. It must be unique in the given class and in all the class enclosing the current one. For example:
class A {
#A = 1;
static B = class B extends A {
accessor a = 2;
getA() {
return this.#A;
}
}
new A.B().getA();
we don't want to transform it to
class A {
#A = 1;
static B = class B extends A {
#A = 2;
get a() { return this.#A }
set a(v) { this.#A = v }
getA() {
return this.#A;
}
}
new A.B().getA();
otherwise it returns 2
rather than 1
.
Now that this PR is mostly finished, could you add new commits without squashing 🙏 It makes it easier to see what changed since the last review.
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.
Ok, updated the logic to remove the public UID part because we no longer use that, and now it takes in all referenced PrivateNames within the class body, so it should be sufficiently unique. If a duplicate key does exist within a parent, I don't think it should matter in the child unless the child references it, correct me if I'm wrong there.
Now that this PR is mostly finished, could you add new commits without squashing 🙏 It makes it easier to see what changed since the last review.
No prob!
2e8d2d6
to
9960464
Compare
Feel free to ignore the |
import { types as t } from "@babel/core"; | ||
import syntaxDecorators from "@babel/plugin-syntax-decorators"; | ||
import ReplaceSupers from "@babel/helper-replace-supers"; | ||
import * as charCodes from "charcodes"; |
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.
Can you edit
Lines 197 to 199 in ddd93b9
test: ["packages/babel-generator"].map(normalize), | |
plugins: ["babel-plugin-transform-charcodes"], | |
}, |
charCodes
in this file?
if (legacy) { | ||
if (version !== undefined) { | ||
throw new Error("'version' can't be used with legacy decorators"); | ||
} |
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.
We could also allow version: "legacy"
, but throw if both version
and legacy
are set. Then in Babel 8 we can remove legacy: true
, to reduce the number of options.
* keeping the length of those names as small as possible. This is important for | ||
* minification purposes, since public names cannot be safely renamed/minified. | ||
* | ||
* Names are split into two namespaces, public and private. Static and non-static |
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.
This isn't true anymore, since we only use it for private fields?
let reifiedId = String.fromCharCode(...currentPrivateId); | ||
|
||
while (privateNames.has(reifiedId)) { | ||
incrementId(currentPrivateId); | ||
reifiedId = String.fromCharCode(...currentPrivateId); | ||
} | ||
|
||
incrementId(currentPrivateId); |
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, but we could compact this code a bit by doing
let reifiedId;
do {
reifiedId = String.fromCharCode(...currentPrivateId);
incrementId(currentPrivateId);
} while (privateNames.has(reifiedId));
and currentPrivateId
starts from []
rather than [charCodes.uppercaseA]
.
packages/babel-plugin-proposal-decorators/src/transformer-2021-12.ts
Outdated
Show resolved
Hide resolved
[_Foo, _initClass] = babelHelpers.applyDecs(Foo, [], [dec]); | ||
})(); | ||
|
||
babelHelpers.defineProperty(Foo, "field", 123); |
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.
Is it correct that this uses Foo
instead of _Foo
?
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.
So technically I don't think so, it should be using _Foo
. However, I think that it's not really possible to do this, particularly with static private fields. We would have to extract all of the static fields and assign them to _Foo
instead, but static private fields need to be defined within a class body to be valid.
I think there's a few options here:
- Keep it as is, and note this as a limitation of the transform. The biggest meaningful difference would be that static public fields would not be own properties of the class, which I don't think is very common behavior to rely on.
- Extract all static public fields, leave static private fields, and disregard ordering. I think that classes tend to rely on field initialization order more often than own-ness, so I don't think this is the best option.
- Extract all static public fields, but assign them within static blocks in between private field assignments. Probably the most accurate transform, but also the most work.
t.classMethod( | ||
"constructor", | ||
t.identifier("constructor"), | ||
[t.restElement(t.identifier("args"))], |
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.
We don't need ...args
if the class does not have superClass , right?
|
||
const elementDecs = [ | ||
[dec, 0, 'a'], | ||
[dec, 7, 'x', '#b'] |
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.
We need to update the example here because the method implementation is now moved to the static block so x
is no longer required.
also fix decoratorsBeforeExport error when version: "legacy" is specified
babel-parser 7.17 has materalized classStaticBlock
c560a5b
to
2fc8ad9
Compare
307948d
to
0876e7a
Compare
There was one minor change we discussed at the decorators meeting this week, which was to have the metadata objects inherit from |
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.
I already approved this PR, but there have been a bunch of new changes.
Co-authored-by: Nicolò Ribaudo <nicolo.ribaudo@gmail.com> Co-authored-by: Huáng Jùnliàng <jlhwung@gmail.com>
Co-authored-by: Nicolò Ribaudo <nicolo.ribaudo@gmail.com> Co-authored-by: Huáng Jùnliàng <jlhwung@gmail.com>
Merged to #13827 |
Co-authored-by: Nicolò Ribaudo <nicolo.ribaudo@gmail.com> Co-authored-by: Huáng Jùnliàng <jlhwung@gmail.com>
Co-authored-by: Nicolò Ribaudo <nicolo.ribaudo@gmail.com> Co-authored-by: Huáng Jùnliàng <jlhwung@gmail.com>
Adds the 2020-09 decorators transform, along with associated tests. See https://github.com/tc39/proposal-decorators for details on the proposal itself.
Resolves #12654.
Update by @JLHwung
Try this feature on REPL