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
Retain pure annotations in class fields #3599
Conversation
Thank you for your contribution! ❤️You can try out this pull request locally by installing Rollup via
or load it into the REPL: |
Codecov Report
@@ Coverage Diff @@
## master #3599 +/- ##
=======================================
Coverage 96.35% 96.35%
=======================================
Files 180 180
Lines 6140 6145 +5
Branches 1799 1801 +2
=======================================
+ Hits 5916 5921 +5
Misses 111 111
Partials 113 113
Continue to review full report at Codecov.
|
src/utils/pureComments.ts
Outdated
@@ -3,6 +3,14 @@ import * as acorn from 'acorn'; | |||
import { base as basicWalker } from 'acorn-walk'; | |||
import { CommentDescription } from '../Module'; | |||
|
|||
// patch up acorn-walk until class-fields are officially supported | |||
basicWalker.FieldDefinition = function (node: any, st: any, c: any) { | |||
c(node.key, st, 'Expression'); |
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.
Shouldn't this line to be if ( node.computed ) { c(node.key, st, 'Expression'); }
, just like MethodDefinition
/ Property
do in acorn-work
?
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.
Good catch, was obviously copying without thinking.
@@ -0,0 +1,6 @@ | |||
class main { | |||
static k = /*#__PURE__*/ V(); | |||
static /*#__PURE__*/ ignored; |
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.
Does this sample line has special meaning in rollup? It seems not a pure mark for function call.
Is it the new feature of rollup?
Or just as a test, to make sure here is no bug, without any other use?
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.
The latter, a test to cover a branch in the logic even if it makes no sense to do so in real code.
32e1295
to
49377f9
Compare
This PR contains:
Are tests included?
Breaking Changes?
List any relevant issue numbers:
Resolves #3591
Description
Unfortunately,
acorn-walk
is not yet properly updated for the new nodes added for class fields. This adds a temporary patch until this is hopefully resolved once the feature is merged to acorn.