Skip to content
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

Tree-shake unused declarations while keeping initializer side-effects #3933

Merged
merged 2 commits into from Jan 22, 2021

Conversation

lukastaegert
Copy link
Member

@lukastaegert lukastaegert commented Jan 22, 2021

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

This will allow partial tree-shaking of declarations where the variable is unused but the right-hand-side has side-effects. This will be important for upcoming improvements for the commonjs plugin. Example:

// main.js
let x = globalFunction();
x = 'this is some reassignment that does not matter but may be retained because x is retained';

// previous output
let x = globalFunction();
x = 'this is some reassignment that does not matter but may be retained because x is retained';

// new output
globalFunction();

@rollup-bot
Copy link
Collaborator

rollup-bot commented Jan 22, 2021

Thank you for your contribution! ❤️

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

npm install rollup/rollup#unused-side-effect-declarations

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

@codecov
Copy link

codecov bot commented Jan 22, 2021

Codecov Report

Merging #3933 (2bc3bc5) into master (e23bb35) will increase coverage by 0.05%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3933      +/-   ##
==========================================
+ Coverage   97.13%   97.18%   +0.05%     
==========================================
  Files         188      188              
  Lines        6669     6681      +12     
  Branches     1940     1944       +4     
==========================================
+ Hits         6478     6493      +15     
+ Misses        101       99       -2     
+ Partials       90       89       -1     
Impacted Files Coverage Δ
src/ast/nodes/AssignmentExpression.ts 100.00% <ø> (ø)
src/ast/nodes/MemberExpression.ts 98.31% <ø> (ø)
src/ast/nodes/ForInStatement.ts 100.00% <100.00%> (ø)
src/ast/nodes/ForOfStatement.ts 100.00% <100.00%> (ø)
src/ast/nodes/Property.ts 92.30% <100.00%> (ø)
src/ast/nodes/VariableDeclaration.ts 97.95% <100.00%> (+0.95%) ⬆️
src/ast/nodes/VariableDeclarator.ts 100.00% <100.00%> (ø)
src/ast/nodes/shared/Node.ts 100.00% <100.00%> (+2.98%) ⬆️

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 e23bb35...2bc3bc5. Read the comment docs.

@lukastaegert lukastaegert force-pushed the unused-side-effect-declarations branch from 6b816c4 to 2bc3bc5 Compare January 22, 2021 16:03
@lukastaegert lukastaegert merged commit e960ca8 into master Jan 22, 2021
@lukastaegert lukastaegert deleted the unused-side-effect-declarations branch January 22, 2021 16:20
@thorn0
Copy link

thorn0 commented Feb 1, 2021

@lukastaegert Please have a look at this comment
prettier/prettier#10199 (comment)
Is my guess there correct?

@lukastaegert
Copy link
Member Author

I fear it is. Damn JavaScript!

@lukastaegert
Copy link
Member Author

Ok, fix at #3947, will release this once CI is done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants