Opened 12 years ago

Closed 11 years ago

Last modified 11 years ago

#2876 closed defect (fixed)

ShrinkSafe v0.1.0 will break javascript with multiple var inside if-else

Reported by: guest Owned by: alex
Priority: high Milestone: 1.3
Component: ShrinkSafe Version:
Keywords: multiple variable definitions Cc:
Blocked By: Blocking:

Description (last modified by dylan)

shrinksafe (the online version as of april 28,2007 v0.1.0) will break a script if it's got multiple variable definitions for the same variable within an if-else:

function test(bVariable) {
	if (bVariable) {
		var aVariable='ok';
	}else{
		var aVariable='not ok';
	}
	alert(aVariable);
}

in the example above, shrinksafe will not recognize that 'aVariable' is the same variable and will assign two different variable names:

function test(_1){if(_1){var _2="ok";}else{var _3="not ok";}alert(_3);}

Change History (7)

comment:1 Changed 12 years ago by sjmiles

Component: GeneralBuildTools
Owner: changed from anonymous to alex
Priority: highnormal

Fwiw, using using multiple var for one variable name inside a single function is not necessary and generally considered bad form.

I'll let Alex decide if this is 'wontfix' or 'fix', but you can solve your immediate problem by simply removing the extra 'var'. Javascript detects var declarations exclusive of any execution path.

I.e. if (false){var x=3;} will declare 'x' (but does not assign 3 to it).

comment:2 Changed 12 years ago by Adam Peller

Component: BuildToolsShrinkSafe
Keywords: shrinksafe removed

comment:3 Changed 12 years ago by dylan

Milestone: 1.1

This bug gets brought up pretty regularly on the mailing lists and forums. Here's the latest feedback:

I'm not sure where I'm supposed to report bugs re: dojo compressor, so I'll post this here. I've found either a bug in dojo compressor, or a very unfortunate feature.

Given the following: function test() {

if( true ) {

var foo = 1;

} else {

var foo = 2;

}

}

dojo compressor produces: function test(){ if(true){ var _1=1; }else{ var _2=2; } }

Thus, it has completely renamed the variable foo, so that the conditional is no longer setting the same variable. Since javascript does not have block-level scoping determined by curly braces the way perl does, this behavior will very easily break someone's code.

I suppose you can make the argument that I should be declaring variables at the top of my script, or not using the var keyword in the way in which i am, but nonetheless, the compressor took working javascript code and broke it. As a result of this behavior, I opted to use YUI Compressor on the same code and it successfully compressed it without breaking it.

I would love to see you take this advice to heart and make dojo compressor more closely align itself with how javascript scoping actually works, rather than align it with "best practices" or whatever the rationale for the current behavior is.

Thanks, Dan

comment:4 Changed 12 years ago by dylan

Milestone: 1.11.2

moving shrinksafe bugs to 1.2

comment:5 Changed 11 years ago by dylan

Description: modified (diff)
Milestone: 1.2future

moving shrinksafe bugs to future... help wanted.

comment:6 Changed 11 years ago by dante

Milestone: future1.3
Resolution: fixed
Status: newclosed

rhino fixed this:

(function(){
if(true){
var _1="1";
}else{
var _1="2";
}
if(_1){
}
})();

comment:7 Changed 11 years ago by dante

to be clear, the orig was:

(function(){
	
	if(true){
		var longVarName = "1";
	}else{
		var longVarName = "2";
	}
	
	if(longVarName){
		//  foo
	}
	
})();

updating Rhino seems to have fixed a lot of these early shrinksafe bugs.

Note: See TracTickets for help on using tickets.