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 the decoratorsAutoAccessors
parser plugin
#13681
Add the decoratorsAutoAccessors
parser plugin
#13681
Conversation
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/48994/ |
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:
|
packages/babel-parser/test/fixtures/experimental/accessor/basic-accessor/input.js
Outdated
Show resolved
Hide resolved
|
New feature, it's the first step to implement the new proposal. |
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. |
We don't follow ESTree for class fields anyway, right? |
d071aa8
to
95d5837
Compare
@@ -0,0 +1,3 @@ | |||
{ | |||
"plugins": [["decorators", { "decoratorsBeforeExport": false, "version": "modern" }]] |
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.
Q: Is decoratorsBeforeExport
still an undecided point in the new proposal?
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 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.
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 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:
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 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;
} |
Or maybe it's ok to use |
@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. |
Yes, the class fields unfortunately differ between Babel and ESTree, but we could coordinate early, for example to add 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. |
...s/babel-parser/test/fixtures/experimental/decorators-modern/accessor/basic-accessor/input.js
Outdated
Show resolved
Hide resolved
95d5837
to
fa5d956
Compare
fa5d956
to
fbe455a
Compare
simpleAutoAccessors
parser plugin
Alright, this PR has been updated to add the @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
Outdated
Show resolved
Hide resolved
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.
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 }
CI errors are also related. |
@pzuraq I have open an AST PR for ESTree: estree/estree#255 feedbacks are welcome. |
@JLHwung We diverge from ESTree when representing private fields and methods. I think we should also diverge when representing private accessors, using |
I tend to merge class C { accessor a { get; #set } } Should it be a |
Ok, fair. I was thinking about the |
5f6f23e
to
09dfd6a
Compare
Ok, this PR has been updated with the extra test cases you gave @nicolo-ribaudo, and I've fixed the failing tests. Couple notes:
Other than those issues, I think this PR is good to go! |
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.
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.
optional: true, | ||
}, | ||
typeAnnotation: { | ||
validate: process.env.BABEL_8_BREAKING |
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.
Since it is a new AST type, we can use the Babel 8 behaviour directly.
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.
How do we do that exactly? Not familiar with the breaking behavior, I copied this from the other definitions
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.
Here you can assume process.env.BABEL_8_BREAKING
is always true and reduce the conditional expression to its true branch.
We (@pzuraq, @JLHwung and me) just had a call to discuss about this PR.
|
2038b40
to
35c26c1
Compare
35c26c1
to
44cb0af
Compare
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
44cb0af
to
179e576
Compare
simpleAutoAccessors
parser plugindecoratorsAutoAccessors
parser plugin
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.
Thanks!
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.
Awesome work. The unresolved comments are nits / edge cases.
Co-authored-by: Huáng Jùnliàng <jlhwung@gmail.com>
decoratorsAutoAccessors
parser plugindecoratorsAutoAccessors
parser plugin
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>
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> 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> Co-authored-by: Nicolò Ribaudo <nicolo.ribaudo@gmail.com>
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 theaccessor
keyword into ClassAccessorProperty and ClassPrivateAccessorPropertynode types.