From e4fc1ce57edb081c03a339c51ba5faa0835e3322 Mon Sep 17 00:00:00 2001 From: Evan Wallace Date: Sat, 7 Aug 2021 17:12:07 -0400 Subject: [PATCH] fix #1507: wrong ts class field side effect order --- CHANGELOG.md | 35 +++++++++ internal/bundler/bundler_ts_test.go | 73 +++++++++++++++++++ .../bundler/snapshots/snapshots_default.txt | 58 +++++++++++++++ internal/js_parser/js_parser_lower.go | 4 +- 4 files changed, 168 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 421df731b64..5cb40cc94d3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,41 @@ With this release, esbuild will now generate source maps for CSS output files when `--sourcemap` is enabled. This supports all of the same options as JS source maps including `--sourcemap=inline` and `--sourcemap=external`. In addition, CSS input files with embedded `/*# sourceMappingURL=... */` comments will cause the CSS output file source map to map all the way back to the original inputs. CSS source maps are used by the browser's style inspector to link back to the original source code instead of linking to the bundled source code. +* Fix computed class fields in TypeScript edge case ([#1507](https://github.com/evanw/esbuild/issues/1507)) + + If TypeScript code contains computed class fields, the target environment supports class fields so syntax lowering is not necessary, and TypeScript's `useDefineForClassFields` setting is set to `true`, then esbuild had a bug where the computed property names were computed after the class definition and were undefined. Note that TypeScript's `useDefineForClassFields` setting defaults to `true` if `tsconfig.json` contains `"target": "ESNext"`. + + ```ts + // Original code + class Foo { + [foo] = 1; + @bar [baz] = 2; + } + + // Old output + var _a, _b; + var Foo = class { + [_a] = 1; + [_b] = 2; + }; + _a = foo, _b = baz; + __decorateClass([ + bar + ], Foo.prototype, _b, 2); + + // New output + var _a; + var Foo = class { + [foo] = 1; + [_a = baz] = 2; + }; + __decorateClass([ + bar + ], Foo.prototype, _a, 2); + ``` + + The problem in this case is that normally TypeScript moves class field initializers into the special `constructor` method (automatically generating one if one doesn't already exist) so the side effects for class field property names must happen after the class body. But if class fields are supported by the target environment then the side effects must happen inline instead. + ## 0.12.18 * Allow implicit `./` in CSS `@import` paths ([#1494](https://github.com/evanw/esbuild/pull/1494)) diff --git a/internal/bundler/bundler_ts_test.go b/internal/bundler/bundler_ts_test.go index c5682f78fef..4bb718cdf43 100644 --- a/internal/bundler/bundler_ts_test.go +++ b/internal/bundler/bundler_ts_test.go @@ -1121,3 +1121,76 @@ func TestThisInsideFunctionTSNoBundleUseDefineForClassFields(t *testing.T) { }, }) } + +func TestTSComputedClassFieldUseDefineFalse(t *testing.T) { + default_suite.expectBundled(t, bundled{ + files: map[string]string{ + "/entry.ts": ` + class Foo { + [q]; + [r] = s; + @dec + [x]; + @dec + [y] = z; + } + new Foo() + `, + }, + entryPaths: []string{"/entry.ts"}, + options: config.Options{ + Mode: config.ModePassThrough, + AbsOutputFile: "/out.js", + UseDefineForClassFields: config.False, + }, + }) +} + +func TestTSComputedClassFieldUseDefineTrue(t *testing.T) { + default_suite.expectBundled(t, bundled{ + files: map[string]string{ + "/entry.ts": ` + class Foo { + [q]; + [r] = s; + @dec + [x]; + @dec + [y] = z; + } + new Foo() + `, + }, + entryPaths: []string{"/entry.ts"}, + options: config.Options{ + Mode: config.ModePassThrough, + AbsOutputFile: "/out.js", + UseDefineForClassFields: config.True, + }, + }) +} + +func TestTSComputedClassFieldUseDefineTrueLower(t *testing.T) { + default_suite.expectBundled(t, bundled{ + files: map[string]string{ + "/entry.ts": ` + class Foo { + [q]; + [r] = s; + @dec + [x]; + @dec + [y] = z; + } + new Foo() + `, + }, + entryPaths: []string{"/entry.ts"}, + options: config.Options{ + Mode: config.ModePassThrough, + AbsOutputFile: "/out.js", + UseDefineForClassFields: config.True, + UnsupportedJSFeatures: compat.ClassField, + }, + }) +} diff --git a/internal/bundler/snapshots/snapshots_default.txt b/internal/bundler/snapshots/snapshots_default.txt index 53c57d41049..026e34c8867 100644 --- a/internal/bundler/snapshots/snapshots_default.txt +++ b/internal/bundler/snapshots/snapshots_default.txt @@ -2564,6 +2564,64 @@ switch (bar) { let a; } +================================================================================ +TestTSComputedClassFieldUseDefineFalse +---------- /out.js ---------- +var _a, _b, _c; +class Foo { + constructor() { + this[_a] = s; + this[_c] = z; + } +} +q, _a = r, _b = x, _c = y; +__decorateClass([ + dec +], Foo.prototype, _b, 2); +__decorateClass([ + dec +], Foo.prototype, _c, 2); +new Foo(); + +================================================================================ +TestTSComputedClassFieldUseDefineTrue +---------- /out.js ---------- +var _a, _b; +class Foo { + [q]; + [r] = s; + [_a = x]; + [_b = y] = z; +} +__decorateClass([ + dec +], Foo.prototype, _a, 2); +__decorateClass([ + dec +], Foo.prototype, _b, 2); +new Foo(); + +================================================================================ +TestTSComputedClassFieldUseDefineTrueLower +---------- /out.js ---------- +var _a, _b, _c, _d; +class Foo { + constructor() { + __publicField(this, _a); + __publicField(this, _b, s); + __publicField(this, _c); + __publicField(this, _d, z); + } +} +_a = q, _b = r, _c = x, _d = y; +__decorateClass([ + dec +], Foo.prototype, _c, 2); +__decorateClass([ + dec +], Foo.prototype, _d, 2); +new Foo(); + ================================================================================ TestThisInsideFunction ---------- /out.js ---------- diff --git a/internal/js_parser/js_parser_lower.go b/internal/js_parser/js_parser_lower.go index d430deea8ed..3171d79d449 100644 --- a/internal/js_parser/js_parser_lower.go +++ b/internal/js_parser/js_parser_lower.go @@ -2070,7 +2070,7 @@ func (p *parser) lowerClass(stmt js_ast.Stmt, expr js_ast.Expr, shadowRef js_ast // Make sure the order of computed property keys doesn't change. These // expressions have side effects and must be evaluated in order. keyExprNoSideEffects := prop.Key - if prop.IsComputed && (p.options.ts.Parse || len(prop.TSDecorators) > 0 || + if prop.IsComputed && (len(prop.TSDecorators) > 0 || mustLowerField || computedPropertyCache.Data != nil) { needsKey := true if len(prop.TSDecorators) == 0 && (prop.IsMethod || shouldOmitFieldInitializer) { @@ -2092,7 +2092,7 @@ func (p *parser) lowerClass(stmt js_ast.Stmt, expr js_ast.Expr, shadowRef js_ast // If this is a computed method, the property value will be used // immediately. In this case we inline all computed properties so far to // make sure all computed properties before this one are evaluated first. - if prop.IsMethod { + if !mustLowerField { prop.Key = computedPropertyCache computedPropertyCache = js_ast.Expr{} }