Add new decorators transform #14004

pzuraq · 2021-11-28T01:05:14Z

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

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

babel-bot · 2021-11-28T01:07:46Z

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

packages/babel-plugin-proposal-decorators/CONTRIB.md

nicolo-ribaudo · 2021-12-05T21:42:20Z

@pzuraq I had to rebase #13827; could you rebase this one on top of that (removing the now duplicate commits

nicolo-ribaudo

I only reviewed the explainer and part of the helper so far.

nicolo-ribaudo · 2021-12-27T18:51:07Z

packages/babel-helpers/src/helpers/applyDecs.js

@@ -0,0 +1,741 @@
+/* @minVersion 7.16.6 */
+
+var getOwnPropertyDescriptor = Object.getOwnPropertyDescriptor;


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).

nicolo-ribaudo · 2021-12-27T18:57:12Z

packages/babel-plugin-proposal-decorators/CONTRIB.md

+    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];


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] ));

nicolo-ribaudo · 2021-12-27T19:08:23Z

packages/babel-plugin-proposal-decorators/CONTRIB.md

+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.


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).

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.

Uhm right, private methods would need to be "hoisted" before all the private fields when converting them.

hmm, yeah I think we can do that and that should work actually, will update

nicolo-ribaudo · 2021-12-27T19:13:00Z

packages/babel-plugin-proposal-decorators/src/index.ts

@@ -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";


Nit: We can rename this to 2021-12, since it's when it was last presented (without @init).

nicolo-ribaudo · 2021-12-27T19:17:35Z

packages/babel-plugin-proposal-decorators/CONTRIB.md

+  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.


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] );

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.

nicolo-ribaudo · 2021-12-27T19:18:26Z

packages/babel-plugin-proposal-decorators/CONTRIB.md

+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


Also here, if possible I'd prefer to pass them in a new last param of applyDecs rather than deleting.

nicolo-ribaudo · 2021-12-27T19:23:13Z

packages/babel-plugin-proposal-decorators/test/fixtures/2020-09-setters/options.json

+{
+  "plugins": [
+    [
+      "proposal-decorators",
+      { "version": "2020-09", "decoratorsBeforeExport": false }
+    ],
+    "proposal-class-properties",
+    "proposal-private-methods",
+    "proposal-class-static-block"
+  ]
+}


Can you copy all these tests to also see how the transform behaves when only compiling decorators?

packages/babel-plugin-proposal-decorators/src/transformer-2021-12.ts

JLHwung · 2021-12-29T20:16:17Z

packages/babel-plugin-proposal-decorators/src/transformer-2021-12.ts

