incorrectly minifying q 8.12 library

[CrisFavero]

the following file https://github.com/kriskowal/q/blob/76afd7b7abaad30cbfee394c72227d9dc06154e6/q.js when run through uglify generates a

    function t(n) {
        return "[object StopIteration]" === he(n) || n instanceof se;
    }

which is the equivalent of

function isStopIteration(exception) {
    return (
        object_toString(exception) === "[object StopIteration]" ||
        exception instanceof QReturnValue
    );
}

which is not the correct as the line called by this

if (n = t(n), v(n)) 

should instead evaluate to code produced for the function valueOf;

[mishoo]

I'm sorry, I don't understand this report. Can you be more specific as to what exactly do you find broken?

[SLaks]

Lines 685 - 687 of the original code are:

    object = valueOf(object);
    // assimilate thenables, CommonJS/Promises/A
    if (isPromiseAlike(object)) 

Uglify transforms that to:

if (n = t(n), v(n)) {

The problem is that the call to valueOf() becomes a call to the wrong function (t()):

    function t(n) {
        return "[object StopIteration]" === he(n) || n instanceof se;
    }

t() is actually the mangled name of isStopIteration():

function isStopIteration(exception) {
    return (
        object_toString(exception) === "[object StopIteration]" ||
        exception instanceof QReturnValue
    );
}

It should have emitted a call to l() (which is the actual mangled name of valueOf():

    function l(n) {
        return d(n) ? n.valueOf() : n;
    }

Unchecking the Mangle checkbox in the online demo causes it to emit correct code.

[mishoo]

If you use the following to compress:

u2 q.js -m -o mq.js -b --comments=all -c

(I used --comments to keep comments in the output, and passed -b to make it easier for me to follow the minified/compressed code).

Here's what happens. In the output (mq.js) at line 242, code looks like this:

        }, void 0, function t() {
            return n;
        });

That's the t that your if should refer to. It corresponds to line 738 in the original code:

    }, void 0, function valueOf() {
        return object;
    });

So minification is correct. The original code is wrong, of course, because that's not a function declaration—it's a function expression instead, and so the valueOf name should simply not be visible in that if expression (in compliant engines at least; it will work in IE, but that's because IE is buggy.)

[SLaks]

You're misunderstanding the original code.

The valueOf() function called by line 685 is not the function expression on line 738; it's the function declaration on line 563.

https://github.com/kriskowal/q/blob/76afd7b7abaad30cbfee394c72227d9dc06154e6/q.js#L563-L568

It looks like Uglify is incorrectly making the valueOf() from the function expression visible in its parent scope.

[mishoo]

Ah, I understand.. And you are right — but UglifyJS behaves this way to be compliant with this quirk in Internet Explorer. AFAIK, in IE the function expression on line 738 will take precedence in that scope and override the outer valueOf function.

A simple solution would be for you to rename the function at line 738.

[SLaks]

I was wondering if that was the case, but unchecking IE-proof output doesn't fix it.

I didn't write the original library, and I don't know if it works in IEs that have that bug.

[mishoo]

Or (sorry, I forgot, this was recently added) — another solution would be to pass --screw-ie. Seriously. ;-)

[mishoo]

Ah, IE-proof output is a code generator option that relates to something else. The --screw-ie that I'm talking about is not exposed in the online demo yet, you'd have to use the command-line tool.

[SLaks]

For anyone else having this issue, just use Q v0.9+, which doesn't have the problem in the first place

[lefnire]

changes in 2.3.2 also caused this issue for me (SLak's link HabitRPG/habitica#920). pinning to 2.3.1 fixed

[SLaks]

@lefnire's issue is caused by transforming (function(){})() to !function(){}(), which Racer didn't expect.

Fixed by derbyjs/racer#130

[mishoo]

@SLaks heh, sorry about that. The transformation is valid, so that's why it's doing it unconditionally.

BTW, I see in a comment there “Uglify can't parse a naked function. Executing it allows Uglify to parse it properly”. It's not a parser issue, it's in the JS spec. (“naked” functions must have a name, otherwise it's a syntax error; I suspect that is the problem).

[SLaks]

Uglify is in the right here; you've saved one character.

The fundamental problem there is that Racer wants to uglify an expression (function expression), whereas Uglify is designed to uglify statements (and function declaration statements must have names).

Should Uglify have a function to uglify expressions rather than statements?

[mishoo]

Well, UglifyJS is designed to uglify programs, and a JS program is a sequence of statements.

But yeah, we could have an option to parse expressions.