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

Add option to deoptimize var declarations for tree-shaking #4139

Merged
merged 10 commits into from Jun 16, 2021

Conversation

@lukastaegert
Copy link
Member

@lukastaegert lukastaegert commented Jun 14, 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 add a new option treeshake.correctVarValueBeforeDeclaration. This option will deoptimize the value of any var declarations to make sure that Rollup does not make wrong assumptions when variables are accessed before their declaration. This will even work in pathological examples like

checkFoo();
var foo = true;

function checkFoo() {
  if (foo) console.log('should not be shown');
}

without this option, this will be simplified to

checkFoo();

function checkFoo() {
  console.log('should not be shown');
}

which is obviously wrong. As this option can lead to heavy deoptimizations depending on the code, it is not switched on by default. It is still better than turning of tree-shaking altogether as basic tree-shaking will still work well: If a variable is not used, it will be removed.

In the meantime, we can work on finding a proper fix for this situation, at which point this option will be deprecated again.

Currently, it will be part of the "safest" tree-shaking preset introduced in #4131.

@github-actions
Copy link

@github-actions github-actions bot commented Jun 14, 2021

Thank you for your contribution! ❤️

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

npm install rollup/rollup#correct-var-value-before-declaration

or load it into the REPL:
https://rollupjs.org/repl/?pr=4139

@codecov
Copy link

@codecov codecov bot commented Jun 14, 2021

Codecov Report

Merging #4139 (97a6cd2) into master (86e8510) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #4139   +/-   ##
=======================================
  Coverage   98.13%   98.13%           
=======================================
  Files         201      201           
  Lines        7114     7116    +2     
  Branches     2081     2083    +2     
=======================================
+ Hits         6981     6983    +2     
  Misses         64       64           
  Partials       69       69           
Impacted Files Coverage Δ
src/utils/options/options.ts 100.00% <ø> (ø)
src/ast/nodes/Identifier.ts 100.00% <100.00%> (ø)
src/utils/options/normalizeInputOptions.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 86e8510...97a6cd2. Read the comment docs.

@lukastaegert lukastaegert force-pushed the treeshake-presets branch 2 times, most recently from ded6187 to d8ad9ac Jun 16, 2021
Base automatically changed from treeshake-presets to master Jun 16, 2021
@lukastaegert lukastaegert force-pushed the correct-var-value-before-declaration branch from 9d68c3c to 97a6cd2 Jun 16, 2021
@lukastaegert lukastaegert merged commit 40c1c6c into master Jun 16, 2021
10 checks passed
@lukastaegert lukastaegert deleted the correct-var-value-before-declaration branch Jun 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

1 participant