esbuild icon indicating copy to clipboard operation
esbuild copied to clipboard

Warn if `name` of something is used as a value during minification

Open maheshbansod opened this issue 1 year ago • 1 comments

For a code like: https://esbuild.github.io/try/#dAAwLjIzLjAALS1taW5pZnktaWRlbnRpZmllcnMAewogICAgY2xhc3MgUGVuZ3VpbiB7fQoKICAgIGNvbnN0IGNsYXNzZXMgPSB7UGVuZ3Vpbn07CgogICAgY29uc3QgdyA9IG5ldyBjbGFzc2VzWyJQZW5ndWluIl0oKTsKICAgIGNvbnNvbGUubG9nKHcpOwp9

{
    class Penguin {}

    const classes = {Penguin};

    const w = new classes["Penguin"]();
    console.log(w);
}

It would work fine without minification, but with minification, something like this would in almost all cases fail if ---minify-identifiers is set.

So, a warning in case a name of an identifier (that could change during minification) is used as a value would be useful to probably help someone identify and fix this problem faster in their code.

Not sure if this is the best place for the issue, but I've had this issue due to minification by esbuild through Vite. Please suggest me a better place if there's any. Thanks.

maheshbansod avatar Jul 22 '24 18:07 maheshbansod

You can enable --keep-names if you want to preserve function / class names during minification. It preserves all function / classes, in case you want it to only work for some of your code, you can also do that manually by Object.defineProperty(Penguin, 'name', {value: 'Penguin', configurable: true}).

hyrious avatar Jul 22 '24 21:07 hyrious

I'm not 100% I understand what you're saying because the issue title mentions name but the generated code doesn't use name at all. Regardless, I'm closing this issue because a) there's no problem with the code sample as-is (it runs the same before and after minification) and b) minification of the name property is an expected result of JavaScript minification and not something to warn about in my opinion. Preserving name values is opt-in with the --keep-names setting, as described by the comment above.

evanw avatar Dec 20 '24 01:12 evanw

Upon re-reading my message, I realize it isn't correct, and i hunted down my commit that made me want to create this issue. Let me rephrase and provide a better example.

https://esbuild.github.io/try/#dAAwLjI0LjEALS1taW5pZnktaWRlbnRpZmllcnMAewogICAgY2xhc3MgUGVuZ3VpbiB7CiAgICAgICAgY29uc3RydWN0b3IoKSB7fQogICAgICAgIGJ1aWxkZXIoKSB7CiAgICAgICAgICAgIHJldHVybiB7IFt0aGlzLmNvbnN0cnVjdG9yLm5hbWVdOiBQZW5ndWluIH0KICAgICAgICB9CiAgICAgICAgZmx5KCkgeyByZXR1cm4gIm5vIiB9CiAgICB9CgogICAgY29uc3QgcCA9IG5ldyBQZW5ndWluKCk7CiAgICBjb25zdCBidWlsZGVycyA9IHAuYnVpbGRlcigpOwoKICAgIGNvbnN0IHcgPSBuZXcgYnVpbGRlcnNbIlBlbmd1aW4iXSgpOwogICAgY29uc29sZS5sb2cody5mbHkoKSk7Cn0

In this, clearly, the execution before and after minification are different since this.constructor.name was used here.

{
    class Penguin {
        constructor() {}
        builder() {
            return { [this.constructor.name]: Penguin }
        }
        fly() { return "no" }
    }

    const p = new Penguin();
    const builders = p.builder();

    const w = new builders["Penguin"]();
    console.log(w.fly());
}

The fix @hyrious mentions is indeed valid, but my intention is to highlight that this is something that could be difficult to debug, and a warning or something would certainly help when the constructor.name (or something that's going to be minified) is used as a value.

I hope it's clear now. Apologies for the misunderstanding, and thanks for your time. I'm unsure if this is the place for the issue though - or is it more of a lint thing.

maheshbansod avatar Dec 20 '24 16:12 maheshbansod