feat: update to `uglify-es` by Hartorn · Pull Request #63 · webpack-contrib/uglifyjs-webpack-plugin

Hartorn · 2017-06-30T00:06:32Z

@michael-ciniawsky @hulkish @graingert : Here is the new PR.

This PR is about moving to uglify-es (following of #55, due to messy chained rebase/push-force, etc...)

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, I will do in the next days.

TODO :

[ ] Update the documentation
[ ] Add new test for the new functionnalities

# Conflicts: # src/index.js

…class

# Conflicts: # package.json

… eslint

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

Hartorn · 2017-06-30T00:13:11Z

@hulkish Honestly, I'm not so familiar with unit testing like this. I corrected the existing ones (only the configuration given to the plugin). I might need help on this, if anyone wants to lend a hand :)

michael-ciniawsky · 2017-06-30T00:27:33Z

I take care of the docs in #61 after this PR :)

cc @hulkish for Unit testing

hulkish · 2017-06-30T00:34:30Z

@Hartorn sure, you wanna give me access to your fork and ill add the test.

hulkish · 2017-06-30T00:43:16Z

@Hartorn Nvm, I see you've already included your updates to the tests: https://github.com/webpack-contrib/uglifyjs-webpack-plugin/pull/63/files

This looks good to me, 👍 🌮

Hartorn · 2017-06-30T00:48:09Z

@hulkish @michael-ciniawsky Already invited you both to my fork, maybe it will be useful.

I pushed the modifications to the package-lock, did not pay attention the repo was in npm 5.
Build should be all good

@hulkish I thought you were talking about adding new cases for uglify-es options.
I might still need to add a check on source map (the one inside uglifyOptions must be ignored, we only care about the one in options).
Thanks for checking :) 👍

graingert · 2017-06-30T00:49:10Z

+      warnings,
+      parse: parse || {},
+      compress: compress || {},
+      /* eslint-disable no-undefined */


eslint-disable-next-line

michael-ciniawsky

We need to find a way to avoid the options nesting

michael-ciniawsky · 2017-06-30T00:52:09Z

          return `License information can be found in ${licenseFile}`;
        },
      },
+      uglifyOptions: {


Possible to keep the individual options in the top level e.g via filter() ?

I felt like they were too many options to keep them at top level. (https://github.com/mishoo/UglifyJS2/tree/harmony#minify-options-structure)
That way, I thought it was easier to understand what's coming from the plugin, or what's coming from UglifyJs, and so easier to read the docs according to the need.

hmm.. I basically agree 😛, but uglifyOptions is too verbose (but no better idea atm, just complaining :D). What's your opinion on removing warningFilters && extractComments ? 🙃 What is especially the usecase for warningFilters?

warningFilters : I think it can be used by some people to filter for example on node_modules and still see warnings about their code, maybe to check dead code presence, or optimisation.

extractComment: It is for copyright/license reason I think => that way, the file you load is still small, plus a preamble redirecting to another file containing all the copyrights/licenses.

graingert · 2017-06-30T00:55:53Z

+      parse: parse || {},
+      compress: compress || {},
+      // eslint-disable-next-line no-undefined
+      mangle: mangle === undefined || mangle === null ? true : mangle,


mangle == null is idiomatic

@graingert I don't understand this. What you mean is that I should not test mangle === null but only mangle === undefined ? true : mangle ?

mangle === undefined || mangle === null is the same as mangle == null

Thks for the explanation.

michael-ciniawsky · 2017-06-30T01:11:43Z

What are the uglifyOptions defaults atm => new UglifyJSPlugin() ?

graingert · 2017-07-05T21:53:59Z

Smash it in Smash it in Smash it in

…

On 5 Jul 2017 22:53, "Michael Ciniawsky" ***@***.***> wrote: I did already and ask in slack aswell no response until now ¯_(ツ)_/¯ — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#63 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAZQTLC2rF0mlzWXvWCOeMkx4_EPM9b-ks5sLAXAgaJpZM4OKAfG> .

michael-ciniawsky · 2017-07-05T22:41:06Z

⚠️ Please wait with the next beta release I quickly finish the docs PR :)

joshwiens · 2017-07-06T03:06:36Z

@jdalton @graingert ...

joshwiens · 2017-07-06T03:11:42Z

For all involved, this is staying on a beta tag at minimum, until the test suite is reliable. I personally wouldn't have merged this yet given Tobias should have looked at it & we didn't answer the performance question though I do understand everyones desire to get this in master.

@Hartorn - Please see my comment in #67

The latest version of striptags is causing the build task to fail because it uses ES6, which is not supported by the version of Uglify that Webpack is using. As soon as [1] is published to a main release and Webpack updates to use that release, we should be able to upgrade striptags to the latest version. [1] webpack-contrib/uglifyjs-webpack-plugin#63

See webpack-contrib/uglifyjs-webpack-plugin#63 and https://github.com/mishoo/UglifyJS2/tree/harmony for more information about this. Fix angular#7532

openjck · 2018-02-23T01:42:37Z

This hasn't been released yet, has it?

edit: Apologies. Answered my own question. Yes, it has been released.

Hartorn and others added 9 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

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

7e42082

… eslint

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

michael-ciniawsky assigned hulkish and michael-ciniawsky Jun 30, 2017

michael-ciniawsky added priority: 2 (urgent) pr_action: Review semver: Major type: Feature labels Jun 30, 2017

michael-ciniawsky added this to the 1.0.0 milestone Jun 30, 2017

Updating package-lock.json and package.json for uglify-es

f317b26

michael-ciniawsky requested review from hulkish, joshwiens and michael-ciniawsky June 30, 2017 00:48

graingert suggested changes Jun 30, 2017

View reviewed changes

Using eslint-disable-next-line instead of eslint-disable

d950f5b

michael-ciniawsky reviewed Jun 30, 2017

View reviewed changes

graingert suggested changes Jun 30, 2017

View reviewed changes

michael-ciniawsky added pr_action: Merge ready and removed pr_action: Review labels Jul 5, 2017

michael-ciniawsky merged commit 1d62560 into webpack-contrib:master Jul 5, 2017

michael-ciniawsky added the status: Approved label Jul 5, 2017

michael-ciniawsky unassigned hulkish and michael-ciniawsky Jul 5, 2017

michael-ciniawsky removed flag: Performance priority: 2 (urgent) pr_action: Merge ready labels Jul 5, 2017

This was referenced Jul 6, 2017

switch to uglify-es #33

Closed

Support UglifyJS 3 #32

Closed

tomkel mentioned this pull request Jul 6, 2017

Consider uglify-es by default TrySound/rollup-plugin-uglify#34

Closed

fregante mentioned this pull request Jul 29, 2017

Uglify all the things refined-github/refined-github#653

Merged

filipesilva mentioned this pull request Sep 5, 2017

fix(@angular/cli): don't break on es6 module uglification angular/angular-cli#7602

Closed

michael-ciniawsky removed this from the 2.0.0 milestone Oct 29, 2017

RomanHotsiy mentioned this pull request Nov 30, 2017

2.0.0-alpha.x issues Redocly/redoc#384

Closed

This was referenced Dec 7, 2017

Update striptags to the latest version 🚀 mozilla/experiments-viewer#328

Closed

Update striptags to the latest version 🚀 mozilla/distribution-viewer#317

Closed

openjck mentioned this pull request Feb 23, 2018

Update dependency striptags to v3 mozilla/distribution-viewer#344

Closed

openjck mentioned this pull request Feb 23, 2018

Update dependency striptags to v3 (Renovate, please reopen) mozilla/experiments-viewer#357

Closed

alekseyzadorozhniy mentioned this pull request Nov 23, 2018

Update options for uglifyjs in Webpack plugin sysgears/spinjs#24

Merged

snyk-bot mentioned this pull request May 17, 2022

[Snyk] Upgrade webpack-sources from 1.3.0 to 1.4.3 AsherLi0103/uglifyjs-webpack-plugin#3

Open

Uh oh!

Uh oh!

Conversation

Hartorn commented Jun 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Hartorn commented Jun 30, 2017

Uh oh!

michael-ciniawsky commented Jun 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hulkish commented Jun 30, 2017

Uh oh!

hulkish commented Jun 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Hartorn commented Jun 30, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michael-ciniawsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michael-ciniawsky Jun 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hartorn Jun 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michael-ciniawsky commented Jun 30, 2017

Uh oh!

graingert commented Jul 5, 2017 via email

Uh oh!

michael-ciniawsky commented Jul 5, 2017

Uh oh!

joshwiens commented Jul 6, 2017

Uh oh!

joshwiens commented Jul 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjck commented Feb 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

Hartorn commented Jun 30, 2017 •

edited

Loading

michael-ciniawsky commented Jun 30, 2017 •

edited

Loading

hulkish commented Jun 30, 2017 •

edited

Loading

michael-ciniawsky Jun 30, 2017 •

edited

Loading

Hartorn Jun 30, 2017 •

edited

Loading

joshwiens commented Jul 6, 2017 •

edited

Loading

openjck commented Feb 23, 2018 •

edited

Loading