implement --treeshake.propertyReadSideEffects=always to handle getters with side effects #3985
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## master #3985 +/- ##
=======================================
Coverage 97.21% 97.21%
=======================================
Files 191 191
Lines 6709 6712 +3
Branches 1962 1963 +1
=======================================
+ Hits 6522 6525 +3
Misses 99 99
Partials 88 88
Continue to review full report at Codecov.
|
lukastaegert
approved these changes
Mar 7, 2021
docs/999-big-list-of-options.md
Outdated
| CLI: `--treeshake.propertyReadSideEffects`/`--no-treeshake.propertyReadSideEffects`<br> | ||
| Default: `true` | ||
|
|
||
| If `true`, only retain property reads that are determined to be needed. Note: this setting assumes that all getters are side effect free. |
There was a problem hiding this comment.
This is somewhat confusing and also not true, we do not assume all getters are side-effect free. true means retain property reads that Rollup can resolve to have side-effects.
Suggested change
| If `true`, only retain property reads that are determined to be needed. Note: this setting assumes that all getters are side effect free. | |
| If `true`, retain unused property reads that Rollup can determine to have side-effects. This includes accessing properties of `null` or `undefined` or triggering explicit getters via property access. Note that this does not cover destructuring assignment or getters on objects passed as function parameters. |
There was a problem hiding this comment.
The wording was changed as requested. I struggled to describe this option because of the known getter issues.
lukastaegert
approved these changes
Mar 9, 2021
This was referenced Mar 9, 2021
Closed
Closed
This was referenced Mar 18, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR contains:
Are tests included?
Breaking Changes?
List any relevant issue numbers:
#2219
#3234
#3974
#3984
Description
Rollup does not correctly handle code dealing with getters with side effects when
treeshakeis enabled (the default setting). To address this shortcoming, this PR adds an'always'setting totreeshake.propertyReadSideEffectsfirst proposed in #3234 (comment).