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

Regression in 0.18.4 when using static class properties inside of decorators #3230

Closed
mattlewis92 opened this issue Jul 13, 2023 · 3 comments
Labels

Comments

@mattlewis92
Copy link

First off I just wanted to say thank you for esbuild and all the work you do maintaining it, it's made such an enormous improvement to our developer productivity ❤️

When upgrading to 0.18.x we found a bug introduced by #3167 where using static class properties inside of decorators causes errors to be thrown at runtime. Playground link here.

Due to export let DocumentSearchOptions = _DocumentSearchOptions; being set after the call to __decorateClass, it means that DocumentSearchOptions.MaxFiltersSize will throw an error at runtime.

Please let me know if you need any more info!

@evanw
Copy link
Owner

evanw commented Jul 13, 2023

Hmm. It looks like this is a difference between legacy TypeScript decorators and standard JavaScript decorators. Accessing the class name before its declaration is finished works with legacy TypeScript decorators but will no longer work with standard JavaScript decorators (compare this to this). Running this code using standard JavaScript decorators will throw this error, which is effectively what esbuild is doing here:

Uncaught ReferenceError: can't access lexical declaration 'DocumentSearchOptions' before initialization

So it looks like what esbuild is doing here is correct for standard JavaScript decorators. Perhaps esbuild's parser needs to mess with scope only when parsing legacy TypeScript decorators to allow them to access the class object before it has been declared.

@evanw evanw added the classes label Jul 13, 2023
@evanw
Copy link
Owner

evanw commented Jul 22, 2023

This is confusing. I'm trying to figure out whether the evaluation of decorators is supposed to happen inside or outside the class body. It seems like the answer is "it depends on the code being compiled" and I'm not sure what is a bug and what is intended behavior.

You are allowed to call await inside a decorator if the class is declared in an async context, which implies that decorators are evaluated outside the class body:

async function foo() {
  console.log('before')
  class Foo {
    @(await new Promise<PropertyDecorator>(r => setTimeout(() => r(() => {}), 1000)))
    static foo = 'foo'
  }
  console.log('after')
}
foo()

You are also allowed to use a private name inside a decorator, which implies that decorators are evaluated inside the class body:

class Foo {
  static #bar = '#bar'
  @((() => { console.log(Foo.#bar) }) as PropertyDecorator)
  static foo = 'foo'
}

The use of private names appears to have been broken until TypeScript 4.8+ since TypeScript 4.7 and earlier generate nonsense code that references private names outside of the class body. Then TypeScript 4.8+ moved decorator evaluation into the class body, which makes this work.

But attempting to do both of these things causes TypeScript to generate nonsense code even with the latest release, because of course await is not allowed in a class body:

async function foo() {
  console.log('before')
  class Foo {
    static #bar = '#bar'
    @((() => { console.log(Foo.#bar) }) as PropertyDecorator)
    @(await new Promise<PropertyDecorator>(r => setTimeout(() => r(() => {}), 1000)))
    static foo = 'foo'
  }
  console.log('after')
}
foo()

It seems like TypeScript wants decorators to be evaluated both inside and outside the class body. So I guess TypeScript decorators don't really have well-defined semantics? Or at least I can't figure them out.

@evanw
Copy link
Owner

evanw commented Aug 11, 2023

This PR is relevant: microsoft/TypeScript#50074. It introduced the behavior of sometimes putting decorator evaluation inside a static block in the class body. TypeScript's condition for doing this is currently whenever a class member has either a decorator or a parameter decorator that contains an private name (even if the private name is unrelated to the class with the decorator).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants