Add the `decoratorsAutoAccessors` parser plugin by pzuraq · Pull Request #13681 · babel/babel

pzuraq · 2021-08-16T02:56:10Z

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

This is the first part of the implementation of the latest version of
the decorators proposal: https://github.com/tc39/proposal-decorators

The keyword is outlined here in the README: https://github.com/tc39/proposal-decorators#class-auto-accessors

It adds a simpleAutoAccessors plugin which can be used to parse the
accessor keyword into ClassAccessorProperty and ClassPrivateAccessorProperty
node types.

babel-bot · 2021-08-16T03:01:05Z

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

codesandbox-ci · 2021-08-16T03:01:06Z

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 7cdc5a0:

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

packages/babel-parser/package.json

packages/babel-parser/src/parser/statement.js

packages/babel-parser/src/types.js

packages/babel-parser/src/parser/statement.js

packages/babel-parser/test/fixtures/experimental/accessor/basic-accessor/input.js

fedeci · 2021-08-16T10:12:22Z

Spec compliance or New feature?

nicolo-ribaudo · 2021-08-16T10:39:21Z

New feature, it's the first step to implement the new proposal.

JLHwung · 2021-08-16T15:02:23Z

On the AST changes, can you open a PR to the ESTree project? See also https://github.com/babel/babel/blob/main/CONTRIBUTING.md#creating-a-new-plugin-spec-new for the workflow of creating a new plugin.

nicolo-ribaudo · 2021-08-16T15:06:02Z

We don't follow ESTree for class fields anyway, right?

nicolo-ribaudo · 2021-08-16T15:26:44Z

packages/babel-parser/test/fixtures/experimental/decorators-modern/options.json

