-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
workaround webkit parsing error #2056
Conversation
lib/output.js
Outdated
@@ -597,6 +598,12 @@ function OutputStream(options) { | |||
return true; | |||
} | |||
|
|||
if (output.option('webkit') | |||
&& output.parent(0) instanceof AST_PropAccess | |||
&& output.parent(1) instanceof AST_Assign) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is AST_Assign part of the condition at all?
The following is not handled:
$ echo 'console.log(function(){}.a);' | bin/uglifyjs -c -b webkit,beautify=false
console.log(function(){}.a);
Should be:
if (output.option("webkit")) {
var p = output.parent();
if (p instanceof AST_PropAccess && p.expression === this) return true;
}
An aside: I noticed a few single quotes in recent commits for string literals. Generally the uglify code base uses double quotes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
function(){1+1}.a
works fine under Safari 5 - I even tried (function(){1+1}.a+1);
and other variations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed a few single quotes in recent commits for string literals.
That was a mess-up on my part - I was too busy getting Node.js REPL to list the candidates correctly that I forgot to replace all the single-quotes to double-quotes in the end. Sorry about that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also Safari 5 would be happy with (function(){1}.a=1);
- so you need some form of binary expression in there to trip on this missing parenthesis issue.
But I got lazy with the conditions, because it also trips on things like (function(){for(;1+1;);}.a=1);
(but not (function(){for(;;);}.a=1);
) and doing a TreeWalker
on this seems to be overkill.
Thanks for the reminder about checking AST_PropAccess.expression
- will fix now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
function(){1+1}.a
works fine under Safari 5
...and presumably under phantomjs
.
That's interesting. It's so rare and obscure I wouldn't make the distinction between LHS and RHS - especially in light of the fact that it is behind a "webkit" option.
In any case the patch above does not look at the parent expression
leading to unnecessary parens:
$ echo 'console.log(a[function(){1 + 1}] = 1);' | bin/uglifyjs -b webkit,beautify=false
console.log(a[(function(){1+1})]=1);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for putting up with me. I was just making a case for less code.
You are always welcome 😉
I just need to let some steam out - both me and my box do not thrive under 35℃ & 100% humidity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now I'm getting failure from pdfjs.js in test/jetstream.js - investigating.
If your patch with the AST_Assign condition works - just use it.
Can the webkit
output option be hardcoded in test/jetstream.js
by default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can the
webkit
output option be hardcoded intest/jetstream.js
by default?
I am contemplating between test/mocha/release.js
and test/jetstream.js
Hence the discovery for the current fireworks - and no, AST_Assign
does not help. Though I just found out I forgot to swap phantomjs
from 2.5.0-beta
to 2.1
, so may be there's a new bug?
Rolling back and re-testing as we speak.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, phantomjs 2.5.0-beta
got some new webkit
bug - the current release version works just fine.
2470145
to
168d593
Compare
apply `webkit` to jetstream tests
Updated |
#2053 (comment)