Change to uglify es & output uglify errors #157
Conversation
|
We should also update the documentation here: https://github.com/parcel-bundler/website/blob/3c54bf479e2b1d244b30d35b15b4bab2e18d7106/src/docs/production.md Change |
src/transforms/uglify.js
Outdated
| const {toEstree} = require('babel-to-estree'); | ||
| const types = require('babel-types'); | ||
| const walk = require('babylon-walk'); | ||
|
|
||
| module.exports = async function(asset) { | ||
| await asset.parseIfNeeded(); | ||
|
|
||
| // Convert to UglifyJS AST | ||
| // Convert to UglifyES AST | ||
| var ast = AST_Node.from_mozilla_ast(toEstree(asset.ast, asset.contents)); |
There was a problem hiding this comment.
This won't work for ES6+ code.
You either have to use the built-in uglify-es parser instead of ESTree or add ES6+ ESTree support to uglify-es.
It's an known issue that only ES5 ESTree constructs are currently converted by uglify-es: mishoo/UglifyJS#968
There was a problem hiding this comment.
Ow damn, thanks for the heads up, i'll read through that issue for sure
src/transforms/uglify.js
Outdated
| @@ -5,7 +5,7 @@ const walk = require('babylon-walk'); | |||
| module.exports = async function(asset) { | |||
| await asset.parseIfNeeded(); | |||
|
|
|||
| // Convert to UglifyES AST | |||
| // Minify with UglifyES | |||
There was a problem hiding this comment.
Suggest to change to "Minify with uglify-es" to match the npm package name. The term "UglifyES" is not used by its maintainers.
There was a problem hiding this comment.
Will do, tnx for the suggestion 💯
There was a problem hiding this comment.
...or better yet - drop the comment altogether.
|
I'm not surprised that the timings are not impacted very much for ES6. Converting an AST is an expensive operation. Regarding the timings for Typescript - is it faster now because an AST no longer has to be generated? |
|
|
|
With this PR is the babel AST still needed? parcel/src/transforms/babel.js Lines 23 to 25 in 548eef9 |
|
I'm pretty sure it is, it's being used in different places |
|
I'm kind of puzzled by 9d59def. |
|
I just copied that piece of code from JSAsset.generate() to uglify, so that it would only generate code from the AST @kzc |
|
Okay, I see. Uglify is only fed ES source code, not ESTree. |
| // Log all warnings | ||
| if (result.warnings) { | ||
| result.warnings.forEach(warning => { | ||
| // TODO: warn this using the logger |
There was a problem hiding this comment.
Would probably be better to use the logger to send warnings to the console, not really sure how to address the logger from here though
There was a problem hiding this comment.
hmm yeah, the logger isn't really accessible from here, it lives only in the main process right now, not workers.
src/transforms/uglify.js
Outdated
| // Convert AST into JS | ||
| let code = asset.isAstDirty | ||
| ? generate(asset.ast).code | ||
| : asset.outputCode || asset.contents; |
There was a problem hiding this comment.
maybe just call asset.generate().js here?
There was a problem hiding this comment.
I extracted this bit from generate because i saw TODO: Source maps and thought this would affect performance in the future. I'll change it back to asset.generate though :)
| // Log all warnings | ||
| if (result.warnings) { | ||
| result.warnings.forEach(warning => { | ||
| // TODO: warn this using the logger |
There was a problem hiding this comment.
hmm yeah, the logger isn't really accessible from here, it lives only in the main process right now, not workers.
* uglify-es * fix error handling * dump estree, enables ES6+ support * fix comment * change comment to match npm package name * remove useless comment * remove unused imports, change asset.generate to babel generate * improve minify options * fix comment * remove unused import * pivot back to asset.generate().js
* uglify-es * fix error handling * dump estree, enables ES6+ support * fix comment * change comment to match npm package name * remove useless comment * remove unused imports, change asset.generate to babel generate * improve minify options * fix comment * remove unused import * pivot back to asset.generate().js
If uglify encountered errors it didn't get thrown, due to a change in uglify v3.X.
I'm aware the current uglify bug people reported in #8 is due to this bug: lightscript/babel-to-estree#1EDIT: This will now also fix #8 due to no longer using estree [FIXED]
I think parcel should be using Uglify ES to support more modern babel configs & js versions to prevent future issues
Performance tests (Based on running the tests 3 times and taking the avg - On Macbook Pro dual core mid-2014)
Related to
closes #145 closes #124 closes #15 closes #8