refactor(bundler): uses uglify-es in place of uglify-js

[rodu]

With better support of ES6+ in modern browsers and the introduction
of babel-preset-env it's possible to serve ES6 code to clients able
to consume it. This change uses uglify-es in place of ugliify-js to
handle minification that supports ES6.

uglify-es is API/CLI compatible with uglify-js@3.
uglify-es is not backwards compatible with uglify-js@2.

Issue #490

[CLAassistant]

All committers have signed the CLA.

[3cp]

@Alexander-Taran I believe @rodu's primary goal here is not to reduce bundles size, it's to support bable-preset-env option on only targeting latest browsers where original uglify-js will fail.

This only reduces transpiled code on local js files. It doesn't reduce the biggest trunk, aka all npm packages that were all pre-compiled to work with old browsers.

[Alexander-Taran]

well.. for libraries that also distribute es6 modules it can open a way..
aurelia webpack plugin allows to select which versions to use for bundle..
if there is a native es with async.. it would improve somehow the bundle size..

[3cp]

Just tried this patch on one of my apps. All work, harmless. Not surprising, given uglify-es and uglify-js are from same developer.

[3cp]

In long term, I think it's better to push minify and source-map out of cli, to rely more on gulp pipeline, if that's possible.

[3cp]

Hi @rodu this PR is a breaking change, existing app that has no 'uglify-es' in their package.json file will fail.

To make it smoother for people to upgrade, I suggest to use try-catch block to try require('uglify-es') first, then fallback to require('uglify-js'). That will make this not a breaking change.

[Alexander-Taran]

@huochunpeng it actually adds a dependency to locally installed cli. I think.

[3cp]

I am talking about exiting app upgrading to newer cli. That requires user to manually change local package.json.

[3cp]

To clarify, that require('uglify-es') is resolved at runtime, in user app's deps, not cli's deps.

[Alexander-Taran]

@huochunpeng don't see what you mean.

[3cp]

Sorry, I missed that local install change. I did not test directly on this PR' code.

But this probably changes existing behavior, it means user does not need to install uglify at all.

[3cp]

@rodu just read your option1 and 2 on #490, I think you need to move 'uglify-es' from devDeps to deps to really make option1 to work.

When you use npm install from git repo, npm pulls all deps and devDeps. But for a future normal release of cli, a normal npm install aurelia-cli only pulls down deps, not devDeps.

[rodu]

@huochunpeng thank you for your feedback. I should have posted my indications for option 1 and 2 here.

I agree that adding the uglify-es dependency to the CLI package.json changes the existing behaviour slightly. I did consider the possibility of using a try/catch to test the presence of the uglify-es package, and in case fallback to uglify-js, if that's missing. I pushed a different branch to try that approach.

At first, it all seems to make sense, but then I am actually not able to test it properly. When I run the build in one of my existing projects, the CLI looks for uglify-es in the CLI node_modules, not in the project node_modules, even though I have one there.
It must be the use of npm link aurelia-cli that I am using to test things out. But I don't have such a deep knowledge of npm. If I physically copy the CLI folder to my node_modules, then it works, because it finds uglify-es in my project now.

I will push a commit to move the uglify-es from devDeps to deps as you suggested. Apart from that, should we also consider the try/catch approach?

[3cp]

You are welcome. npm link confused hell out of me before, the behaviour is different from normal install. So I stopped using it. I guess it's due to the symlink. Now if I want to test my local changes, I either install it from my github repo, or simply replacing my node_modules/aurelia-cli with a copy (not symlink) of my local aurelia-cli repo.

@JeroenVinke will have the final decision. Personally I think, for end users, try-catch is more friendly than adding it to deps.

[Alexander-Taran]

If it settles with try catch - there should be a warning message maybe in case configs target es6 modules..
in ts or babel.. don't know how to figure it out.

[rodu]

Using the try/catch seems a better approach, allowing a fall-back configuration without adding dependencies to the CLI packages.

• New projects will use uglify-es by default.
• Existing projects will use what they currently have, but will get a warning by default if using uglify-js, just to inform users that they can switch to uglify-es if they want to support for ES6 minification.

Please advise on this approach and if you are happy with the warning messages.

The way to test the patch changes a little. Instead of using npm link aurelia-cli in a project it's better to install the patch directly with: npm install rodu/cli#feat-uglify-es and then run the build as usual.

[Alexander-Taran]

@JeroenVinke need your opinion here

[JeroenVinke]

Hey all :) thanks for working on this @rodu and your feedback @huochunpeng

I think we might have to add uglify-es as a dependency of the cli due to #803 (comment). It does work to keep the uglify-es dependency in the package.json of the app but since the cli is the entity that's using uglify-es it should declare it in the package.json of the aurelia-cli

That would also remove the need for the try catch, but we'd need to make sure that switching to uglify-es is not a breaking change

[3cp]

I added some notes in #857 about new project template cleanup.

[3cp]

Switch to uglify-es might introduce side-effect to new tracer in my #862.

Not because uglify-es itself, which is after tracing. It is the transpiled code that could be in newer syntax. I need to test more to make sure new tracer can (most likely) handle them.

Update: tested one app with babel-preset-env with default targeting latest browsers, the new tracer works fine.

"presets": [
    ["env", {"loose": true}],
    "stage-1"
  ],

[3cp]

@JeroenVinke one more thing, since uglify is going to in deps, we can move require('uglify-xx'); to top of the file, plus removing v3 check logic, don't need to delay the require anymore.

[rodu]

@huochunpeng to run your checks about the tracer making sure to be using ES6 output I would suggest you use this configuration for babel:

"presets": [
  ["env", { "targets": { "chrome": 62 } }],
  "stage-1"
]

This way you are asking to target only Chrome 62+ which largely supports ES6. I think with env only it will transpiler in such a way to support browsers like IE11. More here.

@JeroenVinke thanks for your feedback too. For the CLI, adding the uglify-es dependency seems to work well for me, therefore also removing the try/catch.

[3cp]

@rodu, thx, I tested with "targets": { "browsers": ["Firefox ESR"] } before. All good.

You can further

• remove isV3 check and all code branch for dealing uglify-js v2.
• move require('uglify-es') to top of the file.

[JeroenVinke]

Thanks everyone for helping out. Do you think this is ready to go?

[3cp]

I installed this branch as my global cli, created new app, modified package.json to use this branch. All good for me.

One problem, with npm, it consistently hitting bug #870. Let me know if you can reproduce it.

[3cp]

Wait for #884 to be merged first.

[3cp]

FYI, bad news.

uglify-es is no longer maintained
webpack-contrib/uglifyjs-webpack-plugin#285

Terser is actively maintained fork.
terser/terser#2
https://github.com/TrySound/rollup-plugin-uglify/releases/tag/v4.0.0

[rodu]

OK, thanks for keeping an eye on this. I am not sure if the situation is already stable enough to use the new fork. In case let me know at some point. I guess there is still to wait for #884 anyway from your previous comment.

[3cp]

@JeroenVinke now we have another reason to switch from uglify-js to uglify-es or probably terser.

Even with babel targeting es2015, the currently uglify-js still could fail on vendor js file (namely CKEditor) who ship js code with using "const".

SyntaxError: Unexpected token: keyword (const)

[JeroenVinke]

#907 has been merged