+  }
+
+  path.traverse({
+    AssignmentExpression(path) {


Edge cases: A member expression can also appears in a UpdateExpression or a destructuring assignment target:

this.#x++; [...this.#x] = []; for (this.#x of []);

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.

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.

nicolo-ribaudo · 2021-12-31T00:11:32Z

packages/babel-plugin-proposal-decorators/src/transformer-2021-12.ts

+}
+
+/**
+ * Generates a new element name that is unique to the given class. This can be


"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.

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!

nicolo-ribaudo · 2021-12-31T17:55:36Z

Feel free to ignore the CI / Publish to local Verdaccio registry; it will be fixed by rebasing the feat-7.16.0/decorators branch.

nicolo-ribaudo · 2022-01-01T11:03:24Z

packages/babel-plugin-proposal-decorators/src/transformer-2021-12.ts

+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";


Can you edit

babel/babel.config.js

Lines 197 to 199 in ddd93b9

test: ["packages/babel-generator"].map(normalize),

plugins: ["babel-plugin-transform-charcodes"],

},

to also inline charCodes in this file?

nicolo-ribaudo · 2022-01-01T11:05:11Z

packages/babel-plugin-proposal-decorators/src/index.ts

  if (legacy) {
+    if (version !== undefined) {
+      throw new Error("'version' can't be used with legacy decorators");
+    }


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.

nicolo-ribaudo · 2022-01-01T11:05:54Z

packages/babel-plugin-proposal-decorators/src/transformer-2021-12.ts

+ * 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


This isn't true anymore, since we only use it for private fields?

nicolo-ribaudo · 2022-01-01T11:07:40Z

packages/babel-plugin-proposal-decorators/src/transformer-2021-12.ts

+    let reifiedId = String.fromCharCode(...currentPrivateId);
+
+    while (privateNames.has(reifiedId)) {
+      incrementId(currentPrivateId);
+      reifiedId = String.fromCharCode(...currentPrivateId);
+    }
+
+    incrementId(currentPrivateId);


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

nicolo-ribaudo · 2022-01-01T11:17:25Z

packages/babel-plugin-proposal-decorators/test/fixtures/2021-12-classes/initializers/output.js

+  [_Foo, _initClass] = babelHelpers.applyDecs(Foo, [], [dec]);
+})();
+
+babelHelpers.defineProperty(Foo, "field", 123);


Is it correct that this uses Foo instead of _Foo?

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.

JLHwung · 2022-01-03T22:55:13Z

packages/babel-plugin-proposal-decorators/src/transformer-2021-12.ts

+        t.classMethod(
+          "constructor",
+          t.identifier("constructor"),
+          [t.restElement(t.identifier("args"))],


We don't need ...args if the class does not have superClass , right?

JLHwung · 2022-01-03T23:06:39Z

packages/babel-plugin-proposal-decorators/CONTRIB.md

+
+const elementDecs = [
+  [dec, 0, 'a'],
+  [dec, 7, 'x', '#b']


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

pzuraq · 2022-01-16T14:33:00Z

There was one minor change we discussed at the decorators meeting this week, which was to have the metadata objects inherit from Object.prototype instead of being Object.create(null). Other than that, I think this should be good to go!

packages/babel-plugin-proposal-decorators/src/index.ts

nicolo-ribaudo

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>

nicolo-ribaudo · 2022-01-24T22:39:02Z

Merged to #13827

Co-authored-by: Nicolò Ribaudo <nicolo.ribaudo@gmail.com> Co-authored-by: Huáng Jùnliàng <jlhwung@gmail.com>

pzuraq changed the base branch from main to feat-7.16.0/decorators November 28, 2021 01:05

JLHwung reviewed Dec 1, 2021

View reviewed changes

packages/babel-plugin-proposal-decorators/CONTRIB.md Show resolved Hide resolved

nicolo-ribaudo mentioned this pull request Dec 5, 2021

Implement support for the new decorators proposal #13827

Merged

4 tasks

nicolo-ribaudo force-pushed the feat-7.16.0/decorators branch from e292ecc to 28087b7 Compare December 5, 2021 21:38

nicolo-ribaudo added PR: New Feature 🚀 A type of pull request used for our changelog categories Spec: Decorators labels Dec 13, 2021

nicolo-ribaudo force-pushed the feat-7.16.0/decorators branch from 28087b7 to ee06a98 Compare December 16, 2021 11:19

pzuraq force-pushed the add-new-decorators-transform branch 2 times, most recently from a438971 to 8f2bed6 Compare December 25, 2021 14:44

pzuraq marked this pull request as ready for review December 25, 2021 14:46

pzuraq force-pushed the add-new-decorators-transform branch 2 times, most recently from a0f9d4d to f922636 Compare December 25, 2021 15:26

nicolo-ribaudo added this to the v7.17.0 milestone Dec 27, 2021

nicolo-ribaudo reviewed Dec 27, 2021

View reviewed changes

JLHwung self-requested a review December 28, 2021 04:20

pzuraq force-pushed the add-new-decorators-transform branch 2 times, most recently from 6a86175 to 04b7188 Compare December 29, 2021 16:08

JLHwung reviewed Dec 29, 2021

View reviewed changes

pzuraq force-pushed the add-new-decorators-transform branch from 04b7188 to b75a922 Compare December 29, 2021 23:40

nicolo-ribaudo reviewed Dec 31, 2021

View reviewed changes

pzuraq force-pushed the add-new-decorators-transform branch 2 times, most recently from 2e8d2d6 to 9960464 Compare December 31, 2021 15:06

nicolo-ribaudo reviewed Jan 1, 2022

View reviewed changes

nicolo-ribaudo mentioned this pull request Jan 2, 2022

[babel 8] Remove support for the 2018-09 decorators proposal #12712

Merged

JLHwung reviewed Jan 3, 2022

View reviewed changes

nicolo-ribaudo and others added 11 commits January 16, 2022 14:15

Add test for _initProto with existing constructor

faf44db

Fix _initProto injection

5ba1365

Simplify _initProto injection for existing super()

9a876a2

Fix test

9ecadfd

add decoratorsVersion support to babel-standalone

62acebe

fix: throw when legacy and version are both specified

e67b698

also fix decoratorsBeforeExport error when version: "legacy" is specified

fix init

e52f1dc

refactor: centralize option validation in syntax-decorators

be798df

remove noop parser plugin

e9f56f3

babel-parser 7.17 has materalized classStaticBlock

fix test errors

351f158

fix TS errors

2fc8ad9

nicolo-ribaudo force-pushed the add-new-decorators-transform branch from c560a5b to 2fc8ad9 Compare January 16, 2022 13:15

Fallback to Symbol.for("Symbol.metadata")

0876e7a

nicolo-ribaudo force-pushed the add-new-decorators-transform branch from 307948d to 0876e7a Compare January 16, 2022 14:03

Object.create(null) -> standard object

938ce50

JLHwung reviewed Jan 16, 2022

View reviewed changes

packages/babel-plugin-proposal-decorators/src/index.ts Outdated Show resolved Hide resolved

JLHwung and others added 2 commits January 16, 2022 10:25

use legacy visitor when version is "legacy"

aa4d6e2

Fix interop between class and static decorators

1eb2e68

nicolo-ribaudo approved these changes Jan 17, 2022

View reviewed changes

Update contrib.md

df637f9

JLHwung approved these changes Jan 24, 2022

View reviewed changes

nicolo-ribaudo merged commit ec325e2 into babel:feat-7.16.0/decorators Jan 24, 2022

nicolo-ribaudo added a commit that referenced this pull request Jan 24, 2022

Add new decorators transform (#14004)

48e611d

Co-authored-by: Nicolò Ribaudo <nicolo.ribaudo@gmail.com> Co-authored-by: Huáng Jùnliàng <jlhwung@gmail.com>

nicolo-ribaudo added a commit that referenced this pull request Jan 24, 2022

Add new decorators transform (#14004)

6f05357

Co-authored-by: Nicolò Ribaudo <nicolo.ribaudo@gmail.com> Co-authored-by: Huáng Jùnliàng <jlhwung@gmail.com>

nicolo-ribaudo added a commit that referenced this pull request Jan 27, 2022

Add new decorators transform (#14004)

ff521a6

Co-authored-by: Nicolò Ribaudo <nicolo.ribaudo@gmail.com> Co-authored-by: Huáng Jùnliàng <jlhwung@gmail.com>

nicolo-ribaudo added a commit that referenced this pull request Feb 1, 2022

Add new decorators transform (#14004)

05611f6

Co-authored-by: Nicolò Ribaudo <nicolo.ribaudo@gmail.com> Co-authored-by: Huáng Jùnliàng <jlhwung@gmail.com>

github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label May 6, 2022

github-actions bot locked as resolved and limited conversation to collaborators May 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new decorators transform #14004

Add new decorators transform #14004

pzuraq commented Nov 28, 2021 •

edited by JLHwung

babel-bot commented Nov 28, 2021 •

edited

nicolo-ribaudo commented Dec 5, 2021

nicolo-ribaudo left a comment

nicolo-ribaudo Dec 27, 2021

nicolo-ribaudo Dec 27, 2021

nicolo-ribaudo Dec 27, 2021

pzuraq Dec 27, 2021

nicolo-ribaudo Dec 27, 2021

pzuraq Dec 29, 2021

nicolo-ribaudo Dec 27, 2021

nicolo-ribaudo Dec 27, 2021

pzuraq Dec 27, 2021

nicolo-ribaudo Dec 27, 2021

nicolo-ribaudo Dec 27, 2021

JLHwung Dec 29, 2021

pzuraq Dec 29, 2021

nicolo-ribaudo Dec 31, 2021

nicolo-ribaudo Dec 31, 2021

pzuraq Dec 31, 2021

nicolo-ribaudo commented Dec 31, 2021

nicolo-ribaudo Jan 1, 2022

nicolo-ribaudo Jan 1, 2022

nicolo-ribaudo Jan 1, 2022

nicolo-ribaudo Jan 1, 2022

nicolo-ribaudo Jan 1, 2022

pzuraq Jan 1, 2022

JLHwung Jan 3, 2022

JLHwung Jan 3, 2022

pzuraq commented Jan 16, 2022

nicolo-ribaudo left a comment

nicolo-ribaudo commented Jan 24, 2022

		@@ -0,0 +1,741 @@
		/* @minVersion 7.16.6 */

		var getOwnPropertyDescriptor = Object.getOwnPropertyDescriptor;

		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

	test: ["packages/babel-generator"].map(normalize),
	plugins: ["babel-plugin-transform-charcodes"],
	},

Add new decorators transform #14004

Add new decorators transform #14004

Conversation

pzuraq commented Nov 28, 2021 • edited by JLHwung

babel-bot commented Nov 28, 2021 • edited

nicolo-ribaudo commented Dec 5, 2021

nicolo-ribaudo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolo-ribaudo commented Dec 31, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pzuraq commented Jan 16, 2022

nicolo-ribaudo left a comment

Choose a reason for hiding this comment

nicolo-ribaudo commented Jan 24, 2022

pzuraq commented Nov 28, 2021 •

edited by JLHwung

babel-bot commented Nov 28, 2021 •

edited