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
make collapse_vars
consistent with toplevel
#1608
Conversation
lib/compress.js
Outdated
@@ -558,7 +559,8 @@ merge(Compressor.prototype, { | |||
|
|||
// Only interested in cases with just one reference to the variable. | |||
var def = self.find_variable && self.find_variable(var_name); | |||
if (!def || !def.references || def.references.length !== 1 || var_name == "arguments") { | |||
if (!def || !def.references || def.references.length !== 1 | |||
|| var_name == "arguments" || !toplevel && def.global) { |
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 know this code is correct, but could you please put unnecessary parens around (!toplevel && def.global)
?
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.
Ah, I was wondering who put all those parentheses throughout the codebase 👻
No problem, consider it done 😉
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 just find it easier to read.
I know, I know... Uglify devs should not need any whitespace and unnecessary parens. ;-)
test/compress/collapse_vars.js
Outdated
} | ||
input: { | ||
function foo() { | ||
return 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.
Could you please define function foo
as the following to show that collapse_vars
works in functions regardless of the toplevel setting?
function foo(x){ var y = x; return y; }
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.
Done.
test/compress/collapse_vars.js
Outdated
input: { | ||
function foo() { | ||
return 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.
Could you please define function foo
as the following to show that collapse_vars
works in functions regardless of the toplevel setting?
function foo(x){ var y = x; return y; }
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.
Done.
Thanks! LGTM. |
fixes #1605
/cc @kzc