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

esbuild's default esnext target doesn't include useDefineForClassFields: true, unlike TypeScript #2993

Closed
jpzwarte opened this issue Mar 15, 2023 · 2 comments

Comments

@jpzwarte
Copy link

  --target=...          Environment target (e.g. es2017, chrome58, firefox57,
                        safari11, edge16, node10, ie9, opera45, default esnext)

The above is not true: the default is NOT esnext.

Found this while debugging #2992

@jpzwarte
Copy link
Author

Not sure what actually is the default though.

@evanw
Copy link
Owner

evanw commented Mar 17, 2023

I think an explanation of esbuild's current behavior is too nuanced for the brevity of the help text. Specifically the default target is esnext with the (single?) exception of the useDefineForClassFields TypeScript setting which defaults to esbuild's prediction of what the TypeScript compiler would set it to, which is false unless esbuild finds a tsconfig.json or jsconfig.json file with a target value of "ES2022" or "ESNext" (or of course if useDefineForClassFields is explicitly configured).

The historical reason why esbuild does this is because when esbuild was created, TypeScript always defaulted useDefineForClassFields to false. TypeScript then changed this default behavior in TypeScript version 4.3.2 to be false unless target in tsconfig.json was ESNext. So that's what esbuild implemented. At the time it was unclear why they did this or what the reasoning behind this change was because the change was introduced silently without an announcement. This change introduces subtle correctness bugs into existing TypeScript code that uses class due to how it changes class property initialization, so it seemed like a bad idea to me at the time for esbuild's default behavior to be different than TypeScript's default behavior (since esbuild's default target is esnext while TypeScript's default target was at the time ES3).

As time passed it became clear that (AFAIK) TypeScript's plan is for useDefineForClassFields to default to false for all targets <ES2022 and to true for all targets ≥ES2022 (including ESNext). TypeScript introduced support for the class field syntax before it was standardized and unfortunately the initialization behavior of class fields changed after TypeScript shipped class field support. So TypeScript was stuck having shipped a language feature that didn't work like JavaScript, and then had to change the way TypeScript works to align with JavaScript again. Keying this behavior off of the ES2022 target seems reasonable in some sense since before ES2022 JavaScript didn't have class fields (so TypeScript should implement the legacy behavior for compatibility), but since ES2022 and up JavaScript has class fields (so TypeScript should implement JavaScript's standard behavior).

Still, it's not 100% clear to me that changing esbuild's default for TypeScript's useDefineForClassFields setting to be derived from esbuild's target is the better idea. There's no good answer here since either option will screw someone up. The behavior is very subtle. The way esbuild's target setting works is that each target string is converted into a set of unsupported features. For example, --target=es2021 configures esbuild such that the following JavaScript syntax features are considered to be unsupported (I'm using esbuild's internal names for these features here):

  • arbitrary-module-namespace-names
  • class-field
  • class-private-accessor
  • class-private-brand-check
  • class-private-field
  • class-private-method
  • class-private-static-accessor
  • class-private-static-field
  • class-private-static-method
  • class-static-blocks
  • class-static-field
  • hashbang
  • import-assertions
  • node-colon-prefix-import
  • node-colon-prefix-require
  • regexp-match-indices
  • regexp-set-notation
  • top-level-await

The relevant one for #2992 is class-field. But unlike TypeScript, esbuild also supports other environments such as browsers as targets. Using --target=chrome72 gives basically the same set of unsupported features. Should useDefineForClassFields default to false for --target=chrome72 and below but true for --target=chrome73 and above? I suspect it will really mess some people up when they change esbuild's target setting to adapt their code for older browsers (whether or not they use --target=es2021 or --target=chrome72) and code that uses class silently breaks in subtle and confusing ways. I personally wouldn't expect that to happen, and I'm sure others won't either.

I've been thinking about this for a while and I think I should probably just change esbuild's behavior to try to align to TypeScript's behavior. Probably only if an es... target is explicitly mentioned but not for browser targets. It's going to be really subtle and confusing and break people's code. But this unfortunate behavior comes from TypeScript so in that sense it's not esbuild's problem. This is a breaking change so it'll have to be done in a breaking change release. It probably makes sense to tie this in to some other breaking tsconfig-related changes that I'm planning on doing.

See also #2584.

@evanw evanw changed the title --help is lying esbuild's default esnext target doesn't include useDefineForClassFields: true, unlike TypeScript Mar 17, 2023
@evanw evanw added the tsconfig label Mar 17, 2023
@evanw evanw added the classes label Apr 2, 2023
@evanw evanw closed this as completed in 9be7875 May 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants