Rollup class fields support

[guybedford]

This adds class fields and static class fields support.

The reason being that it is almost impossible to add via the inject mechanism due to the different Acorn instances in play giving this error - https://github.com/acornjs/acorn-private-class-elements/blob/master/index.js#L23.

In addition both plugins needed to be forked to add the Acorn 7 support which is missing (with other similar PRs outstanding for some time).

For me personally this was to get the feature working, but I suspect there may be components of the treeshaking that will need attention. In addition the tests have not been fleshed out, hence the WIP status. But this is functional in my own test cases.

Maybe RollupJS can distribute some of its funds to help Acorn plugin development. @adrianheine would financial support help here in moving things forward?

This PR contains:

• bugfix
• feature
• refactor
• documentation
• other

Are tests included?

• yes (bugfixes and features will not be merged without tests)
• no

Breaking Changes?

• yes (breaking changes will not be merged unless absolutely necessary)
• no

List any relevant issue numbers:

Description

[guybedford]

These should be devDependencies of course.

[rollup-bot]

Thank you for your contribution! ❤️

You can try out this pull request locally by installing Rollup via

npm install rollup/rollup#rollup-class-fields

or load it into the REPL:
https://rollupjs.org/repl/?circleci=10460

[codecov]

Codecov Report

Merging #3488 into master will increase coverage by 0.10%.
The diff coverage is 87.50%.

@@            Coverage Diff             @@
##           master    #3488      +/-   ##
==========================================
+ Coverage   95.96%   96.06%   +0.10%     
==========================================
  Files         174      175       +1     
  Lines        5947     5950       +3     
  Branches     1752     1752              
==========================================
+ Hits         5707     5716       +9     
+ Misses        123      118       -5     
+ Partials      117      116       -1     

Impacted Files	Coverage Δ
src/ast/nodes/CallExpression.ts	95.41% <ø> (ø)
src/ast/nodes/MemberExpression.ts	98.27% <ø> (ø)
src/ast/nodes/NodeType.ts	100.00% <ø> (ø)
src/ast/nodes/index.ts	100.00% <ø> (ø)
src/ast/nodes/shared/Node.ts	96.96% <ø> (+5.54%)	⬆️
cli/run/watch.ts	86.11% <33.33%> (+1.17%)	⬆️
src/Graph.ts	98.67% <100.00%> (+0.66%)	⬆️
src/Module.ts	98.86% <100.00%> (ø)
src/ast/nodes/ClassBody.ts	100.00% <100.00%> (ø)
src/ast/nodes/Identifier.ts	100.00% <100.00%> (ø)
... and 4 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7abfb93...86843d1. Read the comment docs.

[lukastaegert]

I fixed the dependencies and also added a simple test. Was also able to solve an old issue related to this now that we can test this: #2775. From my side this is ready to be merged.

I just hope they can agree on an AST structure for optional chaining and nullish coalescing soon so that we can complete ES2020 support soon...

[lukastaegert]

On second thought, maybe I want to at least add the new AST node types for class properties...

[lukastaegert]

Ok, Nodes are added.

[guybedford]

I do not pretend to understand this magic, but it looks great! I suppose we can remove the WIP status of this PR then?

Should we test static expressions and static functions as well here?

[guybedford]

*private static methods I mean

[lukastaegert]

I suppose we can remove the WIP status of this PR then?

Definitely

Should we test static expressions and static functions as well here?

Please feel free to add this, I must admit I was not entirely sure what was included. I will not be able to look at this again before tonight.

[lukastaegert]

@guybedford I extended the test to include all new features and, who would have thought, support is incomplete: Private methods, setters and getters produce an "unexpected token" while parsing. I assume this needs to be fixed in the plugin. Should we still release it as "incomplete support"? It still provides some value and also fixes a bug.

[lukastaegert]

You can remove the comments in my test case to see what I mean.

[guybedford]

@lukastaegert thankfully, for now at least, I believe this syntax support level exactly matches Chrome stable (and hence the major usage). But yes as soon as Chrome lands those that will be an issue. Perhaps maintaining and developing the forks as part of Rollup will be an option here.

Note that also eg Terser doesn't support any of this yet (I know @fabiosantoscode was originally against it)... perhaps this is another area where RollupJS could assist with funding for these features.

[lukastaegert]

You are right, Chrome does not support any of the missing ones. It also does not support static private methods, so there is at least one additional case that is supported. Then I will push this one out as well.

[bathos]

Private methods and accessors, both static and instance, are implemented in V8 and are unflagged now in Chrome 84 as well. Will Rollup be supporting these as well, and if so, should an issue be opened to track that?

[bathos]

I doubt it’s really this simple, but FWIW, in this code —

    // Parse private static methods
    parsePropertyName(prop) {
      if (prop.static && this.type == this.privateNameToken) {
        this.parsePrivateClassElementName(prop);
      } else {
        super.parsePropertyName(prop);
      }
    }

If I remove prop.static from the condition, unexpected token errors from instance private methods stop and the output looks correct.

[lukastaegert]

You can create an issue, but this is not Rollup code, without an upstream issue in the corresponding acorn plugin (likely https://github.com/acornjs/acorn-private-class-elements), nothing will move forward here.

[bathos]

Cool — in that case, it seems like maybe Rollup support (at least for instance methods) might end up being “free” if the upstream plugin is updated to handle these cases. Will investigate a bit further, thanks.

[lukastaegert]

Yes, that is my expectation. Others will profit from this as well.

[fabiosantoscode]

Hey there!

I was against class fields, but I wouldn't stop progress for just that :) they are implemented in Terser. I don't recall if I got to implementing private fields too, though.