Use terser es6+ compatible for JS minification #26531
Merged
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
sirreal
added
[Type] Question
[Status] Needs Review
sirreal
added
[Status] Needs Review
sirreal
commented
Aug 7, 2018
| ecma: 5, | ||
| safari10: true, |
There was a problem hiding this comment.
According to docs, this is mangle: { safari10: true }, output: { safari10: true }. Do we prefer just the mangle safari option?
There was a problem hiding this comment.
we should be setting both. I don't think the output option was available when we added the mangle setting, but maybe we just missed it.
| * Enable when the following is resolved: | ||
| * https://github.com/mishoo/UglifyJS2/issues/3010 | ||
| */ | ||
| collapse_vars: false, |
There was a problem hiding this comment.
Should be fixed in terser.
|
You can verify that the bug which caused us to disable
See #23388 for the PR which originally disabled |
sirreal
added
[Status] Needs e2e Testing
[Status] Needs Jetpack e2e Testing
|
ICFY report is in 😁 Bytes are saved. |
|
Just to highlight: |
sirreal
commented
Aug 7, 2018
| mangle: { | ||
| safari10: true, | ||
| }, | ||
| terserOptions: { | ||
| ecma: 5, |
There was a problem hiding this comment.
ecma options are available for parse, compress and output. Would it make sense to keep all of these at the highest ecma version available, and let babel deal with output?
https://github.com/fabiosantoscode/terser#minify-options
There was a problem hiding this comment.
Probably? Though if we misconfigure babel, uglify currently acts as a really bad canary...
But bumping it up would allow us to try to transpile to more modern targets.
blowery
approved these changes
Aug 10, 2018
sirreal
force-pushed
the
update/use-es6compat-terser-minimizer
branch
from
771df26 to
60baf77
Compare
sirreal
added
[Type] Enhancement
|
Rebased with fresh |
blowery
added
the
[Status] Needs Review
sirreal
added
[Status] Needs Jetpack e2e Testing
hoverduck
added
[Status] Needs e2e Testing
[Status] Needs Jetpack e2e Testing
sirreal
added
[Status] Needs Jetpack e2e Testing
|
Jetpack e2e tests are reporting failures. @brbrr will have a look tomorrow to see if failures are related. If not, we'll merge then. Thanks everyone! |
|
I'd say we good to go. Jetpack PR tests are something new, and may not behave :) Failures aren't related to this PR. |
|
Thanks for checking @brbrr! |
sirreal
added
[Status] Ready to Merge
and removed
[Status] Needs Jetpack e2e Testing
jeherve
added a commit
to Automattic/jetpack
that referenced
this pull request
Jan 4, 2019
uglify-es is not maintained anymore: mishoo/UglifyJS#3156 (comment) Terser seems to be the replacement of choice, and is also what we use in Calypso: Automattic/wp-calypso#26531 This commit introduces gulp-minify, which uses terser behind the scenes: https://www.npmjs.com/package/gulp-minify
jeherve
pushed a commit
to Automattic/jetpack
that referenced
this pull request
Jan 4, 2019
…11079) * New npm module for allowing gulp-uglify to process es6 * remove debug code * Move away from uglify-es. Use terser instead. uglify-es is not maintained anymore: mishoo/UglifyJS#3156 (comment) Terser seems to be the replacement of choice, and is also what we use in Calypso: Automattic/wp-calypso#26531 This commit introduces gulp-minify, which uses terser behind the scenes: https://www.npmjs.com/package/gulp-minify * Update yarn.lock
jeherve
pushed a commit
to Automattic/jetpack
that referenced
this pull request
Jan 4, 2019
…11079) * New npm module for allowing gulp-uglify to process es6 * remove debug code * Move away from uglify-es. Use terser instead. uglify-es is not maintained anymore: mishoo/UglifyJS#3156 (comment) Terser seems to be the replacement of choice, and is also what we use in Calypso: Automattic/wp-calypso#26531 This commit introduces gulp-minify, which uses terser behind the scenes: https://www.npmjs.com/package/gulp-minify * Update yarn.lock
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.
Use https://github.com/webpack-contrib/terser-webpack-plugin for JS minification
This appears to be the direction webpack-contrib is taking with regards to the unmaintained es6+ uglify-es minimizer.
See:
This PR is mostly to raise awareness for now. We might like to wait to see how community-wide adoption goes before making the change:
https://www.npmjs.com/package/terser-webpack-plugin