Opened 9 years ago

Closed 7 years ago

Last modified 5 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 9 years ago.
Include named functions in the ShrinkSafe? minifications
shrinksafe.cleanup.patch (3.5 KB) - added by Remoun Metyas 9 years ago.
Fix the ShrinkSafe? build script launching tests; some refactoring/cleanup.

Download all attachments as: .zip

Change History (12)

comment:1 Changed 9 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 9 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 9 years ago by Remoun Metyas

Include named functions in the ShrinkSafe? minifications

comment:3 Changed 9 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 9 years ago by Remoun Metyas

Attachment: shrinksafe.cleanup.patch added

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

comment:4 Changed 8 years ago by ben hockey

#8691 is a duplicate of this ticket.

comment:5 Changed 8 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 8 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 8 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 8 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 7 years ago by bill

Resolution: wontfix
Status: reopenedclosed

Shrinksafe is deprecated in favor of optimization using closure.

comment:10 Changed 5 years ago by ben hockey

#18514 is a duplicate of this ticket.

Note: See TracTickets for help on using tickets.