uglify-es compress conditionals: increment erroneously not reached

[sirreal]

Bug report or feature request?

Bug report

ES5 or ES6+ input?

ES5 input using uglify-es@3.3.9

Uglify version (uglifyjs -V)

uglify-es 3.3.9

JavaScript input

var index = 0;
var whereIsTheLetterC = 0;

[ 'a', 'b', 'c' ].forEach( function( letter ) {
	index++;
	if ( 'c' === letter ) {
		whereIsTheLetterC = index;
	}
} );

console.log( 'This letter "c" is number 3: %d', whereIsTheLetterC );

The uglifyjs CLI command executed or minify() options used.

--compress conditionals=true

The issue is not observed when:

--compress conditionals=false

JavaScript output or error produced.

var index = 0, whereIsTheLetterC = 0;

[ "a", "b", "c" ].forEach(function(letter) {
    "c" === letter && (whereIsTheLetterC = ++index);
}), console.log('This letter "c" is number 3: %d', whereIsTheLetterC);

Detailed description

In the samples above, the input will produce:

This letter "c" is number 3: 3

The uglified (compressed conditionals) version will produce:

This letter "c" is number 3: 1

You can see that in the input, index will always be incremented. However, in the uglified output, index is prefix-incremented only when "c" === letter evaluates to true.

This bug was discovered in Automattic/wp-calypso#21489

The following is the "real-life" code that exposed the bug:

https://github.com/visionmedia/debug/blob/22f993216dcdcee07eb0601ea71a917e4925a30a/src/browser.js#L90-L102

The bug was noticed in the result of applying uglify-es to the above code as part of a Webpack build using https://github.com/webpack-contrib/uglifyjs-webpack-plugin.

Massive thanks to @ockham for help with this issue.

[sirreal]

The repo used to track this down is here: https://github.com/sirreal/debug-debug

It should be simple to checkout, npm install and npm start to see the issue.

[sirreal]

Note that the issue does not manifest with uglify-js@3.3.15. It appears to be uglify-es specific.

[jsnajdr]

Duplicate of #2908, fixed in 0c4f315c026e6, fix released in uglify-js@3.3.11, but never merged to the harmony branch.

#2908 (comment) is interesting:

I currently have no plans to work on harmony - fix is in the latest release of uglify-js.

@alexlamsl Does that mean that uglify-es is unmaintained and we shouldn't use it?

[kzc]

Does that mean that uglify-es is unmaintained and we shouldn't use it?

uglify-es can still be useful even in this state by disabling the options that don't work for a given code base.

You can try another ES6+ minifier, but every alternative has their own problems - whether it's a JVM dependency, configuration complexity, slowness, or bugs of their own. All such projects have the same lack of contributors.

Anyone is free to fork uglify-es, apply patches, fix bugs, and maintain their own copy. People don't appreciate that this is an unpaid volunteer effort that requires a lot of work - and frankly, a lot of grief.

[sirreal]

@kzc I understand that open source work can be thankless. If anything on this issue has caused grief, you have my apologies 🙇

We all greatly appreciate this project and the amount of work that goes into it. It provides a lot of value for a lot of other projects ❤️💚💜💛

Please take the question at face value. If we know that uglify-es won't receive bug fixes then we can consider the appropriate steps to take 🙂

[alexlamsl]

@sirreal I think @kzc has already answered your query, but just to clarify:

• I will only maintain master a.k.a. uglify-js from now on
• I only have commit rights to this respository, which means I cannot grant others commit rights (or rename this repository)

(Note: I am not the only maintainer for this repository, so I can only speak for myself here.)

[sirreal]

Thanks for the information @alexlamsl 👍

[jsnajdr]

@alexlamsl I'd like to better understand the relationship between the master and harmony branches. With my very limited knowledge of the project, I would expect that the harmony branch is the one that receives most attention and is eventually merged into master. It's a superset of the ES5-only master, with added support for ES6+ features like arrows and classes, or isn't it? What is the rationale behind the decision to work only on the ES5 version in master?

How hard would it be to regularly merge master to harmony and keep both packages up to date? Can we offer any help?

[kzc]

@sirreal No worries. No grief in this thread.

@jsnajdr Your understanding is correct. The ES6+ uglify-es harmony branch is a superset of the ES5 uglify-js master branch. I respect that the current uglify maintainer only wants to support ES5 and keep uglify-js ES5 only.

In my opinion the only viable path forward for uglify-es is to fork it, rename it, and find a new home for it: #2949 (comment).

Merging master to harmony is not always straightforward because uglify-es supports additional AST classes and the concept of block scope that does not exist in uglify-js. Each ES5 bug fix or new optimization added to uglify-js has the potential to break ES6+ code once merged to uglify-es. The only way to find out is to try it yourself. Any hypothetical uglify-es fork could cherry pick bug fixes from the uglify-js master branch in order to stabilize it, as well as applying pending harmony ES6+ PRs. Once the uglify-es fork is stabilized it could be independent of the uglify-js project.

[kzc]

@fabiosantoscode Just throwing the idea out there - ignore if you want... since you're the only active harmony PR contributor at the moment and the creator of the harmony branch, any thoughts or care to pick up the uglify-es reigns as maintainer? You could maintain a fork in your repo. Not sure about whether npm publish rights could be granted to you for uglify-es, or whether you'd have to come up with a new package name. I don't have the time or inclination to be a maintainer, but could help with the transition.

[fabiosantoscode]

@kzc I think I could do it, as long as it's in this repository. I think managing a new package name or a new repository is not for me, but here I have more help from you and @alexlamsl.

[kzc]

Fair enough.

@mishoo or @rvanvelzen Could you please grant @fabiosantoscode commit rights to mishoo/UglifyJS2 to maintain uglify-es (a.k.a. the harmony branch)?

@alexlamsl Could you please grant @fabiosantoscode npm publish authority for uglify-es?

[alexlamsl]

@fabiosantoscode as I stated repeatedly, I do not wish to get involved with the harmony branch - and that very much includes the onslaught on issues being filed here when new packages are inadvertently published.

So the repeated suggestion by @kzc to fork that branch would be the best way forward.

[fabiosantoscode]

@alexlamsl got it. Renamed my repository to https://github.com/fabiosantoscode/uglify-es and made the master branch into what the harmony branch is in this repo.

[guybedford]

Reading through this thread, it sounds like a rebranding is necessary for the new project.

@fabiosantoscode if we move forward with your fork, could we perhaps open an issue there to discuss naming further? As for support, if we end up using this fork in Rollup we can discuss what we might be able to do to help support it. Deciding on the organization repo and name to move forward with seems like the first step though towards these goals to me.

[kzc]

@guybedford An npm package exists for the uglify-es fork:

@fabiosantoscode/uglify-es

However, a number of patches have yet to applied. See comments in:

terser/terser@e91a4a3

Should anyone wish to fork uglify-es that would be a good starting point.

[guybedford]

It seems like no single individual wants to take this huge challenge on, but as a community effort it would be good to come up with a memorable name and place it in its own GitHub organization with multiple owners to avoid exactly the problem that has happened here happening again. No need to publicise anything at this point, but at least to provide a home for issues (issue queue not being open on @fabiosantoscode is also an issue...) and community PR contributions.

I was hoping to start with brainstorming a name on @fabiosantoscode's fork in the issue queue there if it can be enabled.

@kzc thanks for the patch list, I'm also happy to contribute where I can on these things, but once we've set up the structures to know patches aren't wasted effort.

[guybedford]

Created terser/terser#2.

[alexlamsl]

Verified to work in uglify-js