Does not shake unused class with static property

[kurkle]

model.js

export class Test {
    test() {
        console.log('test');
    }
}

Test.id = 'A';

export function foo() {
  console.log('foo');
}

index.js

import {foo} from './model';
foo();

npx esbuild --bundle --format=esm --outfile=bundle.js index.js

expected:

// model.js
function foo() {
  console.log('foo');
}

// index.js
foo();

actual:

// model.js
class Test {
  test() {
    console.log("test");
  }
}
Test.id = "A";
function foo() {
  console.log("foo");
}

// index.js
foo();

[kurkle]

It does shake, if the property is defined inside the class AND --target=esnext:

export class Test {
    static id = 'A';
    test() {
        console.log('test');
    }
}

export function foo() {
  console.log('foo');
}

[evanw]

Thanks for calling out this limitation. This is because esbuild's tree shaking is conservative for correctness and doesn't make the unsafe assumption that Test doesn't have a setter called id that could potentially cause side effects.

I could see making the second case work (using the static syntax and having esbuild do the lowering itself).

[kurkle]

Shouldn't it be possible to know if there is a setter defined? Or could there be an option for "aggressive" tree shaking.
I'm not at all familiar with the code base here, so just guessing.

[kurkle]

Not sure if it makes any difference, but terser did decide to shake these: terser/terser#724
Also rollup is shaking those (and even Test.prototype.id = 'A' case)

[evanw]

Rollup has known tree shaking bugs where code is incorrectly removed that actually does have side effects. See rollup/rollup#2219 for some cases where this happens.

Since JavaScript is a dynamic language, it's impossible even with static analysis to determine whether a statement like that has externally-visible side effects or not. As an example, the code in the original post will have side effects if this code is evaluated before it runs:

Object.defineProperty(Object.prototype, 'id', {
  set() {
    console.log('important side effects');
  }
});

class Test {
  test() {
    console.log("test");
  }
}
Test.id = "A";

Running these two code snippets should print important side effects. It would be a correctness bug if that code was removed due to tree shaking, since that would change the externally-visible side effects. I'd like for esbuild to not contain correctness bugs like this.

I could see potentially adding an opt-in "unsafe mode" that may cause otherwise-valid code to break if it makes use of setters like this. That way by default you can be sure that tree shaking is not going to break your code. This mode would be something to opt-in to only if you have very comprehensive test coverage or you have audited all code in your bundle to make sure it's not going to break any of the assumptions made by the unsafe mode (whatever those end up being).

[bathos]

@evanw I’m not sure if maybe you’re referencing TypeScript semantics here, which may differ, but as the relevant proposals stand right now, a static (or instance) property is initialized with CreateDataPropertyOrThrow, meaning that the setter in the example above would not be invoked — it’s a define-own-property op, not a set op (Object.defineProperty(Test, 'id', {...}) vs Test.id = "A").

See step 34.a. of ClassDefinitionEvaluation as modified by the static class features proposal and step 9 of DefineField as introduced by the class fields proposal.

As the name CreateDataPropertyOrThrow indicates, it may throw, and this is a side effect in its own right (as is the evaluation of the initializer expression potentially), so the conclusion is correct regardless, but illustrating why it can have side effects requires weirder shenanigans than inherited setters, e.g.:

class Foo {
  static bar = (Object.freeze(Foo), true);
}

// “desugaring” to:

class Foo {}
Object.freeze(Foo);
Object.defineProperty(Foo, 'bar', { writable: true, enumerable: true, configurable: true, value: true });

// (where the last line throws a TypeError)

[kurkle]

If aiming for correctness, it should not shake anything, right?

export class Test {
    test() {
        console.log('test');
    }
}

eval("Test.id = 'A'");

export function foo() {
  console.log('foo');
}

import {foo} from './model';
foo();

results to

// model.js
eval("Test.id = 'A'");
function foo() {
  console.log("foo");
}

// index.js
foo();

[fabiosantoscode]

@evanw I'm with you, in principle, regarding avoiding every possible side effect.

However, it's often safe to assume some things. For example, Terser now assumes that a == null is only true when a is null or undefined. You can't turn this off.

And if you're setting a property on a fresh object or class, it's also pretty safe to assume that's not going to have a side effect even though that's technically possible.

[evanw]

If aiming for correctness, it should not shake anything, right?

I'm fixing the bug with direct eval() in the next release. Direct eval() will disable all tree shaking and wrap the module in a CommonJS closure.

[hardfist]

If aiming for correctness, it should not shake anything, right?

I'm fixing the bug with direct eval() in the next release. Direct eval() will disable all tree shaking and wrap the module in a CommonJS closure.

@evanw It seems that direct eval still disable treeshaking in latest version?

[marcofugaro]

Thanks @evanw for adressing this! 🙏

Does this work for the Foo.prototype.bar = ... as well? Libraries such as three.js use this pattern.

Currently it's the only thing preventing three.js from being tree-shaken by esbuild, it would be great if this was looked into.

[evanw]

No that’s something completely different. This change doesn’t do that. Doing that is very different, and also complicated. It requires object shape tracking which is a partial evaluation technique that esbuild doesn’t do.

[tank0317]

@evanw Static property shaking is still not working in typescript，is there any plan to support it ?