-
Notifications
You must be signed in to change notification settings - Fork 14k
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
fix: reenabling terser plugin for webpack #17205
Conversation
Codecov Report
@@ Coverage Diff @@
## master #17205 +/- ##
==========================================
- Coverage 76.95% 76.95% -0.01%
==========================================
Files 1038 1038
Lines 55597 55597
Branches 7585 7585
==========================================
- Hits 42787 42785 -2
- Misses 12560 12562 +2
Partials 250 250
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what an amazing one liner. Thanks for fixing!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking care of that!
@rusackas could you add a before vs after bundle size comparison? |
@graceguo-supercat |
I was trying to run it with
Wondering if it's just me. It must not be webpack's fault if folks were able to run it post-upgrade. Anyone else able to run it on this branch? |
superset-frontend/webpack.config.js
Outdated
minimizer: [new CssMinimizerPlugin()], | ||
minimizer: ['...', new CssMinimizerPlugin()], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might not make a difference, but the example in the docs places the ellipsis after the plugin, in which case this would be:
minimizer: [new CssMinimizerPlugin(), '...'],
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. Made the change, though it made no discernible difference. ¯\_(ツ)_/¯
7073258
to
cf9ce04
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
SUMMARY
In the Webpack 5 upgrade, we inadvertently lost the inclusion of the terser plugin due to the way the CssMinimizerPlugin was loaded. This PR uses webpack's
...
syntax to extend existing minimizers and ADD the CssMinimizerPlugin on top of the default terser plugin, rather than it being the ONLY minimizer.This should shrink the JS bundle/chunk sizes by about 17MB in total.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
Poke all over the place in an ephemeral build and make sure nothing randomly exploded, but I believe this is low risk.
ADDITIONAL INFORMATION