Move pure comment annotation to Graph.contextParse #3981

yannayl · 2021-03-02T15:37:42Z

If a plugin calls PluginContext.parse method to parse the AST of the module
rollup ignored the PURE annotations. This commit fixes the issue by moving
the annotation functionality to a central location - the contextParse method.

Resolves #3979

This PR contains:

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:

Resolve #3979

Description

Move pure comment annotation to Graph.contextParse

If a plugin calls PluginContext.parse method to parse the AST of the module
rollup ignored the PURE annotations. This commit fixes the issue by moving
the annotation functionality to a central location - the contextParse method.

If a plugin calls `PluginContext.parse` method to parse the AST of the module rollup ignored the `PURE` annotations. This commit fixes the issue by moving the annotation functionality to a central location - the `contextParse` method. Resolves rollup#3979

codecov · 2021-03-02T15:48:33Z

Codecov Report

Merging #3981 (d4e3db5) into master (a12ec5b) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #3981   +/-   ##
=======================================
  Coverage   97.21%   97.22%           
=======================================
  Files         191      191           
  Lines        6709     6729   +20     
  Branches     1962     1969    +7     
=======================================
+ Hits         6522     6542   +20     
  Misses         99       99           
  Partials       88       88

Impacted Files	Coverage Δ
src/utils/PluginContext.ts	`100.00% <ø> (ø)`
src/Graph.ts	`100.00% <100.00%> (ø)`
src/Module.ts	`100.00% <100.00%> (ø)`
src/ast/keys.ts	`100.00% <100.00%> (ø)`
src/ast/nodes/CallExpression.ts	`95.45% <100.00%> (+0.04%)`	⬆️
src/ast/nodes/NewExpression.ts	`100.00% <100.00%> (ø)`
src/ast/nodes/shared/Node.ts	`100.00% <100.00%> (ø)`
src/utils/pureComments.ts	`100.00% <100.00%> (ø)`
src/utils/treeshakeNode.ts	`100.00% <100.00%> (ø)`

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 a12ec5b...d4e3db5. Read the comment docs.

yannayl · 2021-03-02T16:02:06Z

Added tests

Use the `onComment` feautre. This expands the coverage.

lukastaegert · 2021-03-05T16:34:33Z

Sorry for the delay, things came up. Will look at this soon.

lukastaegert

Thanks a lot! This goes very much in the direction I would have thought. Regarding your comments on the other ticket, though:

The description of "this.parse" states that it parses code "like Rollup" (and also adds exactly the same plugins), so I like it very much that the users do not need an extra step to add annotations (if they want a "pure" parser, they can always just import their own copy of acorn or whatever other parser). You could see the added annotations as a special "Rollup plugin".
Annotation properties are non-standard and undocumented—on purpose. Because only that way, we have the freedom to change implementation details. But maybe we should use this chance to change property names. My suggestion would be to prefix the properties with _rollup, (i.e. _rollupPure), to make clear:
- these are internal implementation details (_, official ESTree properties will never have a preceding underscore I presume)
- avoid conflicts with other potential tool specific fields
  Would you do this in this ticket?

lukastaegert · 2021-03-07T06:26:25Z

src/Graph.ts

+		options.onComment = onCommentOrig;
+
+		markPureCallExpressions(comments.map((cmt) => {
+			return {...cmt, block: cmt.type == "Block", text: cmt.value};


This mapping strikes me as unnecessary if you either push the comments into the array in the previous way or change markPureCallExpressions to work with a slightly changed format?

I agree. I will change it.

In general I try to keep my changes minimal because it's my first contribution and I don't want to accidentally break anything...

This change eliminates needless mapping between the types.

yannayl · 2021-03-07T14:01:03Z

* The description of "this.parse" states that it parses code "like Rollup" (and also adds exactly the same plugins), so I like it very much that the users do not need an extra step to add annotations (if they want a "pure" parser, they can always just import their own copy of acorn or whatever other parser). You could see the added annotations as a special "Rollup plugin".

I am afraid I wasn't clear. I agree that annotations should be added automatically. I just thought it could be convenient if plugins can have a clear API to add more annotations on top of the automatic.

* Annotation properties are non-standard and undocumented—on purpose. Because only that way, we have the freedom to change implementation details.

I understand. This is why I suggested a function for this purpose - to keep it separated from the implementation and not touching the nodes directly. Assume a user wishes to skip some nodes in the tree-shaking process, currently there is no way to do it.

But maybe we should use this chance to change property names. My suggestion would be to prefix the properties with _rollup, (i.e. _rollupPure), to make clear:
  * these are internal implementation details (`_`, official ESTree properties will never have a preceding underscore I presume)
  * avoid conflicts with other potential tool specific fields
    Would you do this in this ticket?

Yes, no problem.
I will change it from annotations to _rollupAnnotations in the acorn.Node and ~~rollupAnnotations~~annotations in src/ast/nodes/shared/Node.ts

When parsing the JS code we add attributes to acorn.Node for later use: * annotations: acorn.Commnet - the comment that defined the annotation * annotatedPure: boolean - declares this node as pure call/new expression This commit puts all these attributes under one name: `_rollupAnnotations` with a strong type. Thus we clarify that these attributes are for internal use only and decrease the change of ever coliding with internal/future attributes of `acorn.Node`.

lukastaegert

Looks good, thanks a lot

Yannay Livneh added 2 commits March 2, 2021 17:29

Remove unused import

1b78532

Add test call-marked-pure-with-plugin-parse-ast

1c599c8

Improv test call-marked-pure-with-plugin-parse-ast

2ba2a46

Use the `onComment` feautre. This expands the coverage.

lukastaegert reviewed Mar 7, 2021

View reviewed changes

Change type CommentDescription -> acorn.Comment

43edcca

This change eliminates needless mapping between the types.

yannayl mentioned this pull request Mar 7, 2021

Change removal of sourcemap comment #3987

Merged

9 tasks

Yannay Livneh added 3 commits March 7, 2021 20:21

Remove '_rollupAnnotations' key from Node.keys

dc93f2e

Remove dead code

d4e3db5

lukastaegert approved these changes Mar 9, 2021

View reviewed changes

lukastaegert merged commit 992a285 into rollup:master Mar 9, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move pure comment annotation to Graph.contextParse #3981

Move pure comment annotation to Graph.contextParse #3981

yannayl commented Mar 2, 2021 •

edited

codecov bot commented Mar 2, 2021 •

edited

yannayl commented Mar 2, 2021

lukastaegert commented Mar 5, 2021

lukastaegert left a comment •

edited

lukastaegert Mar 7, 2021

yannayl Mar 7, 2021

yannayl commented Mar 7, 2021 •

edited

lukastaegert left a comment

Move pure comment annotation to Graph.contextParse #3981

Move pure comment annotation to Graph.contextParse #3981

Conversation

yannayl commented Mar 2, 2021 • edited

Description

codecov bot commented Mar 2, 2021 • edited

Codecov Report

yannayl commented Mar 2, 2021

lukastaegert commented Mar 5, 2021

lukastaegert left a comment • edited

Choose a reason for hiding this comment

lukastaegert Mar 7, 2021

Choose a reason for hiding this comment

yannayl Mar 7, 2021

Choose a reason for hiding this comment

yannayl commented Mar 7, 2021 • edited

lukastaegert left a comment

Choose a reason for hiding this comment

yannayl commented Mar 2, 2021 •

edited

codecov bot commented Mar 2, 2021 •

edited

lukastaegert left a comment •

edited

yannayl commented Mar 7, 2021 •

edited