Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Parameter reassigned before arguments #5939

Open
harai opened this issue Sep 25, 2024 · 8 comments
Open

Parameter reassigned before arguments #5939

harai opened this issue Sep 25, 2024 · 8 comments

Comments

@harai
Copy link

harai commented Sep 25, 2024

Uglify version (uglifyjs -V)

uglify-js 3.19.3

JavaScript input

((e) => {
  const t = e.pushState,
    r = e.replaceState;
  (e.pushState = function (r) {
    const n = new CustomEvent("pushstate", { state: r });
    return window.dispatchEvent(n), t.apply(e, arguments);
  }),
    (e.replaceState = function (t) {
      const n = new CustomEvent("replacestate", { state: t });
      return window.dispatchEvent(n), r.apply(e, arguments);
    }),
    (window.wcNavigation.historyPatched = !0);
})(window.history);

The uglifyjs CLI command executed or minify() options used.

$ uglifyjs work/test.js --compress annotations=false,arguments=false,arrows=false,assignments=false,awaits=false,booleans=false,collapse_vars=false,comparisons=false,conditionals=false,dead_code=false,default_values=false,directives=false,evaluate=false,functions=false,hoist_exports=false,hoist_props=false,if_return=false,imports=false,inline=false,join_vars=false,loops=false,merge_vars=true,negate_iife=false,objects=false,properties=false,pure_getters=false,reduce_funcs=false,reduce_vars=true,rests=false,sequences=false,side_effects=false,spreads=false,strings=false,switches=false,templates=false,typeofs=false,unused=true,varify=true,yields=false --keep-fnames -b --output work/test.min.js

Disable most compress options and enable merge_vars, reduce_vars, unused, and varify.

JavaScript output or error produced.

(e => {
    let t = e.pushState, r = e.replaceState;
    e.pushState = function(r) {
        r = new CustomEvent("pushstate", {
            state: r
        });
        return window.dispatchEvent(r), t.apply(e, arguments);
    }, e.replaceState = function(t) {
        t = new CustomEvent("replacestate", {
            state: t
        });
        return window.dispatchEvent(t), r.apply(e, arguments);
    }, window.wcNavigation.historyPatched = !0;
})(window.history);

t is reassigned just before getting its value with arguments keyword, which results in Error: Failed to execute 'replaceState' on 'History': CustomEvent object could not be cloned. error.

@alexlamsl
Copy link
Collaborator

Thanks for the report − investigating.

@alexlamsl alexlamsl added the bug label Sep 28, 2024
@alexlamsl
Copy link
Collaborator

So it turns out to be a strict-mode bug/feature of Node.js / V8, i.e. if you add "use strict"; to the uglified output above, it will produce the correct output.

Which web browser were you running the output code on?

@alexlamsl
Copy link
Collaborator

For a quick workaround in your use case, add --no-module to the command line:

$ uglifyjs work/test.js --no-module --compress --keep-fnames -b --output work/test.min.js

@alexlamsl alexlamsl removed the bug label Sep 28, 2024
@harai
Copy link
Author

harai commented Sep 29, 2024

@alexlamsl

So it turns out to be a strict-mode bug/feature of Node.js / V8, i.e. if you add "use strict"; to the uglified output above, it will produce the correct output.

This page says the same thing:

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Strict_mode

Which web browser were you running the output code on?

I was using the latest version of Chrome.

--no-module option seems to be used to support old JS runtime.

The code above is part of WooCommerce 9.3.2. Download the zip file and you will find the code in woocommerce/assets/client/admin/navigation/index.js.

The file was automatically re-minified with UglifyJS during optimization process on my server and caused the issue. The real command used for the auto-minification process is:

uglifyjs "$INPUT_JS" --compress --mangle --keep-fnames --source-map "$SOURCEMAP_CONFIG" --output "$OUTPUT_JS"

@alexlamsl
Copy link
Collaborator

--no-module option seems to be used to support old JS runtime.

With latest (& greatest?) ES module "strict" mode is supposed to be implicit:

Also, note that you might get different behavior from sections of script defined inside modules as opposed to in standard scripts. This is because modules use strict mode automatically.

So I guess the reality is that somehow Chrome still runs your code in "sloppy mode", hence the need for --no-module in this case.

@magicds
Copy link

magicds commented Nov 28, 2024

Thank you so much for your hard work and for taking the time to respond! I’ve encountered the same issue and understand that it’s related to the behavior of "use strict". However, I believe this could still be considered a minor flaw.

Here’s the original source code:

window.mini_regClass = window.mini.regClass = function (clazz, className) {
    var args = [clazz];
    var exts = mini.getEpointExt(className);
    if ($.isArray(exts) && exts.length) {
        $.each(exts, function (i, ext) {
            args.push(ext);
        });

        mini.overwrite.apply(mini, args);
    }

    return old_regClass.apply(this, arguments);
};

After:

window.mini_regClass = window.mini.regClass = function(t, e) {
    var i = [t],
       t = mini.getEpointExt(e);
    return n.isArray(t) && t.length && (n.each(t, function(t, e) {
        i.push(e)
    }),
    mini.overwrite.apply(mini, i)),
    s.apply(this, arguments)
}

I’d like to suggest a possible improvement to avoid this behavior. Specifically, local variables could use names that don’t overlap with parameter names.

For example, in version 3.13.5(I'm not sure what the correct last minor version is), the result was correct and looked like this:

window.mini_regClass = window.mini.regClass = function (t, e) {
    var i = [t],
        n = mini.getEpointExt(e);
    return (
        aI.isArray(n) &&
            n.length &&
            (aI.each(n, function (t, e) {
                i.push(e);
            }),
            mini.overwrite.apply(mini, i)),
        bI.apply(this, arguments)
    );
};

@magicds
Copy link

magicds commented Nov 28, 2024

My source code has a long history—maybe two to three times longer than my own work experience—so I’m not entirely sure how "use strict" might affect it.

I trust that uglify-js is continuously improving, and as long as there are no breaking issues, I’m willing to keep updating it within minor versions.

@magicds
Copy link

magicds commented Nov 28, 2024

I didn't notice, maybe I can use {module: false} to avoid it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants