douglascrockford / JSMin

JavaScript Minification Filter
http://javascript.crockford.com/jsmin.html
695 stars 151 forks source link

Unary Plus operator (+) to convert boolean to number, preceded by addition operator causes JS syntax error #9

Closed jaydiablo closed 12 years ago

jaydiablo commented 12 years ago

Found this today when trying to minify the d3.js library (http://d3js.org/). Here's one of the two lines in question where the author is using the unary operator to convert a boolean value into a one or zero to be added to another value:

function arc(r, p, a) {
    return "A" + r + "," + r + " 0 " + +(a > Math.PI) + ",1 " + p;
}

JSMin converts this code to:

function arc(r,p,a){return"A"+r+","+r+" 0 "++(a>Math.PI)+",1 "+p;}

Which causes the Javascript syntax error: "SyntaxError: invalid increment operand"

Perhaps not recommended (I see that JSLint complains about the confusing plusses), but possible to make JSMin handle these better? (leave the space?)

Thanks

craigbarnes commented 12 years ago

This is mentioned in the readme:

Use parens with confusing sequences of + or -. For example, minification changes

a + ++b

into

a+++b

which is interpreted as

a++ + b

which is wrong. You can avoid this by using parens:

a + (++b)

JSLint checks for all of these problems. It is suggested that JSLint be used before using JSMin.

You should probably use parens for this case anyway, regardless of minification issues.

jaydiablo commented 12 years ago

If it were my code I would :)

I can try filing a ticket with d3.

They provide a minified version of their library, but obviously don't use JSMin to minify (their minified version doesn't have the syntax issue that a JSMin minified version does).

frankhale commented 12 years ago

Can't you just modify a local copy of d3.js since you know what the issue is?

jaydiablo commented 12 years ago

Sure, but then we have to be sure that change always makes it into any future releases of the d3 library that we use.

Besides, the point of ticket was to improve the compatibility of JSMin with JS libraries in the wild. I knew going into this that the odds of JSMin getting patched were slim (after that whole bootstrap semicolon fiasco), but figured it was worth a shot.

frankhale commented 12 years ago

I'm totally with you on this but the easiest path is just to patch d3.js. That's the way I'd go. Or better yet, fork JSMin and make the change then submit a pull request. I hear you, I am totally with you. Perhaps we can work together to figure out a patch on our own?

douglascrockford commented 12 years ago

Try it now.

jaydiablo commented 12 years ago

Douglas, thanks for the update, much appreciated.