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

Ensure webpack users don't unknowingly use uglify-es #8350

Closed
gaearon opened this issue Nov 7, 2018 · 24 comments
Closed

Ensure webpack users don't unknowingly use uglify-es #8350

gaearon opened this issue Nov 7, 2018 · 24 comments

Comments

@gaearon
Copy link
Contributor

gaearon commented Nov 7, 2018

We keep getting bug reports in the React repo that turn out to be uglify-es bugs. It seems that many people use uglify-es through webpack without realizing it's been abandoned and is known to be buggy and produce incorrect code.

While I know you're switching to terser in 5.x IMO this is not sufficient. Ideally it would be great if there was a way for users of webpack 4.x to:

  • Easily opt into a "safe" minifier like uglify-js or terser, ideally with a one-line change
  • See a deprecation warning if they're running uglify-es encouraging to do the step above

Without a deprecation warning, it's likely people will keep bumping into these bugs for a few more years.

Does this sound like something you would consider?

@sokra
Copy link
Member

sokra commented Nov 7, 2018

I think emitting a warning in the plugin makes most sense.

@sophiebits
Copy link

If we could add an npm deprecation note to all 1.x versions of uglifyjs-webpack-plugin (which use uglify-es), it would be a little noisy, but I think that could help a lot.

@sokra
Copy link
Member

sokra commented Nov 7, 2018

You would get the message even if you are overriding the minimizer in webpack, since webpack depends on it.

@sokra
Copy link
Member

sokra commented Nov 7, 2018

The lastest version shouldn't be affected afaik because inlining is disabled by default

@kzc
Copy link

kzc commented Nov 7, 2018

It would make more sense to have webpack@4 simply use terser by default. It's a drop in replacement for both uglify packages.

AFAIK if terser-webpack-plugin were to accept uglifyOptions as an alias for terserOptions then webpack@4 could trivially replace uglifyjs-webpack-plugin with terser-webpack-plugin with no backwards compatibility issues. As it stands now terser-webpack-plugin is incompatible with uglifyjs-webpack-plugin due to this plugin API change.

@robpalme
Copy link

robpalme commented Nov 7, 2018

@sokra Which component is making the decision to disable inlining by default?

@kzc
Copy link

kzc commented Nov 8, 2018

The bug in question was in reduce_funcs in uglify-es@3.3.9 and is still present in webpack@4 by default. It is not related to the inline option which was partially disabled in uglifyjs-webpack-plugin@1.

But this specific bug is immaterial. Dozens of bugs in uglify-es@3.3.9 have been fixed in terser - including several bugs that still exist in uglify-js.

@robpalme
Copy link

robpalme commented Nov 8, 2018

Interesting. So Uglify was the first project to fix the constructor inlining issue back in Feb, then Terser cherry picked the fix. And the reason this it is a problem for webpack today is that Uglify-ES has not published since then, whereas Terser has.

@kzc
Copy link

kzc commented Nov 8, 2018

minifier ES version last release status
uglify-es@3.3.9 ES8 2018-01-27 not maintained
uglify-js@3.4.9 ES5 2018-08-31 maintained?
terser@3.10.11 ES9 2018-11-03 maintained

@alexander-akait
Copy link
Member

@kzc

uglify-js@3.4.9

maintained, but very bad

@kzc
Copy link

kzc commented Nov 8, 2018

@evilebottnawi There's no need to disparage uglify-js or its maintainer.

@sokra
Copy link
Member

sokra commented Nov 8, 2018

Ok you've got me. Let's switch to terser for webpack 4 and don't care if it might be a breaking change...

@Reinmar
Copy link

Reinmar commented Nov 13, 2018

I'm trying to understand why suddenly everyone started switching to terser. I understand that this is supposed to be a drop-in replacement for uglify-es and, hence, a transparent change to most of us. However, for projects like CKEditor 5, it means updating and explaining the change to our users (we want to recommend the same setup which webpack uses by default so people are familiar with it).

My point is – I'm not against making such changes – I completely understand that to move forward we need to change things. What I'm trying to understand, though, is the background behind this decision. Is terser better than uglify-es only because it resolved some existing issues (which makes it a better choice at the moment)? Or did it ensure stable funding and we finally have a standard minifier for JS? I keep my 🤞 for the latter because such a critical tool for the entire JS ecosystem should be backed somehow.

BTW, before I found this ticket I thought that only webpack 5 will be switching to terser. Now, I understand that webpack 4 will do that too. But when? To be honest, it would be good if there was a bit more transparency in such cases.

@gaearon
Copy link
Contributor Author

gaearon commented Nov 13, 2018

What I'm trying to understand, though, is the background behind this decision. Is terser better than uglify-es only because it resolved some existing issues (which makes it a better choice at the moment)?

Nobody is working on uglify-es. It's abandoned. terser resolved some of the bugs in uglify-es, but the more important part is that it's being actively maintained.

@Reinmar
Copy link

Reinmar commented Nov 13, 2018

Thanks for the clarification. Seeing the sudden adoption of terser, I hoped that it got some sustainable funding or other kind of support from some companies/organizations. I'm just worried that terser is being actively maintained for now, just like all other projects were at their time.

I can see that terser is currently maintained by @fabiosantoscode and @kzc – maybe you could shed some light on this, guys? It's of mutual interest for all of us to secure the position of the JavaScript standard minifier. Perhaps we could help?

