Skip to content
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

[Feature Request] ES6 support for the uglifier #6041

Closed
ThorstenBux opened this issue Jan 9, 2018 · 18 comments
Closed

[Feature Request] ES6 support for the uglifier #6041

ThorstenBux opened this issue Jan 9, 2018 · 18 comments

Comments

@ThorstenBux
Copy link

To me it appears that the uglifier-js used in version 1.37.22 does not support ES6 syntax. It refuses to work with arrow-functions (=>), it refuses to work with default values for function parameters ( var x = function(x=1) ), it refuses to work with the new variable declarations ( let ), it refuses to work with template strings (backticks '`' eg: var s = `test ${i}` )

@kripken
Copy link
Member

kripken commented Jan 9, 2018

Correct, this is a current limitation. Some discussion about one possible way to solve this is in #6000, but currently as far as I know no one is working on it. It would be great if someone could.

@ThorstenBux
Copy link
Author

Is it possible to change the uglifier? using this https://github.com/mishoo/UglifyJS2/tree/harmony for example?

@kripken
Copy link
Member

kripken commented Jan 9, 2018

Sadly no, it's not that easy. We use an older version, Uglify1, and not Uglify2. There's a big API difference between 1 and 2, so migrating to Uglify2 would be as easy as migrating to Babel as suggested in #6000.

With that said, neither is extremely hard. Probably a few weeks of work. Most of the work is to rewrite a small number of passes we have from Uglify1 to the new API - it's a small number since we don't need to modify any asm.js passes, just the ones that run on generic JS, not many of those.

@curiousdannii
Copy link
Contributor

curiousdannii commented Jan 10, 2018

I didn't realise that this was an issue not just for the optimiser, but also for parsing library files. :(

@kripken
Copy link
Member

kripken commented Jan 10, 2018

Only when they are parsed by the optimizer, though. If JS isn't being optimized (-O0, -O1, or if JS opts are disabled in higher opt levels), then it's just copied through. Unless I'm missing something?

@curiousdannii
Copy link
Contributor

Oh, you're right, sorry. I'm building with -O3.

@saschanaz
Copy link
Collaborator

Personally I don't like what Emscripten does with npm packages, can we install from npm instead of copy-pasting whole packages into the repo?

@kripken
Copy link
Member

kripken commented Jan 11, 2018

Yeah, in general it would be good to use npm properly for upstream things. The Uglify1 code we use is an exception though since it's been modified and exists only here (so I'm not sure of the benefit of using npm for it).

@curiousdannii
Copy link
Contributor

How is UglifyJS called? Pass a string in, get a string out? Do we use the UglifyJS parser for any other purposes? If it's used mostly as a closed box then replacing it will be more straightforward.

@kripken
Copy link
Member

kripken commented Jan 12, 2018

We use it for asm.js in some ways that are irrelevant to this topic. For the non-asm.js stuff that is relevant, we have 2 main uses:

  • Just use it to minify whitespace. We do that in -O2 and above by default. This is a black box use and is easily replaced.
  • A few JS optimization passes run on the non-asm.js code. In these cases, Ugilfy generates an AST, which we then optimize, then pass back to Uglify to get the JS. To replace these, those passes would need to be rewritten to use the a new AST (of whatever thing we switch to). There are few passes here (JSDCE, the metadce helper code) and they are fairly short, and we have tests written that will make verifying the rewrites easy. But this will take some work, probably more than a few days.

@curiousdannii
Copy link
Contributor

curiousdannii commented Jan 12, 2018

Just use it to minify whitespace. We do that in -O2 and above by default. This is a black box use and is easily replaced.

Is this the code, and could this function therefore be replaced with one using Babel? If so it could be an easy place to start. However it does receive the ast, not a string...

https://github.com/kripken/emscripten/blob/5952e749af724ee8c1c3838e7ed8497c816d051b/tools/js-optimizer.js#L171-L178

@kripken
Copy link
Member

kripken commented Jan 12, 2018

Yes, that's it. The ast is generated near the bottom of the file, as we read the input and parse it.

Probably we'd need a new file for Babel. So there would still be the current js-optimizer (still used on asm.js), and the new one would provide the same external interface (but just support the small set of passes we port to Babel).

@mathiasbynens
Copy link

This can be solved by switching from uglify to terser.

@kripken
Copy link
Member

kripken commented Dec 5, 2018

@mathiasbynens

This can be solved by switching from uglify to terser.

Looking into this now, I may be missing something, but terser does not seem to have an AST compatible with Uglify1. Reading

The AST looks similar to the SpiderMonkey AST (but is apparently not identical). For example, in Uglify1 a conditional would be ['conditional', x, y, z], while terser has { TYPE: 'conditional', condition: x, consequent: y, alternative: z [..] }.

So switching to terser seems similar to switching to Babel/Acorn/etc. - a rewrite of our JS optimizer passes. (Which isn't the end of the world - for wasm we have very few relevant passes, as discussed above. But if we had a compatible AST we could have done it in a day...)

@saschanaz
Copy link
Collaborator

Maybe we can write a compatibility layer for uglify1/uglify2(terser) to ease migration.

@kripken
Copy link
Member

kripken commented Dec 5, 2018

@saschanaz To convert between the ASTs? I think the hard part would be translating ES6-specific constructs there.

In any case, the crucial passes are probably just a few hundred lines of code - around a week or so of work, I'd guess, for someone familiar with both ASTs.

@curiousdannii
Copy link
Contributor

Long term I think moving Emscripten to operating on Acorn ASTs would be best as then Babel would be an option too. Terser can accept an Acorn AST, but it does convert it internally. But as long as the Emscripten passes were run before Terser, that should still work I think?

@kripken
Copy link
Member

kripken commented Feb 7, 2019

PR up for this, feedback very welcome,

#7973

This issue is a duplicate of #6000 I believe, closing in favor of that one.

@kripken kripken closed this as completed Feb 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants