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

transform API downgrades supported features when compared with build API #1423

Closed
melink14 opened this issue Jul 6, 2021 · 2 comments
Closed

Comments

@melink14
Copy link

melink14 commented Jul 6, 2021

I searched a few terms and couldn't find anything about if this is expected so posting an new issue.

Version: 0.12.15

Build API:
npx esbuild extension/options.ts --outdir=dist --target=chrome89

Transform API:
cat extension/options.ts | npx esbuild --loader=ts --target=chrome89 | less

That file has a some class fields like:

class OptionsForm extends LitElement {
  private content: Promise<TemplateResult> = this.fetchAndRender();

When using build API, this becomes the expected:

class OptionsForm extends LitElement {
  content = this.fetchAndRender();

When using the transform API, this gets downgraded:

class OptionsForm extends LitElement {
  constructor() {
    super(...arguments);
    this.content = this.fetchAndRender();
  }

(Actual options.ts is here)

Is this difference expected? I mostly care about it since I usually use snowpack which uses the transform API first before applying optimizations using build API.

@evanw
Copy link
Owner

evanw commented Jul 6, 2021

The difference is probably that the build API is reading tsconfig.json from the file system while the transform API isn't (you have to pass tsconfig.json to the transform API explicitly) and that file is telling esbuild to compile class fields the JavaScript way instead of the TypeScript way.

Here's a demonstration of the build API picking up tsconfig.json (specifically the useDefineForClassFields setting which says to use JavaScript-style class fields instead of TypeScript-style class fields). Each line starting with $ is a terminal command:

$ cat extension/options.ts 
class OptionsForm extends LitElement {
  private content: Promise<TemplateResult> = this.fetchAndRender();
}

$ esbuild extension/options.ts --target=chrome89
class OptionsForm extends LitElement {
  constructor() {
    super(...arguments);
    this.content = this.fetchAndRender();
  }
}

$ echo '{"compilerOptions":{"useDefineForClassFields":true}}' > extension/tsconfig.json

$ esbuild extension/options.ts --target=chrome89
class OptionsForm extends LitElement {
  content = this.fetchAndRender();
}

Here's a demonstration of passing tsconfig.json to the transform API using esbuild's tsconfigRaw setting:

$ cat extension/options.ts | esbuild --loader=ts --target=chrome89
class OptionsForm extends LitElement {
  constructor() {
    super(...arguments);
    this.content = this.fetchAndRender();
  }
}

$ cat extension/options.ts | esbuild --loader=ts --target=chrome89 --tsconfig-raw='{"compilerOptions":{"useDefineForClassFields":true}}'
class OptionsForm extends LitElement {
  content = this.fetchAndRender();
}

Edit: Also this is not "downgrading" since downgrading (i.e. transforming into equivalent syntax for older browsers) would imply that the code before and after are equivalent, which is not the case here. These two different outputs have different semantics. Doing content = ... in the class body is the same as calling Object.defineProperty(this, 'content', ...) which does not evaluate any setters. Doing this.content = ... in the constructor will call the setter called content on the super class if present. These two forms are not semantically equivalent.

The TypeScript compiler's default behavior is to transform the first form to the second which is bad because the behavior is different. But it's been broken for so long that the TypeScript team can't fix it because doing so would break a lot of real-world code. So instead they added the useDefineForClassFields flag to allow people to opt-in to the correct behavior. However the TypeScript compiler still has the incorrect behavior active by default. I have attempted to emulate this behavior from the TypeScript compiler in esbuild's TypeScript-to-JavaScript converter.

@melink14
Copy link
Author

melink14 commented Jul 6, 2021

Thanks for the comment. This makes sense and apologies about the semantic looseness.

I had briefly considered tsconfig as a source of differences but I tested for them poorly and eliminated it as a possibility. (passed --tsconfig= to clear it which doesn't work it seems)

I also eliminated it since I checked by config and it didn't have any options which esbuild was documented to care about, namely useDefineForClassFields in this case. It turns out that recently typescript shipped a change which defaults that option to true if the target option is set to ESNEXT which I had set and was causing the difference as you explained.

(I actually see you mentioned that PR in an old issue but instead of shipping as a breaking change it was stealth released in 4.3.1 it seems)

Closing now since there doesn't seem to be any other action here. Thanks!

@melink14 melink14 closed this as completed Jul 6, 2021
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