feat: update to `uglify-es` #55

Hartorn · 2017-06-24T21:38:19Z

I tried this, because I wanted to be able to use webpack, with uglify es and not js.

Basically, I moved the options for Uglify into uglifyOptions key, and added an option for ES5 or ES6.

I also changed the code, to use the uglify.minify method.

Not sure if I understood something wrong, but I hope this can help.
I can correct this if needed (tested on some of my projects, and it seemed to work)

EDIT :
Here is a quick sum up of the modifications, please ask for precisions if needed.

Uglify now expose a single high level function minify(code, options), which go through all the steps of minification (parse, compress, mangle, output)
it does not throw errors anymore, so the try-catch is not needed (test if there is an error key in the return of the function)
it is possible to get the warnings in the result also, so it is not needed anymore to mingle with AST_NODE warn_function
it is possible to give a filter for comments, so we can hook on that for exporting comments
it is possible to give a preamble in the output, so we can use that for the banner thing (License file, comments, ...)

Basically, we do not need to use any other function than minify from uglify, it is mainly about giving proper options, handle the source maps and output.

I did not had time to update the docs, will do in the next days.

jsf-clabot · 2017-06-24T21:38:20Z

Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
3 out of 5 committers have signed the CLA.

✅ bebraw
✅ d3viant0ne
✅ mkarajohn
❌ SebastianS90
❌ jumoel

michael-ciniawsky · 2017-06-26T00:01:04Z

Fixes #32, #33

michael-ciniawsky

Could you please elaborate a bit on the removals? They are related to the breaking API changes between ugilfy-js && ugilfy-es ?

michael-ciniawsky · 2017-06-26T00:01:55Z

src/index.js

 const RequestShortener = require("webpack/lib/RequestShortener");
 const ModuleFilenameHelpers = require("webpack/lib/ModuleFilenameHelpers");
-const uglify = require("uglify-js");
+const uglifyEs = require("uglify-es");


Better dynamically required in L24?

michael-ciniawsky · 2017-06-26T00:03:05Z

src/index.js

+
+		this.uglifyOptions = Object.assign({}, this.options.uglifyOptions || {}, { sourceMap: null });
+
+		this.uglify = this.options.es6 ? uglifyEs : uglifyJs;


this.uglify = this.options.es6 ? require('uglify-es') : require('uglify-js');

michael-ciniawsky · 2017-06-26T00:04:03Z

src/index.js

-		if(typeof options.compressor !== "undefined") options.compress = options.compressor;
-		this.options = options;
+
+		this.options = Object.assign({}, options);


this.options = options || {};

There is nothing to assign here (e.g defaults)

michael-ciniawsky · 2017-06-26T00:05:29Z

src/index.js

-					const oldWarnFunction = uglify.AST_Node.warn_function;
-					const warnings = [];
+					// Copy uglify options
+					const uglifyOptions = Object.assign({}, this.uglifyOptions);


const options = this.uglifyOptions || {};

michael-ciniawsky · 2017-06-26T00:07:21Z

src/index.js

-								}
-							}
+						if(minifyResult.error) {
+							throw minifyResult.error;


this.emitError(minifyResult.error)

? Not 💯 if this.emitError is Loader API only

michael-ciniawsky · 2017-06-26T00:12:07Z

Not sure about the semver tbh, guessing it would be major

graingert · 2017-06-26T00:32:46Z

What's the point of supporting uglify-js. Doesn't uglify-es support a superset of the functionality of uglify-js?

graingert · 2017-06-26T00:33:33Z

No need for peerdep either, that was just a hack to support the harmony branch on github

michael-ciniawsky · 2017-06-26T00:42:35Z

What's the point of supporting uglify-js doesn't uglify-es support a superset of the functionality of uglify-js?

Yeah... good point, we need to clearify that once and for all 😛

michael-ciniawsky · 2017-06-29T03:47:27Z

Sry it landed in master now 😅 , could you switch again and rebase against current master one more time please 😛

graingert · 2017-06-29T08:19:02Z

package.json

@@ -82,4 +81,4 @@
  },
  "homepage": "https://github.com/webpack-contrib/uglifyjs-webpack-plugin",
  "license": "MIT"
-}
+}


invalid text file. Files a sequence lines terminated with a newline character.

hulkish · 2017-06-29T19:02:54Z

@Hartorn can you please rebase to master?

michael-ciniawsky · 2017-06-29T19:03:49Z

yep, please rebase against current master, sry for the inconvenience again, this is definitely the next to land 💯 😛

hulkish · 2017-06-29T19:08:05Z

@Hartorn Also, i noticed you removed the wrapping try/catch here: https://github.com/webpack-contrib/uglifyjs-webpack-plugin/pull/55/files#diff-1fdf421c05c1140f6d71444ea2b27638L55. Can you explain that a bit?

# Conflicts: # src/index.js

…class

# Conflicts: # package.json

Hartorn · 2017-06-29T20:32:55Z

@hulkish @michael-ciniawsky done rebasing. I will have a look at the test now.

@hulkish Uglify does not throw error anymore, instead it is given in the result object.
See the few lines above https://github.com/mishoo/UglifyJS2/tree/harmony#minify-options

… eslint

michael-ciniawsky · 2017-06-29T23:32:18Z

⚠️ The git commit author history seems to be messed up, you either need to edit the authors via git or open a fresh PR, otherwise the CLA Bot will complain and block the PR

* Putting back try-catch (wrong asset handling) * Adding banner for comments after being sure comments are presents * Correcting filter extraction * Correcting error handling

hulkish · 2017-06-29T23:55:33Z

@Hartorn I recommend making a new pr based on master and just re apply your changes - sorry for the hassle, but I think this is your easiest route tbh.

Also, can you please make a new test for these new options that are supported in uglify-es? see https://github.com/webpack-contrib/uglifyjs-webpack-plugin/blob/master/test/all-options.test.js for an example - you could probably just copy this test and swap out the options with your options.

michael-ciniawsky changed the title ~~Allowing to use both ES5 or ES6 Uglify 3~~ feat: allow usage of both ES5 or ES2015 uglify versions Jun 26, 2017

michael-ciniawsky added priority: 3 (required) severity: 4 (inconvenient) status: Blocked type: Feature labels Jun 26, 2017

michael-ciniawsky suggested changes Jun 26, 2017

View reviewed changes

michael-ciniawsky added pr_action: Discuss semver: Major labels Jun 26, 2017

michael-ciniawsky assigned hulkish, bebraw, alexander-akait and joshwiens Jun 26, 2017

michael-ciniawsky requested review from hulkish, bebraw, alexander-akait and joshwiens June 26, 2017 00:11

michael-ciniawsky assigned michael-ciniawsky and unassigned bebraw, alexander-akait and joshwiens Jun 26, 2017

michael-ciniawsky mentioned this pull request Jun 26, 2017

refactor: upgrade to uglifyes v3.0.0 #54

Closed

michael-ciniawsky added flag: Community help wanted flag: Needs more info flag: Needs Tests labels Jun 26, 2017

michael-ciniawsky removed the status: Blocked label Jun 29, 2017

michael-ciniawsky removed the pr_action: Discuss label Jun 29, 2017

graingert reviewed Jun 29, 2017

View reviewed changes

michael-ciniawsky mentioned this pull request Jun 29, 2017

refactor: changed __UglifyJsPlugin dereference to use WeakSet instead #57

Merged

Hartorn and others added 6 commits June 29, 2017 22:08

Updating to Uglify 3 (both ES6 or ES5), using new API

a69c085

# Conflicts: # src/index.js

Adding extractComments options handling

315a5e4

Handling error and warnings correctly, moving comments function into …

7152f94

…class

Correcting small mistakes

07b37a0

Correcting lint (tab, space, ...)

49c199d

Correcting package.json spacing, correcting export

35d9f9d

# Conflicts: # package.json

Hartorn changed the base branch from feature-defaults to master June 29, 2017 20:28

michael-ciniawsky changed the title ~~feat: update to uglifyes~~ feat: update to uglify-es Jun 29, 2017

Correcting static function calls, changing file list building, fixing…

7e42082

… eslint

Hartorn added 2 commits June 30, 2017 01:46

Correcting test cases (by restructuring options)

