Tree-shake parameter defaults (replaces #4498)#4510
Merged
lukastaegert merged 12 commits intomasterfrom May 27, 2022
Merged
Conversation
Also improve side effect detection for unused parameter defaults
Thank you for your contribution! ❤️You can try out this pull request locally by installing Rollup via npm install rollup/rollup#parameter-defaultsor load it into the REPL: |
Codecov Report
@@ Coverage Diff @@
## master #4510 +/- ##
==========================================
+ Coverage 98.74% 98.78% +0.03%
==========================================
Files 207 208 +1
Lines 7357 7425 +68
Branches 2084 2118 +34
==========================================
+ Hits 7265 7335 +70
- Misses 33 34 +1
+ Partials 59 56 -3
Continue to review full report at Codecov.
|
This was referenced May 25, 2022
This was referenced May 29, 2022
Closed
7 tasks
Member
Author
|
To everyone following this, I decided to roll back the feature for good as it turned out to be far too brittle with far too much overhead for my liking at the moment, see #4520 |
pos777
pushed a commit
to pos777/rollup
that referenced
this pull request
Jun 18, 2022
* Recreate parameter tree-shaking but keep boolean return values * Properly include parameters when tree-shaking is disabled Also improve side effect detection for unused parameter defaults * Make null checks more compact * Include super class parameter defaults * Include return value parameter defaults * Limit feature to top-level functions * Remove path from deoptimization signature * Ensure class methods are deoptimized * Add comment * Class fields need Node 12 * Improve coverage * Improve coverage
9 tasks
This file contains hidden or 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
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:
Resolves #4498
Resolves #4503
Resolves #4504
Resolves #4505
Resolves #4506
Resolves #4507
Description
This replaces the previous PR #4498 which has been rolled back. In the previous implementation, I had a hard time finding a way to make the feature "non-optimized by default" which meant an unprecedented amount of different bugs crept in. As it was feeling increasingly like playing whack-a-mole, I decided to revisit the general assumptions and greatly reduced the scope of the feature.
Now, parameter default tree-shaking will only be applied to top-level functions and IIFEs as in those cases, we can be fairly certain we can either track every possible call of a function or know when it is not possible.
In the previous implementation, tree-shaking was also possible for objects and static class methods, but I found there were sneaky bugs because here it is possible to call a function via
this, or worse, viasuper, and those entirely depend on call context.Still, all examples of the previous PR still work, which is why I will copy them here:
This allows tree-shaking for unnecessary default parameters in functions. This includes arrow functions but does not include nested defaults in destructuring patterns REPL
While the feature may sound simple at first glance, it was actually quite complex. The main problem is that JavaScript does not make a difference between not passing a parameter to a function and passing the value
undefined: In both cases, the default value will be used.Therefore, there are quite some subtleties involved which in the end I resolved by extending the mechanism I created for omitting unneeded parameters. This mechanism was removing arguments from function calls if the corresponding parameter was unused. Now, it will also see if parameter defaults need to be included. Care was taken to ensure no side effects are omitted REPL
It is very important to track the instance where we "lose track" of how a function is used, e.g. because it is exported, passed to an unknown function etc. In those cases, all necessary defaults are included REPL