rspivak / slimit

SlimIt - a JavaScript minifier/parser in Python
MIT License
551 stars 94 forks source link

slimit removes parentheses from ternary expression, causes syntax error in jQuery #42

Closed hallvors closed 11 years ago

hallvors commented 11 years ago

There is a bug in the Slimit JSParser / serializer, which we hit when trying to convert some files including jQuery. It removes some parentheses it shouldn't remove, causing a syntax error in jquery.js.

e.isPlainObject(d) ? (a = [c.createElement(j[1])], e.fn.attr.call(a, d, !0))

becomes

e.isPlainObject(d) ? a = [c.createElement(j[1])], e.fn.attr.call(a, d, !0)

which breaks the ternary expression.

Probably another variant of https://github.com/rspivak/slimit/issues/21 ?

The full line of code is:

d=d instanceof e?d[0]:d,k=d?d.ownerDocument||d:c,j=m.exec(a),j?e.isPlainObject(d)?(a=[c.createElement(j[1])],e.fn.attr.call(a,d,!0)):a=[k.createElement(j[1])]:(j=e.buildFragment([g[1]],[k]),a=(j.cacheable?e.clone(j.fragment):j.fragment).childNodes);

rspivak commented 11 years ago

Thanks a lot for the bug report. I should be able to look into it in the next couple days.

rspivak commented 11 years ago

What version of Slimit do you use?

hallvors commented 11 years ago

Seems to be slimit 0.7.4

hallvors commented 11 years ago

Another snippet of code broken by the same bug:

null!=this._locales&&(this._defaultLocale=b.default_locale||"en",this._currentLocale=null!=a&&""!=a?a.slice(0,2).toLowerCase():this._defaultLocale,this._messages={},a=this._getLocaleMessages(this._currentLocale),null!=a&&(this._messages[this._currentLocale]=a),this._currentLocale!=this._defaultLocale&&(this._messages[this._defaultLocale]=
this._getLocaleMessages(this._defaultLocale)))

Turns into

null != this._locales && this._defaultLocale = b.default_locale || "en", this._currentLocale = null != a && "" != a ? a.slice(0, 2).toLowerCase() : this._defaultLocale, this._messages = {
    }, a = this._getLocaleMessages(this._currentLocale), null != a && (this._messages[this._currentLocale] = a), this._currentLocale != this._defaultLocale && (this._messages[this._defaultLocale] = this._getLocaleMessages(this._defaultLocale));
hallvors commented 11 years ago

Maybe the simplest test case is

(function(){ /* code */ }());

which (according to a colleague) becomes

function(){ /* code */ }();
miketaylr commented 11 years ago

Maybe the simplest test case is (function(){ /* code */ }());

Yet (function(){ /* code */ })(); is handled just fine.