-
-
Notifications
You must be signed in to change notification settings - Fork 5.7k
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
Implement class features in estree #12370
Implement class features in estree #12370
Conversation
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/41540/ |
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 9d36a74:
|
4a293e3
to
e4ae011
Compare
0c98726
to
714e834
Compare
8e4fb27
to
abd06d9
Compare
abd06d9
to
227e1d1
Compare
227e1d1
to
ba7afc8
Compare
Do we know how we want to handle a situation like this? Are we going to wait until major versions to change the AST or add options or something? Just want to make sure we don't break any plugins that rely on the current AST shape. |
So far we have always considered these changes as "new features" and shipped them in minors enabled by default: the contract of the "estree" option is "I want Babel to generate an AST that respects the ESTree spec", so if the ESTree spec changes not aligning with it is a bug (similarly, when the ECMAScript spec changes we introduce the breaking change to align as a bugfix). However, I wouldn't be opposed to something like |
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.
Code looks good to me - just a few questions for clarity!
The class features ESTree proposal has been moved to stage 3 recently: estree/estree#242, which means unless the spec undergoes significant change, the current ESTree AST shape will not change. See also https://github.com/estree/estree/blob/master/stage3/README.md |
Q: Does eslint support this natively now? |
I haven't been involved in a few months, but, last I checked, ESLint's policy is to not crash on stage 3 syntax. Rules aren't updated for new syntax until they hit stage 4. |
No, sadly they're still insisting on waiting til stage 4 to support things that have already shipped in browsers and node. |
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 have a few small suggestions for improving the clarity of comments, but the code LGTM!
class A { | ||
#foo = "bar"; | ||
static isA(obj) { | ||
return #foo in obj; |
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 the estree private brand checks proposal is also moved to stage 3. I just add a test case for private-in.
This PR implements https://github.com/estree/estree/blob/master/experimental/class-features.md given that class features are stage 3 and shipped in several engines (V8, JSC, Gecko).
Note that ESLint does not support class features yet because Acorn has not yet implemented this proposal, though it supports a variant AST shape via
acorn-class-features
.Eventually Acorn will support the AST shape supported in this PR since once class features are merged to ESTree. Hence we went ahead and implement this proposal in the
estree
plugin.Support of ESTree class features is still experimental, Babel may change the class features AST if, when the ESTree proposal merged, the AST shapes differs, though it is very unlikely as far as I know.
This PR includes commits in #12375.