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

Dynamic method/property names (static or instance) are treated as a side effect for tree shaking #3189

Closed
pjeby opened this issue Jun 23, 2023 · 9 comments

Comments

@pjeby
Copy link

pjeby commented Jun 23, 2023

See this example:

Try

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

If you replace [use.me] with method, the unused class is omitted from the bundle. The issue affects both static and instance methods and properties.

(I'm guessing this is treating the method name calculation/assignment as if it were top-level module code, even though it's only executed if the class is actually used.)

pjeby added a commit to ophidian-lib/core that referenced this issue Jun 23, 2023
@evanw
Copy link
Owner

evanw commented Jun 23, 2023

Yes, this is true. There are two potential evaluation points for [use.me]: the property access and the ToString operation, either of which could potentially have important side effects.

The analysis to figure out whether an arbitrary JavaScript expression evaluation is side-effect free is complicated and is not something esbuild does. This is not a bug with esbuild. Instead it is out of scope.

Also it’s not true that it’s only evaluated if the class is used. In JavaScript, computed class member names are always evaluated immediately when the class is declared. So top-level code is exactly what the method name calculation/assignment is here.

@pjeby
Copy link
Author

pjeby commented Jun 23, 2023

I mean "used" in the sense of being included in the bundle. If nothing uses the class, then the class need not be declared, so how would there be a side-effect?

@evanw
Copy link
Owner

evanw commented Jun 23, 2023

If nothing uses the class, then the class need not be declared, so how would there be a side-effect?

Here is an example. Try running that code to see what it does.

@pjeby
Copy link
Author

pjeby commented Jun 23, 2023

I understand the effect can occur if the class is used. But if I never reference the class, why would I want that side effect to occur? There doesn't seem to be any way to mark the expression "pure" -- or at least none of the ways I tried worked.

Is there no way to say, "no really, this module / class / package / entire project is intended to be pure and should never bundle things (including lets, vars, and consts) that aren't explicitly referenced somewhere (and re-exporting doesn't count as a reference)?" The library I'm writing falls in its entirety under that umbrella: things not explicitly used via reference from outside the library do not need to be included in bundles built with the library as any side-effects should be suppressed when unreferenced.

@hyrious
Copy link

hyrious commented Jun 23, 2023

But if I never reference the class, why would I want that side effect to occur?

It is almost impossible to guess what side effects are intended to be kept after bundling. For example, those polyfills never gets referenced after imported, but you have to keep them anyway. So to make things at least correct, esbuild does a very careful tree-shaking.

no really, this module / class / package / entire project is intended to be pure and should never bundle things

Yep it does have some ways to do so.

  • You can mark a single object to be pure by using pure annotations. For example:

    export let PerWindowComponent = /*@__PURE__*/ (() => class PerWindowComponent {
      [use.me] () {}
    })();
  • You can mark an import path to be pure by using the sideEffects field in package.json. When no name is references from that file, esbuild would just ignore it as if it was never imported.

You may argue that var a = {}; a.b = 1 is undoubtedly pure. However esbuild doesn't track object properties, so this code will not be shaked by esbuild. If you need this level of tree-shaking, you can use rollup or other bundlers.

@evanw
Copy link
Owner

evanw commented Jun 23, 2023

I'm closing this issue because esbuild is working correctly here. Using a side-effect annotation to indicate that it's ok to ignore the possibility of important side effects is required in cases like this.

@evanw evanw closed this as not planned Won't fix, can't repro, duplicate, stale Jun 23, 2023
@pjeby
Copy link
Author

pjeby commented Jun 23, 2023

Thanks for your patience, and for explaining what's required to use PURE in this case -- I never in a million years would've guessed an IIFE was required to mark a class pure, which explains why none of the many, many ways I tried before opening this issue worked. 😉 Perhaps a future enhancement would be to allow the annotation on the line before an export or class declaration?

I tried using sideEffects: false as well, placing it in every package.json that seemed relevant but could not get it to work. I'm not sure why, since it's working perfectly today. Probably something dumb like not restarting my esbuild watch. Anyway, thanks again for your patience and consideration.

@aleclarson
Copy link

At the very least, computed properties like [Symbol.iterator]() {…} should not prevent a class from being tree-shaked.

@evanw
Copy link
Owner

evanw commented Feb 12, 2024

Yes. That was fixed last year. See #3561.

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

4 participants