@kzc
Copy link

kzc commented Nov 13, 2018

terser, like uglify-js and uglify-es before it don't receive any funding of any kind. It's solely an unpaid volunteer effort. If you're looking for some sort of guarantee of long term maintenance with this or any OSS packages, you are mistaken.

This discussion is about changing the default minifier from an abandoned one to one that is maintained. Does terser have fewer bugs than uglify-es? Most certainly. Is terser bug free? Of course not. If the webpack project thinks it will be better served by babel-minify, butternut, google closure or prepack - by all means go for it.

@Reinmar
Copy link

Reinmar commented Nov 13, 2018

If you're looking for some sort of guarantee of long term maintenance with this or any OSS packages, you are mistaken.

I'm not looking for any guarantees. I'm an OSS maintainer for many years now and I know how it works.

What I'm trying to achieve here is:

  1. understanding what's the current status of things,
  2. helping in finding a solution to potential problems.

JavaScript as a language/community requires a stable and maintained minifier. It's in the interest of all of us that at least one such solution exists. We should not assume that such a complex project is maintained by unpaid volunteers for years. I would never expect that anyone will be willing to spend her/his free time just for a fame and satisfaction. I don't think it can work in a long term.

As a JavaScript community we need a long term solution. We should not be forced to change a minifier every couple of months because a new, more "alive" solution appeared. I don't think this is good for our ecosystem if we're constantly changing the things.

So, tl;dr is: I wanted to know what's the status of things (now I know) and if this topic (sustainable funding) has been discussed or perhaps even resolved.

Thank you for your awesome work!

@kzc
Copy link

kzc commented Nov 13, 2018

I'm not looking for any guarantees.

This statement says otherwise:

JavaScript as a language/community requires a stable and maintained minifier.

Terser is as stable as any other OSS project, and better than most. It has more than 3000 unit tests and is regression tested against a number of javascript code bases. It works. The project has very few open issues and most of those are not bugs, but feature requests.

When uglify-es was no longer maintained no major project, company or organization stepped up to support it. Writing and maintaining a full featured ES6+ minifier is extremely difficult and only a handful of people have made any significant contributions to uglify and terser. Babel-minify also has similar problems with contributions. Google Closure is written in Java and has its own issues with speed and memory consumption. Butternut has been abandoned. Prepack is an early alpha project. These are the only options available today. Take your pick.

Since I'm here in the webpack forum, I'd like to request that webpack support a --no-minify option that will work with production mode in order to improve the quality of minify bug reports. Webpack generates different code for development and production modes, and webpack users have no idea of how to create unminified production test cases. Reports of "terser doesn't work with webpack" are not helpful and are a waste of everyone's time.

@Reinmar
Copy link

Reinmar commented Nov 13, 2018

This statement says otherwise:

I don't think so, but ok. Point taken.

When uglify-es was no longer maintained no major project, company or organization stepped up to support it.

That's exactly why I started commenting under this thread. I'm following the tales of various minifiers for a couple of years now and I found it sad and wrong that such important, popular, and complex projects get so little help.

The thing is that while you and I know this, I don't think that the community is that much aware of how it works. That their apps and websites rely on a complex project maintained by unpaid volunteers. Therefore, I would love if this problem got more publicity. I chose this thread because I noticed @gaearon, @sophiebits, and @sokra commenting here already. You are switching and advising your users to switch to terser. Wouldn't it be good to raise the awareness about terser's situation?


EDIT: I reported terser/terser#174 – add 👍 to that ticket if you'd like to be able to support terser.

@sokra
Copy link
Member

sokra commented Nov 14, 2018

Since I'm here in the webpack forum, I'd like to request that webpack support a --no-minify option that will work with production mode in order to improve the quality of minify bug reports. Webpack generates different code for development and production modes, and webpack users have no idea of how to create unminified production test cases. Reports of "terser doesn't work with webpack" are not helpful and are a waste of everyone's time.

There is a optimization.minimize: false flag you can set (even in production mode) to disable minimizing.

BTW, before I found this ticket I thought that only webpack 5 will be switching to terser. Now, I understand that webpack 4 will do that too. But when? To be honest, it would be good if there was a bit more transparency in such cases.

Send a PR (cherry-pick #8036 onto master). When merged the next minor version will include it.

@kzc
Copy link

kzc commented Nov 14, 2018

There is a optimization.minimize: false flag you can set (even in production mode) to disable minimizing.

That's difficult for novice users. At the very least it would require them to change their webpack config, and would not work at all without a config:

https://webpack.js.org/api/cli/#usage-without-config-file

Since minification is automatic in production mode it would be better if webpack supported a --no-minify CLI flag override like Parcel does.

@sokra
Copy link
Member

sokra commented Nov 14, 2018

You can propose it to webpack/webpack-cli: --optimization-minimize false or --no-optimization-minimize

@kzc
Copy link

kzc commented Nov 14, 2018

You can propose it to webpack/webpack-cli: --optimization-minimize false or --no-optimization-minimize

@sokra You can do that if you like. I'm not a webpack user. The Terser project just closes issues without unminified JS test cases and will not run third party tools or scripts.

@vkrol
Copy link
Contributor

vkrol commented Nov 18, 2018

Send a PR (cherry-pick #8036 onto master). When merged the next minor version will include it.

Done #8392.

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

9 participants