890ea12

Corrections to respect test

c9c39d5

* Putting back try-catch (wrong asset handling) * Adding banner for comments after being sure comments are presents * Correcting filter extraction * Correcting error handling

Hartorn closed this Jun 29, 2017

Hartorn mentioned this pull request Jun 30, 2017

feat: update to uglify-es #63

Merged

michael-ciniawsky removed this from the 1.0.0 milestone Jun 30, 2017

michael-ciniawsky removed flag: Community help wanted priority: 2 (urgent) pr_action: Rebase semver: Major type: Feature labels Jun 30, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: update to `uglify-es` #55

feat: update to `uglify-es` #55

Hartorn commented Jun 24, 2017 •

edited

Loading

jsf-clabot commented Jun 24, 2017 •

edited

Loading

michael-ciniawsky commented Jun 26, 2017 •

edited

Loading

michael-ciniawsky left a comment

michael-ciniawsky Jun 26, 2017

michael-ciniawsky Jun 26, 2017

michael-ciniawsky Jun 26, 2017

michael-ciniawsky Jun 26, 2017

michael-ciniawsky Jun 26, 2017

michael-ciniawsky commented Jun 26, 2017 •

edited

Loading

graingert commented Jun 26, 2017 •

edited

Loading

graingert commented Jun 26, 2017

michael-ciniawsky commented Jun 26, 2017 •

edited

Loading

michael-ciniawsky commented Jun 29, 2017

graingert Jun 29, 2017

hulkish commented Jun 29, 2017

michael-ciniawsky commented Jun 29, 2017 •

edited

Loading

hulkish commented Jun 29, 2017 •

edited

Loading

Hartorn commented Jun 29, 2017

michael-ciniawsky commented Jun 29, 2017

hulkish commented Jun 29, 2017 •

edited

Loading


		this.uglifyOptions = Object.assign({}, this.options.uglifyOptions \|\| {}, { sourceMap: null });

		this.uglify = this.options.es6 ? uglifyEs : uglifyJs;

feat: update to uglify-es #55

feat: update to uglify-es #55

Conversation

Hartorn commented Jun 24, 2017 • edited Loading

jsf-clabot commented Jun 24, 2017 • edited Loading

michael-ciniawsky commented Jun 26, 2017 • edited Loading

michael-ciniawsky left a comment

Choose a reason for hiding this comment

michael-ciniawsky Jun 26, 2017

Choose a reason for hiding this comment

michael-ciniawsky Jun 26, 2017

Choose a reason for hiding this comment

michael-ciniawsky Jun 26, 2017

Choose a reason for hiding this comment

michael-ciniawsky Jun 26, 2017

Choose a reason for hiding this comment

michael-ciniawsky Jun 26, 2017

Choose a reason for hiding this comment

michael-ciniawsky commented Jun 26, 2017 • edited Loading

graingert commented Jun 26, 2017 • edited Loading

graingert commented Jun 26, 2017

michael-ciniawsky commented Jun 26, 2017 • edited Loading

michael-ciniawsky commented Jun 29, 2017

graingert Jun 29, 2017

Choose a reason for hiding this comment

hulkish commented Jun 29, 2017

michael-ciniawsky commented Jun 29, 2017 • edited Loading

hulkish commented Jun 29, 2017 • edited Loading

Hartorn commented Jun 29, 2017

michael-ciniawsky commented Jun 29, 2017

hulkish commented Jun 29, 2017 • edited Loading

feat: update to `uglify-es` #55

feat: update to `uglify-es` #55

Hartorn commented Jun 24, 2017 •

edited

Loading

jsf-clabot commented Jun 24, 2017 •

edited

Loading

michael-ciniawsky commented Jun 26, 2017 •

edited

Loading

michael-ciniawsky commented Jun 26, 2017 •

edited

Loading

graingert commented Jun 26, 2017 •

edited

Loading

michael-ciniawsky commented Jun 26, 2017 •

edited

Loading

michael-ciniawsky commented Jun 29, 2017 •

edited

Loading

hulkish commented Jun 29, 2017 •

edited

Loading

hulkish commented Jun 29, 2017 •

edited

Loading