Opened 11 years ago

Closed 9 years ago

Last modified 7 years ago

#11673 closed defect (wontfix)

[patch] problem with shrinksafe and recursing using named functions

Reported by: ben hockey Owned by: Richard Backhouse
Priority: low Milestone: tbd
Component: ShrinkSafe Version: 1.5
Keywords: Cc:
Blocked By: Blocking:

Description

shrinksafe has a problem with the following input...

var clone = function recurse(it) {
        var d = dojo,
                prop,
                ret = it && d.isArray(it) ? [] : {};

        if (it && d.isObject(it)) {
                for (prop in it) {
                        if (it.hasOwnProperty(prop)) {
                                ret[prop] = recurse(it[prop]);
                        }
                }
        }
        else {
                ret = it;
        }
        return ret;
};

the output is:

var clone=function recurse(it){
var d=dojo,_1,_2=it&&d.isArray(it)?[]:{};
if(it&&d.isObject(it)){
for(_1 in it){
if(it.hasOwnProperty(_1)){
_2[_1]=_3(it[_1]);
}
}
}else{
_2=it;
}
return _2;
};

the problem is this line _2[_1]=_3(it[_1]); should be _2[_1]=recurse(it[_1]);

Attachments (2)

shrinksafe.named_functions.patch (4.4 KB) - added by Remoun Metyas 11 years ago.
Include named functions in the ShrinkSafe? minifications
shrinksafe.cleanup.patch (3.5 KB) - added by Remoun Metyas 11 years ago.
Fix the ShrinkSafe? build script launching tests; some refactoring/cleanup.

Download all attachments as: .zip

Change History (12)

comment:1 Changed 11 years ago by xMartin

Same problem, one more exampe:

(function myFunction(x){
	x++;
	console.log(x);
	if(x < 10){
		myFunction(x);
	}
})(7);

becomes

(function myFunction(x){
x++;
console.log(x);
if(x<10){
_1(x);
}
})(7);

So you can't use a named function while it is anonymous or has a different name in the containing scope.

In my opinion this is a critical issue as shrinksafe breaks valid JavaScript? code!

comment:2 Changed 11 years ago by xMartin

Seems as if the problem even occurs if you use the same name, like var myFunction = function myFunction(){...

see #8691

Changed 11 years ago by Remoun Metyas

Include named functions in the ShrinkSafe? minifications

comment:3 Changed 11 years ago by Remoun Metyas

I've attached a patch (with test cases) to include named functions in the compression. All (now 13) tests pass.

Changed 11 years ago by Remoun Metyas

Attachment: shrinksafe.cleanup.patch added

Fix the ShrinkSafe? build script launching tests; some refactoring/cleanup.

comment:4 Changed 10 years ago by ben hockey

#8691 is a duplicate of this ticket.

comment:5 Changed 10 years ago by bill

Resolution: patchwelcome
Status: newclosed

I think closure will be the compressor of choice going forwards, and we don't have anyone working on shrinksafe, so closing this ticket.

comment:6 Changed 10 years ago by Remoun Metyas

While closure is now the compressor of choice, it looks like shrinksafe is still available as an option. So why not integrate these patches?

comment:7 Changed 10 years ago by bill

Resolution: patchwelcome
Status: closedreopened
Summary: problem with shrinksafe and recursing using named functions[patch] problem with shrinksafe and recursing using named functions

The patches still apply cleanly but when I try to run the regression tests I get an error:

$ ./runner.sh
js: uncaught JavaScript runtime exception: ReferenceError: "define" is not defined.

... so no idea how to test the patch.

@neonstalwart, you want to take it?

Also @rem do you have a CLA? Not sure of your real name so I can't confirm if you are on the list.

comment:8 Changed 10 years ago by ben hockey

Priority: highlow

@bill i haven't touched java code in over 15 years. i don't think now is a good time to start.

for a while i just worked around this bug and now i've switched completely to closure - dead code removal alone was enough of a reason to switch. i'm not willing to put any time into a tool that i'm not going to use or help support.

comment:9 Changed 9 years ago by bill

Resolution: wontfix
Status: reopenedclosed

Shrinksafe is deprecated in favor of optimization using closure.

comment:10 Changed 7 years ago by ben hockey

#18514 is a duplicate of this ticket.

Note: See TracTickets for help on using tickets.