@@ -0,0 +1,3 @@
+{
+  "plugins": [["decorators", { "decoratorsBeforeExport": false, "version": "modern" }]]


Q: Is decoratorsBeforeExport still an undecided point in the new proposal?

We haven't discussed this in a long time, but I believe the status quo is that decorators will come after exports. There are some objectors to this, notably the Angular team, but most other objectors have withdrawn at this point.

nicolo-ribaudo · 2021-08-16T16:31:35Z

I'm reading tc39/proposal-decorators#426, https://github.com/tc39/proposal-decorators#class-auto-accessors and https://github.com/tc39/proposal-grouped-and-auto-accessors, and I find it slightly annoying that we have to implement this feature under the "decorators" plugin.

It feels like it should go under the plugin for https://github.com/tc39/proposal-grouped-and-auto-accessors. I 100% understand that this feature is needed for the decorators MVP, but it feels like it needs to be there just because the decorators proposal happens to be Stage 2 while https://github.com/tc39/proposal-grouped-and-auto-accessors is Stage 1.

There are a few options, considering a feature where we'll implement https://github.com/tc39/proposal-grouped-and-auto-accessors:

accessor x = 1 is enabled both by the decorators and the autoAccessors plugin
accessor x = 1 is only enabled by autoAccessors. We now add an incomplete autoAccessors plugin just for what's needed by the decorators proposal, and we'll complete it in the future.
accessor x = 1 is enabled by autoAccessors and by a temporary simpleAutoAccessors plugin (which is the intersection between the two proposals). When we'll release autoAccessors, we will documentation-deprecate simpleAutoAccessors.

I slightly prefer 3, but I'd like to wait for the opinion of other team members. The same question applies to the Babel transform plugin.

Also, regardless of what we choose, we should design the AST for accessor x = 1 to be forward-compatible with the other accessors proposal. I think I initially said that the .accessor: boolean property was ok, but now that I'm more aware about the grouped accessors proposal I fear that it won't be compatible with things like accessor x { get } = 1. We might need a new node type:

interface ClassAccessorProperty {
  type: "ClassAccessorProperty",
  key: Expression | PrivateName;
  computed: boolean;
  init?: Expression;

  // These will be added when implementing the other proposal
  get?: Identifier | PrivateName | ClassMethod | ClassPrivateMethod;
  set?: Identifier | PrivateName | ClassMethod | ClassPrivateMethod;
}

nicolo-ribaudo · 2021-08-16T16:35:48Z

Or maybe it's ok to use .accessor: true, and we'll introduce ClassAccessorProperty only when the accessor has explicitly { ... }.

pzuraq · 2021-08-16T16:58:32Z

@nicolo-ribaudo that's what I was thinking, since grouped and auto-accessors is currently at stage 1 and there's no guarantee that it will advance, there are a lot of concerns with certain aspects of the proposal. It is a bit annoying though, I would be open to adding a new type of node for this if you prefer to keep a cleaner separation.

I think option 3 makes the most sense as well, I can update the implementation accordingly.

JLHwung · 2021-08-16T17:41:49Z

We don't follow ESTree for class fields anyway, right?

Yes, the class fields unfortunately differ between Babel and ESTree, but we could coordinate early, for example to add accessor: boolean to both Babel's ClassProperty and ESTree's equivalent PropertyDefinition so we don't end up having more differences, let's say, hasAccessor: boolean in PropertyDefinition but not the other.

The idea is to port early proposals to the ESTree so in the future we don't introduce more discrepancy between two AST. E.g. StaticBlock is identical in Babel and ESTree.

...s/babel-parser/test/fixtures/experimental/decorators-modern/accessor/basic-accessor/input.js

pzuraq · 2021-08-21T17:52:01Z

Alright, this PR has been updated to add the simpleAutoAccessors plugin that @nicolo-ribaudo outlined. I also added a number of test cases to cover the different combinations of modifiers and options to accessor properties.

@JLHwung I'll make a separate PR to add the types to ESTree as soon as I have time, thanks for pointing that out!

packages/babel-parser/test/fixtures/experimental/decorators-modern/options.json

nicolo-ribaudo

Some missing tests with newlines:

```
class A {
  accessor
  x
}
```
```
class A {
  static accessor
  x
}
```
```
class A {
  accessor
  [x]
}
```

And a test for invalid swapped static/accessor:

```
class A {
  accessor static x
}
```
```
class A {
  accessor static
  x
}
```

packages/babel-parser/src/parser/statement.js

fedeci · 2021-09-01T16:02:00Z

CI errors are also related.

JLHwung · 2021-09-09T20:02:29Z

@pzuraq I have open an AST PR for ESTree: estree/estree#255 feedbacks are welcome.

nicolo-ribaudo · 2021-09-12T16:00:35Z

@JLHwung We diverge from ESTree when representing private fields and methods. I think we should also diverge when representing private accessors, using ClassPrivateAccessor, for internal consistency.

JLHwung · 2021-09-12T16:30:03Z

I tend to merge PrivateClassAccessor into ClassAccessor because unlike class methods/properties, a class accessor can have a public getter and a private setter:

class C { accessor a { get; #set } }

Should it be a PrivateClassAccessor or a PublicClassAccessor? I think people may have different opinions here but we don't have to classify them if we cover them with ClassAccessor.

nicolo-ribaudo · 2021-09-12T17:15:53Z

Ok, fair. I was thinking about the accessor #x = y case, but it doesn't map well to the extended proposal.

pzuraq · 2021-09-20T16:25:36Z

Ok, this PR has been updated with the extra test cases you gave @nicolo-ribaudo, and I've fixed the failing tests. Couple notes:

In keeping with the ESTree spec proposal, I changed init back to value for the accessor nodes, so they can essentially be extensions of class property/class private property. If we still want to have it be init instead, I can change it back.
For some reason, computed shows up on ClassPrivateAccessorProperty. I can't figure out how to remove it, and it isn't really necessary. Happy to remove if anyone can figure out how to do that.

Other than those issues, I think this PR is good to go!

JLHwung

For some reason, computed shows up on ClassPrivateAccessorProperty

Good catch! The computed: false is assigned by parsePropertyName when we parse accessor as a property name. Since then we decide to reinterpret it as an accessor keyword, the computed property gets no chance to be deleted.

Currently we have similar issues on private accessors / async private methods. The AST will contain computed: false even though computed is not in the ClassPrivateMethod interface.

Note that deleting a property from AST node is a perf killer. To fix this issue we can move computed assignment out of parsePropertyName and assign it when we are sure the consumed token is a class modifier / key identifier. This can be in a separate PR so it can get merged in patch releases.

packages/babel-compat-data/data/plugins.json

packages/babel-types/src/definitions/core.ts

JLHwung · 2021-09-21T01:28:34Z

packages/babel-types/src/definitions/core.ts

+      optional: true,
+    },
+    typeAnnotation: {
+      validate: process.env.BABEL_8_BREAKING


Since it is a new AST type, we can use the Babel 8 behaviour directly.

How do we do that exactly? Not familiar with the breaking behavior, I copied this from the other definitions

Here you can assume process.env.BABEL_8_BREAKING is always true and reduce the conditional expression to its true branch.

nicolo-ribaudo · 2021-09-22T19:04:47Z

We (@pzuraq, @JLHwung and me) just had a call to discuss about this PR.

The plugin name should be decoratorsAutoAccessors, to make it clear that it's only meant to be used for the intersection between the "class accessors" and "decorators" proposals.
Chris will try following the ESTree AST shape, but if it ends up being too complex we will fallback to a simple .accessor: boolean on ClassProperty and ClassPrivateProperty. Regardless of what path we choose, when implementing the full class accessors plugin we can change the AST in a minor since it would be opt-in.

packages/babel-parser/src/parser/statement.js

packages/babel-parser/src/types.js

This is the first part of the implementation of the latest version of the decorators proposal: https://github.com/tc39/proposal-decorators The keyword is outlined here in the README: https://github.com/tc39/proposal-decorators#class-auto-accessors It adds a `decoratorAutoAccessors` plugin which can be used to parse the `accessor` keyword into the ClassAccessorProperty node. update plugin name and fix

nicolo-ribaudo

Thanks!

packages/babel-generator/src/generators/classes.ts

JLHwung

Awesome work. The unresolved comments are nits / edge cases.

Co-authored-by: Huáng Jùnliàng <jlhwung@gmail.com>

nicolo-ribaudo · 2021-10-04T15:08:24Z

Thanks! Merged to https://github.com/babel/babel/tree/feat-7.16.0/decorators

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

fedeci reviewed Aug 16, 2021

View reviewed changes

fedeci added pkg: parser Spec: Decorators labels Aug 16, 2021

nicolo-ribaudo added the PR: New Feature 🚀 A type of pull request used for our changelog categories label Aug 16, 2021

nicolo-ribaudo added this to the v7.16.0 milestone Aug 16, 2021

pzuraq force-pushed the add-accessor-to-babel-parser branch from d071aa8 to 95d5837 Compare August 16, 2021 15:24

nicolo-ribaudo reviewed Aug 16, 2021

View reviewed changes

JLHwung reviewed Aug 18, 2021

View reviewed changes

...s/babel-parser/test/fixtures/experimental/decorators-modern/accessor/basic-accessor/input.js Show resolved Hide resolved

pzuraq force-pushed the add-accessor-to-babel-parser branch from 95d5837 to fa5d956 Compare August 21, 2021 17:41

pzuraq changed the title ~~Add accessor contextual modifier to class properties~~ Adds the ClassAccessorProperty and ClassPrivateAccessorProperty node types Aug 21, 2021

pzuraq force-pushed the add-accessor-to-babel-parser branch from fa5d956 to fbe455a Compare August 21, 2021 17:48

pzuraq changed the title ~~Adds the ClassAccessorProperty and ClassPrivateAccessorProperty node types~~ Adds the simpleAutoAccessors parser plugin Aug 21, 2021

fedeci reviewed Aug 25, 2021

View reviewed changes

packages/babel-parser/test/fixtures/experimental/decorators-modern/options.json Outdated Show resolved Hide resolved

nicolo-ribaudo reviewed Sep 1, 2021

View reviewed changes

packages/babel-parser/src/parser/statement.js Outdated Show resolved Hide resolved

pzuraq force-pushed the add-accessor-to-babel-parser branch 2 times, most recently from 5f6f23e to 09dfd6a Compare September 20, 2021 15:48

JLHwung reviewed Sep 21, 2021

View reviewed changes

pzuraq force-pushed the add-accessor-to-babel-parser branch 2 times, most recently from 2038b40 to 35c26c1 Compare September 27, 2021 19:03

nicolo-ribaudo reviewed Sep 27, 2021

View reviewed changes

packages/babel-parser/src/parser/statement.js Outdated Show resolved Hide resolved

packages/babel-parser/src/parser/statement.js Outdated Show resolved Hide resolved

JLHwung reviewed Sep 28, 2021

View reviewed changes

packages/babel-parser/src/types.js Outdated Show resolved Hide resolved

pzuraq force-pushed the add-accessor-to-babel-parser branch from 35c26c1 to 44cb0af Compare September 29, 2021 15:04

pzuraq force-pushed the add-accessor-to-babel-parser branch from 44cb0af to 179e576 Compare September 29, 2021 15:06

nicolo-ribaudo changed the title ~~Adds the simpleAutoAccessors parser plugin~~ Adds the decoratorsAutoAccessors parser plugin Sep 29, 2021

nicolo-ribaudo approved these changes Sep 29, 2021

View reviewed changes

JLHwung reviewed Sep 29, 2021

View reviewed changes

packages/babel-generator/src/generators/classes.ts Show resolved Hide resolved

JLHwung approved these changes Sep 29, 2021

View reviewed changes

Update packages/babel-generator/src/generators/classes.ts

7cdc5a0

Co-authored-by: Huáng Jùnliàng <jlhwung@gmail.com>

nicolo-ribaudo changed the base branch from main to feat-7.16.0/decorators October 4, 2021 15:07

nicolo-ribaudo changed the title ~~Adds the decoratorsAutoAccessors parser plugin~~ Add the decoratorsAutoAccessors parser plugin Oct 4, 2021

nicolo-ribaudo merged commit 0f5663a into babel:feat-7.16.0/decorators Oct 4, 2021

nicolo-ribaudo mentioned this pull request Oct 7, 2021

Implement support for the new decorators proposal #13827

Merged

4 tasks

nicolo-ribaudo removed this from the v7.16.0 milestone Oct 20, 2021

nicolo-ribaudo added a commit that referenced this pull request Nov 24, 2021

Add the decoratorsAutoAccessors parser plugin (#13681)

e6f6d3b

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

nicolo-ribaudo added a commit that referenced this pull request Dec 5, 2021

Add the decoratorsAutoAccessors parser plugin (#13681)

94eb1a6

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

nicolo-ribaudo added a commit that referenced this pull request Dec 16, 2021

Add the decoratorsAutoAccessors parser plugin (#13681)

ee06a98

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

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

Add the decoratorsAutoAccessors parser plugin (#13681)

579ab3f

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

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

Add the decoratorsAutoAccessors parser plugin (#13681)

3f3ce5f

Co-authored-by: Huáng Jùnliàng <jlhwung@gmail.com> Co-authored-by: Nicolò Ribaudo <nicolo.ribaudo@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 Jan 20, 2022

github-actions bot locked as resolved and limited conversation to collaborators Jan 20, 2022

		@@ -0,0 +1,3 @@
		{
		"plugins": [["decorators", { "decoratorsBeforeExport": false, "version": "modern" }]]

Uh oh!

Conversation

pzuraq commented Aug 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

babel-bot commented Aug 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codesandbox-ci bot commented Aug 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fedeci commented Aug 16, 2021

Uh oh!

nicolo-ribaudo commented Aug 16, 2021

Uh oh!

JLHwung commented Aug 16, 2021

Uh oh!

nicolo-ribaudo commented Aug 16, 2021

Uh oh!

nicolo-ribaudo Aug 16, 2021

Choose a reason for hiding this comment

Uh oh!

pzuraq Aug 16, 2021

Choose a reason for hiding this comment

Uh oh!

nicolo-ribaudo commented Aug 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicolo-ribaudo commented Aug 16, 2021

Uh oh!

pzuraq commented Aug 16, 2021

Uh oh!

JLHwung commented Aug 16, 2021

Uh oh!

Uh oh!

pzuraq commented Aug 21, 2021

Uh oh!

Uh oh!

nicolo-ribaudo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fedeci commented Sep 1, 2021

Uh oh!

JLHwung commented Sep 9, 2021

Uh oh!

nicolo-ribaudo commented Sep 12, 2021

Uh oh!

JLHwung commented Sep 12, 2021

Uh oh!

nicolo-ribaudo commented Sep 12, 2021

Uh oh!

pzuraq commented Sep 20, 2021

Uh oh!

JLHwung left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

JLHwung Sep 21, 2021

Choose a reason for hiding this comment

Uh oh!

pzuraq Sep 21, 2021

Choose a reason for hiding this comment

Uh oh!

JLHwung Sep 21, 2021

Choose a reason for hiding this comment

Uh oh!

nicolo-ribaudo commented Sep 22, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nicolo-ribaudo left a comment

pzuraq commented Aug 16, 2021 •

edited

Loading

babel-bot commented Aug 16, 2021 •

edited

Loading

codesandbox-ci bot commented Aug 16, 2021 •

edited

Loading

nicolo-ribaudo commented Aug 16, 2021 •

edited

Loading