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

Consider allowing alternate minifier instead of uglify for better ESNext support #490

Closed
delebash opened this Issue Feb 16, 2017 · 11 comments

Comments

Projects
None yet
7 participants
@delebash

delebash commented Feb 16, 2017

I'm submitting a feature request

  • Library Version:
    0.24.0

Please tell us about your environment:

  • Operating System:
    Windows 10

  • Node Version:
    7.50

  • NPM Version:
    4.0.5
  • Browser:
    Chrome 56

  • Language:
    ESNext

Current behavior:
UglifyJS does not support some ESNext features available in babel and current browsers such as Generators and Async Functions.

Both fail with error 'SyntaxError: Unexpected token operator «*», expected punc «(»',

Expected/desired behavior:
ESNext features supported by babel should also be supported by the aurelia-cli minification process.

To reproduce
Add a generator function to your code and run au build --env prod

Possible solutions:
If the minification process is extracted to a gulp task then the user can swap out uglify with
babili which supports the same features as babel.

Uglify does have a harmony version which has better support for ESNext features but there is a long discussion on which might be better to use moving forward. By extracting to a gulp task it gives the user the ability to choose.

@AStoker

This comment has been minimized.

Member

AStoker commented Feb 16, 2017

I could be an extra layer of thick today, so bear with me if I'm overlooking something :)
You're talking about the process of transpiling the code, right? That process takes place in the transpile task located inside your aurelia_project/tasks folder. In there, there's a gulp task that uses Babel to transpile. But that process is completely up to the user to change, convert, or do whatever to. If you don't want to use Babel, are you not simply able to swap out the transpiler there?

@delebash

This comment has been minimized.

delebash commented Feb 16, 2017

No :) When you specify --env prod aurelia-cli runs uglify-js process which is baked into the cli, uglify is not a gulp task in aurelia_project/tasks. If you look at node_modules/aurelia-cli/lib/build/bundle.js you can see uglify is hard coded.

const UglifyJS = require('uglify-js'); //added to user project devDependencies
let minificationResult = UglifyJS.minify(String(contents), minificationOptions);

Yes you can use any transpiler you want with your own minification process via gulp tasks, but if you want to use the built in --env prod switch then aurelia-cli will use it's own minification process via uglify.

@AStoker

This comment has been minimized.

Member

AStoker commented Feb 16, 2017

Okay, thanks for getting me straight.
So the Babel task happens first, and then the uglification happens as the last step. So what babel is outputting isn't compatible with the uglifier built in, correct?
It sounds reasonable to have something like that pluggable. Do you think you could generate a PR for that so we could take a look?

@delebash

This comment has been minimized.

delebash commented Feb 16, 2017

Yep. It will be awhile before I can really dig in, but as soon as I get the chance I will. Right now just installing the harmony branch of uglify takes care of generators and async. I have only tested with generators and async so I am not sure what other ESNext features uglify-js harmony provides.

@kushwahashiv

This comment has been minimized.

kushwahashiv commented Jun 23, 2017

I am also getting the error

