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

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

Closed
thodnev opened this issue Sep 22, 2024 · 2 comments

Comments

@thodnev
Copy link

thodnev commented Sep 22, 2024

Problem statement

Esbuild uses __knownSymbol in generated code for Symbol lookups, that relies on Symbol.for()

var __knownSymbol = (name, symbol) => (symbol = Symbol[name]) ? symbol : Symbol.for('Symbol.' + name)

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:
if (dispose === void 0) {
dispose = value[__knownSymbol('dispose')]
if (async) inner = dispose
}
if (typeof dispose !== 'function') __typeError('Object not disposable')

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:

  • It does not correspond to the polyfill provided in the official TS documentation:
    Symbol.dispose ??= Symbol("Symbol.dispose");
    (the one provided there simply won't work, as per Symbol spec, the Symbol(key) is guaranteed to return a distinct object every time, even for the same key
  • It is very obscure to find a correct solution, as it requires digging inside toolchain internals
  • Why does the user have to polyfill it himself everywhere, yet more, in his source files

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):

  • Test TS fragment:
     class TestCls implements Disposable {
         [Symbol.dispose]() {
             console.log('I was disposed')
         }
     }
  • Output JS fragment built with esbuild dispose.ts --target=es2022 --outfile=dispose.js
    class TestCls {
      [Symbol.dispose]() {
        console.log("I was disposed");
      }
    }

Please find the TS input and the corresponding Esbuild JS output attached
(built with esbuild dispose.ts --target=es2022 --outfile=dispose.out.js) :
dispose.ts.txt
dispose.out.js.txt

@evanw
Copy link
Owner

evanw commented Sep 23, 2024

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
Copy link
Author

thodnev commented Sep 23, 2024

@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')

peterhirn added a commit to phi-ag/argon2 that referenced this issue Sep 23, 2024
@thodnev thodnev closed this as completed Sep 28, 2024
@thodnev thodnev closed this as not planned Won't fix, can't repro, duplicate, stale Sep 28, 2024
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

2 participants