Functions are sometimes put inside if blocks, breaking current Firefox, Safari & IE 10

[mgol]

This is related to #179. Current Firefox & Safari as well as IE 10 don't accept function declarations inside and UglifyJS 2.6.2 with compress options:

{
    "hoist_funs": false,
    loops: false,
    unused: false
}

sometimes applies such a treatment.

This caused syntax errors in mentioned browsers on current jQuery master (source from commit 3befe5911a) where we enabled strict mode. More specifically, this function in the built file is transformed to be contained in a negation of this if.

[kzc]

@mgol There are too many external links for me to understand the issue. Can you please post in this ticket (1) a complete reduced test case of the javascript input, and (2) the uglify output with a comment beside the problematic generated code.

[mgol]

Sorry, I should've started with a reduced test case, I just hoped someone will know what's going on.

The test case is as follows - with the source file:

"use strict";

( function() {
    if ( !window ) {
        return;
    }

    function f() {}
    function g() {}
} )();

running:

uglifyjs test-case.js -c hoist_funs=false -c loops=false -c unused=false -m -b

results in:

"use strict";

!function() {
    if (window) {
        function n() {}
        function i() {}
    }
}();

which wasn't allowed in ES5 & many browsers (including current Firefox, Safari & IE 10) throw an immediate Syntax Error when such code is encountered. The original code works fine.

Note that if you remove the second named function declaration (g) from the source, you get this:

"use strict";

!function() {
    if (window) function i() {}
}();

which is a Syntax Error even in current Chrome.

[rvanvelzen]

Is there a specific reason for disabling hoist_funs? At least for now, enabling it should resolve this issue.

E.g., the output with hoist_funs:

"use strict";

!function() {
    function n() {}
    function i() {}
    !window;
}();

... which is functionally the same.

[mgol]

If we don't disable hoist_funs, the full jQuery minified+gzipped build will get larger by 496 bytes. That's a lot from our perspective, we sometimes reject non-critical patches slightly improving some logic if it bumped the size by 100-200 bytes.

[mgol]

For now I resolved it by also disabling if_return but it also increases the size; not by 496 bytes but by 90 but it's still a significant amount.

[mishoo]

I agree it is a bug and it should be fixed.

[rvanvelzen]

I've a potential fix, PR coming up.

[mishoo]

@rvanvelzen Thanks! It looks good to me. Can't do a proper review right now though.

@mgol Can you test with the attached patch?

Any case, this issue makes me think that hoisting is actually the right thing to do—even if it increases the file size. For a library the size of jQuery, 500 more bytes shouldn't be a big deal…

[mgol]

I'll look into it tomorrow.

[mgol]

@mishoo #1053 (comment). The PR LGTM.

[mgol]

Any case, this issue makes me think that hoisting is actually the right thing to do—even if it increases the file size. For a library the size of jQuery, 500 more bytes shouldn't be a big deal…

Full jQuery is currently around 30 KB minified+gzipped. 500 bytes might seem like not a lot but if we didn't care about such changes, landing a couple of them could easily bump the size to 33 KB or more. For a library loaded by half of the internet, that's actually making a big difference.