evanw / esbuild

An extremely fast bundler for the web
https://esbuild.github.io/
MIT License
37.98k stars 1.14k forks source link

Incorrect polyfill __knownSymbol lookups for Symbol.dispose (and probably others) #3920

Open thodnev opened 5 days ago

thodnev commented 5 days ago

Problem statement

Esbuild uses __knownSymbol in generated code for Symbol lookups, that relies on Symbol.for() https://github.com/evanw/esbuild/blob/d34e79e2a998c21bb71d57b92b0017ca11756912/internal/runtime/runtime.go#L78 However, the property dispose does not get set on global Symbol by the generated code. This results in Symbol.dispose resolving to undefined in the generated code, thus making the objects non-disposable. Which further causes the TypeError: Object not disposable runtime error (Firefox 131.0b9 & Chromium 129.0.6668.58 tested, but sure the others will fail as well). Error is produced by this fragment: https://github.com/evanw/esbuild/blob/d34e79e2a998c21bb71d57b92b0017ca11756912/internal/runtime/runtime.go#L510-L514

Temporary (and hackish) solutions right now

One possible solution will be to polyfill the Symbol.dispose in TS ourselves, like the following:

Symbol.dispose ??= Symbol.for('Symbol.dispose')

But:

Proper fix proposal

Add to the generated code a procedure to test&set dispose property on Symbol object, something like:

if (Symbol.dispose === undefined) {
    Object.defineProperty(Symbol, 'dispose', {value: Symbol.for('Symbol.dispose')})
}

Examples

Generated code (as of now):

evanw commented 5 days ago

Sorry, I don't understand. You need to polyfill Symbol.dispose before you use it with esbuild, just like all other new APIs. This is because esbuild transforms newer syntax to older syntax for you but does not polyfill APIs for you. And polyfilling Symbol.dispose should already work fine. You can do it however you like. For example, you can do it exactly as TypeScript's documentation tells you to do it (as you pointed out):

Symbol.dispose ??= Symbol("Symbol.dispose");

function demo() {
  class Foo {
    constructor() { console.log('constructor') }
    [Symbol.dispose]() { console.log('dispose') }
  }

  using foo = new Foo
}

demo()

If you build this with esbuild and run it, it will output the following:

constructor
dispose

So everything already appears to be working correctly. This happens because __knownSymbol just returns the value of Symbol[name] if it's present. If you polyfill Symbol.dispose before using it, then Symbol['dispose'] will be your polyfill. The stuff with Symbol.for is irrelevant for your use case. It's only there for obscure compatibility reasons with code generated by Babel.

You have to polyfill Symbol.dispose with esbuild just like you have to do with TypeScript. This is already in esbuild's documentation:

Note that this is only concerned with syntax features, not APIs. It does not automatically add polyfills for new APIs that are not used by these environments. You will have to explicitly import polyfills for the APIs you need (e.g. by importing core-js). Automatic polyfill injection is outside of esbuild's scope.

thodnev commented 4 days ago

@evanw Thank you for the clarification. In the case the user has not provided his own polyfill, maybe there should be a hint in the error message. This would largely reduce uncertainty for the user. Now the error message states TypeError: Object not disposable. Which leads to doubts that probably there is something wrong with the object itself. What about TypeError: Object not disposable or Symbols.dispose not provided?

UPD: Tested it with the core-js/modules/esnext.symbol.dispose polyfill, and it works as well. But core-js introduced a lot of code into the bundle (+8.1 KiB pure, +3 KiB gzipped, +2.7 KiB brotli). So for the sole purpose of having this feature, I'll probably stick with a simpler TS-native polyfill

Symbol.dispose ??= Symbol.for('Symbol.dispose')