[12:52:01] SyntaxError: Unexpected token operator «*», expected punc «(»
    at JS_Parse_Error.get (eval at <anonymous> (/Users/Shiv/projects/rapidui-parser/node_modules/uglify-js/tools/node.js:27:1), <anonymous>:86:23)
    at formatError (/Users/Shiv/projects/rapidui-parser/node_modules/gulp/bin/gulp.js:165:17)
    at Gulp.<anonymous> (/Users/Shiv/projects/rapidui-parser/node_modules/gulp/bin/gulp.js:195:15)
    at emitOne (events.js:120:20)
    at Gulp.emit (events.js:210:7)
    at Gulp.Orchestrator._emitTaskDone (/Users/Shiv/projects/rapidui-parser/node_modules/orchestrator/index.js:264:8)
    at /Users/Shiv/projects/rapidui-parser/node_modules/orchestrator/index.js:275:23
    at finish (/Users/Shiv/projects/rapidui-parser/node_modules/orchestrator/lib/runTask.js:21:8)
    at /Users/Shiv/projects/rapidui-parser/node_modules/orchestrator/lib/runTask.js:45:4
    at <anonymous>
[12:52:01] 'rapidui-parser:build-release:clean' errored after 1.62 s
[12:52:01] SyntaxError in plugin 'run-sequence(rapidui-parser:build:bundles)'
Message:

is there any workaround for this?

Any quick help would be appreciated.

/Shiv

@delebash

This comment has been minimized.

delebash commented Jun 23, 2017

Are you using async or generator functions in your code? If so did you try installing the harmony branch of uglifyjs? Run npm install uglify-es -g to install the latest harmony release.

@rodu

This comment has been minimized.

rodu commented Apr 3, 2018

Would it be an option to use uglify-es instead of uglify-js?

For example, when using the new babel-preset-env, transpiling can easily produce ES6 code, but then the build fails because of uglify-js.

I have created a branch of the CLI using uglify-es at bundling time. It seems to work for me with ES5/ES6 output from babel. Would this be a viable solution?

@Alexander-Taran

This comment has been minimized.

Member

Alexander-Taran commented Apr 3, 2018

@rodu could it be also incorporated in ts builds?
and webpack builds for that matter?
Also, you can create a PR.. so we could look at your modifications.

@rodu

This comment has been minimized.

rodu commented Apr 4, 2018

From my tests, webpack@4 seems OK as it should be using uglify-es under the hood already:
https://webpack.js.org/plugins/uglifyjs-webpack-plugin/

For TypeScript, I created a new project and changed the tsconfig.json to target ES6 output. Then the au build --prod completed successfully. Same for ESNext project using babel to generate ES6 code as output.

I am not an expert of the CLI by any means, and while I tried to do my best, some more verifications of the patch would be very welcome.

@Alexander-Taran

This comment has been minimized.

Member

Alexander-Taran commented Apr 4, 2018

@rodu
can you comment on how ppl can test your PR in their setups?
https://discourse.aurelia.io/t/testers-welcome/1037

@rodu

This comment has been minimized.

rodu commented Apr 5, 2018

From my analysis, new projects created with the CLI containing this patch should work out of the box. This is easy to test:

  1. Take a note of the current CLI you are using: au --version just in case you want to reinstall that
  2. Pull down the patch
  3. uninstall your current aurelia-cli (npm uninstall -g aurelia-cli)
  4. Follow the instructions in the README of the CLI project to build the patch version
  5. Make sure to run npm link aurelia-cli in your existing project as the README says
  6. Run your project build as usual and observe how minification completes

I would expect that existing projects can keep the current configuration and build with the new version of the CLI that uses uglify-es with no problems. Unless they were created long ago and have some other kind of incompatibility. Those projects should probably not even bother and keep the CLI they have.

Please note the following

  1. In this change, we are adding the uglify-es dependency to the package.json of the CLI itself. Before, the dependency on the minifier was satisfied via the package.json of the project you generate using the CLI (au new).

Adding the dependency to the CLI package.json, existing projects that have uglify-js in their package.json and upgrade the CLI will be able to build without the need for installing uglify-es first, because the CLI npm install pulls that in.

This also means that future new projects created with this CLI patch will get the uglify-es dependency both from the CLI package.json and then from the project package.json. Surely one is redundant, but this would help smooth the transition for pre-existing projects.

  1. In an alternative to 1, if we change the minifier and don't add the uglify-es dependency in the CLI package.json, things are more in line with the initial approach where uglify-js was installed via the project dependency after au new. In that case, new projects would be OK, but people would have to manually install uglify-es in their existing projects (i.e.: npm install -D uglify-es) when upgrading the CLI.

I can highlight the situation here, but I would not know what's the best approach you would like to follow in these cases.

huochunpeng added a commit to huochunpeng/cli that referenced this issue Jul 27, 2018

feat: use terser to replace uglifyjs for better es6 support
Terser is the current active fork of uglify-es. This is not only required to support babel targeting es6, but also improve compatibility of any vendor js code which uses "const".

closes aurelia#883, aurelia#490, supersedes aurelia#864
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment