diff --git a/CHANGELOG.md b/CHANGELOG.md index 4cfefee1ee7..2f41520dd21 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -60,6 +60,33 @@ } ``` +* Avoid referencing `this` from JSX elements in derived class constructors ([#3454](https://github.com/evanw/esbuild/issues/3454)) + + When you enable `--jsx=automatic` and `--jsx-dev`, the JSX transform is supposed to insert `this` as the last argument to the `jsxDEV` function. I'm not sure exactly why this is and I can't find any specification for it, but in any case this causes the generated code to crash when you use a JSX element in a derived class constructor before the call to `super()` as `this` is not allowed to be accessed at that point. For example + + ```js + // Original code + class ChildComponent extends ParentComponent { + constructor() { + super(
) + } + } + + // Problematic output (with --loader=jsx --jsx=automatic --jsx-dev) + import { jsxDEV } from "react/jsx-dev-runtime"; + class ChildComponent extends ParentComponent { + constructor() { + super(/* @__PURE__ */ jsxDEV("div", {}, void 0, false, { + fileName: "", + lineNumber: 3, + columnNumber: 15 + }, this)); // The reference to "this" crashes here + } + } + ``` + + The TypeScript compiler doesn't handle this at all while the Babel compiler just omits `this` for the entire constructor (even after the call to `super()`). There seems to be no specification so I can't be sure that this change doesn't break anything important. But given that Babel is pretty loose with this and TypeScript doesn't handle this at all, I'm guessing this value isn't too important. React's blog post seems to indicate that this value was intended to be used for a React-specific migration warning at some point, so it could even be that this value is irrelevant now. Anyway the crash in this case should now be fixed. + * Allow package subpath imports to map to node built-ins ([#3485](https://github.com/evanw/esbuild/issues/3485)) You are now able to use a [subpath import](https://nodejs.org/api/packages.html#subpath-imports) in your package to resolve to a node built-in module. For example, with a `package.json` file like this: diff --git a/internal/bundler_tests/bundler_default_test.go b/internal/bundler_tests/bundler_default_test.go index 9197129c370..1c3fcd54402 100644 --- a/internal/bundler_tests/bundler_default_test.go +++ b/internal/bundler_tests/bundler_default_test.go @@ -8534,3 +8534,57 @@ func TestDecoratorPrintingCJS(t *testing.T) { `, }) } + +// React's development-mode transform has a special "__self" value that's sort +// of supposed to be set to "this". Except there's no specification for it +// AFAIK and the value of "this" isn't always allowed to be accessed. For +// example, accessing it before "super()" in a constructor call will crash. +// +// From what I understand the React team wanted to have it in case they need it +// for some run-time warnings, but having it be accurate in all cases doesn't +// really matter. For example, I'm not sure if it needs to even be any value in +// particular for top-level JSX elements (top-level "this" can technically be +// the module's exports object, which could materialize a lot of code to +// generate one when bundling, so Facebook probably doesn't want that to +// happen?). +// +// Anyway, this test case documents what esbuild does in case a specification +// is produced in the future and it turns out esbuild should be doing something +// else. +func TestJSXDevSelfEdgeCases(t *testing.T) { + default_suite.expectBundled(t, bundled{ + files: map[string]string{ + "/function-this.jsx": `export function Foo() { return
}`, + "/class-this.jsx": `export class Foo { foo() { return
} }`, + "/normal-constructor.jsx": `export class Foo { constructor() { this.foo =
} }`, + "/derived-constructor.jsx": `export class Foo extends Object { constructor() { super(
); this.foo =
} }`, + "/normal-constructor-arg.jsx": `export class Foo { constructor(foo =
) {} }`, + "/derived-constructor-arg.jsx": `export class Foo extends Object { constructor(foo =
) { super() } }`, + "/normal-constructor-field.tsx": `export class Foo { foo =
}`, + "/derived-constructor-field.tsx": `export class Foo extends Object { foo =
}`, + "/static-field.jsx": `export class Foo { static foo =
}`, + "/top-level-this-esm.jsx": `export let foo =
; if (Foo) { foo = nested top-level this }`, + "/top-level-this-cjs.jsx": `exports.foo =
`, + "/typescript-namespace.tsx": `export namespace Foo { export let foo =
}`, + "/typescript-enum.tsx": `export enum Foo { foo =
}`, + "/tsconfig.json": `{ "compilerOptions": { "useDefineForClassFields": false } }`, + }, + entryPaths: []string{"*"}, + options: config.Options{ + Mode: config.ModeBundle, + AbsOutputDir: "/out", + JSX: config.JSXOptions{ + AutomaticRuntime: true, + Development: true, + }, + UnsupportedJSFeatures: compat.ClassStaticField, + ExternalSettings: config.ExternalSettings{ + PreResolve: config.ExternalMatchers{ + Exact: map[string]bool{ + "react/jsx-dev-runtime": true, + }, + }, + }, + }, + }) +} diff --git a/internal/bundler_tests/snapshots/snapshots_default.txt b/internal/bundler_tests/snapshots/snapshots_default.txt index cd3227f8f4b..f27c81138b3 100644 --- a/internal/bundler_tests/snapshots/snapshots_default.txt +++ b/internal/bundler_tests/snapshots/snapshots_default.txt @@ -2287,6 +2287,226 @@ console.log(/* @__PURE__ */ React.createElement("[", null)); // string-template.jsx console.log(/* @__PURE__ */ React.createElement("]", null)); +================================================================================ +TestJSXDevSelfEdgeCases +---------- /out/class-this.js ---------- +// class-this.jsx +import { jsxDEV } from "react/jsx-dev-runtime"; +var Foo = class { + foo() { + return /* @__PURE__ */ jsxDEV("div", {}, void 0, false, { + fileName: "class-this.jsx", + lineNumber: 1, + columnNumber: 35 + }, this); + } +}; +export { + Foo +}; + +---------- /out/derived-constructor-arg.js ---------- +// derived-constructor-arg.jsx +import { jsxDEV } from "react/jsx-dev-runtime"; +var Foo = class extends Object { + constructor(foo = /* @__PURE__ */ jsxDEV("div", {}, void 0, false, { + fileName: "derived-constructor-arg.jsx", + lineNumber: 1, + columnNumber: 53 + })) { + super(); + } +}; +export { + Foo +}; + +---------- /out/derived-constructor-field.js ---------- +// derived-constructor-field.tsx +import { jsxDEV } from "react/jsx-dev-runtime"; +var Foo = class extends Object { + constructor() { + super(...arguments); + this.foo = /* @__PURE__ */ jsxDEV("div", {}, void 0, false, { + fileName: "derived-constructor-field.tsx", + lineNumber: 1, + columnNumber: 41 + }, this); + } +}; +export { + Foo +}; + +---------- /out/derived-constructor.js ---------- +// derived-constructor.jsx +import { jsxDEV } from "react/jsx-dev-runtime"; +var Foo = class extends Object { + constructor() { + super(/* @__PURE__ */ jsxDEV("div", {}, void 0, false, { + fileName: "derived-constructor.jsx", + lineNumber: 1, + columnNumber: 57 + })); + this.foo = /* @__PURE__ */ jsxDEV("div", {}, void 0, false, { + fileName: "derived-constructor.jsx", + lineNumber: 1, + columnNumber: 77 + }); + } +}; +export { + Foo +}; + +---------- /out/function-this.js ---------- +// function-this.jsx +import { jsxDEV } from "react/jsx-dev-runtime"; +function Foo() { + return /* @__PURE__ */ jsxDEV("div", {}, void 0, false, { + fileName: "function-this.jsx", + lineNumber: 1, + columnNumber: 32 + }, this); +} +export { + Foo +}; + +---------- /out/normal-constructor-arg.js ---------- +// normal-constructor-arg.jsx +import { jsxDEV } from "react/jsx-dev-runtime"; +var Foo = class { + constructor(foo = /* @__PURE__ */ jsxDEV("div", {}, void 0, false, { + fileName: "normal-constructor-arg.jsx", + lineNumber: 1, + columnNumber: 38 + }, this)) { + } +}; +export { + Foo +}; + +---------- /out/normal-constructor-field.js ---------- +// normal-constructor-field.tsx +import { jsxDEV } from "react/jsx-dev-runtime"; +var Foo = class { + constructor() { + this.foo = /* @__PURE__ */ jsxDEV("div", {}, void 0, false, { + fileName: "normal-constructor-field.tsx", + lineNumber: 1, + columnNumber: 26 + }, this); + } +}; +export { + Foo +}; + +---------- /out/normal-constructor.js ---------- +// normal-constructor.jsx +import { jsxDEV } from "react/jsx-dev-runtime"; +var Foo = class { + constructor() { + this.foo = /* @__PURE__ */ jsxDEV("div", {}, void 0, false, { + fileName: "normal-constructor.jsx", + lineNumber: 1, + columnNumber: 47 + }, this); + } +}; +export { + Foo +}; + +---------- /out/static-field.js ---------- +// static-field.jsx +import { jsxDEV } from "react/jsx-dev-runtime"; +var _Foo = class _Foo { +}; +__publicField(_Foo, "foo", /* @__PURE__ */ jsxDEV("div", {}, void 0, false, { + fileName: "static-field.jsx", + lineNumber: 1, + columnNumber: 33 +}, _Foo)); +var Foo = _Foo; +export { + Foo +}; + +---------- /out/top-level-this-cjs.js ---------- +// top-level-this-cjs.jsx +import { jsxDEV } from "react/jsx-dev-runtime"; +var require_top_level_this_cjs = __commonJS({ + "top-level-this-cjs.jsx"(exports) { + exports.foo = /* @__PURE__ */ jsxDEV("div", {}, void 0, false, { + fileName: "top-level-this-cjs.jsx", + lineNumber: 1, + columnNumber: 15 + }); + } +}); +export default require_top_level_this_cjs(); + +---------- /out/top-level-this-esm.js ---------- +// top-level-this-esm.jsx +import { jsxDEV } from "react/jsx-dev-runtime"; +var foo = /* @__PURE__ */ jsxDEV("div", {}, void 0, false, { + fileName: "top-level-this-esm.jsx", + lineNumber: 1, + columnNumber: 18 +}); +if (Foo) { + foo = /* @__PURE__ */ jsxDEV(Foo, { children: "nested top-level this" }, void 0, false, { + fileName: "top-level-this-esm.jsx", + lineNumber: 1, + columnNumber: 43 + }); +} +export { + foo +}; + +---------- /out/tsconfig.js ---------- +// tsconfig.json +var compilerOptions = { useDefineForClassFields: false }; +var tsconfig_default = { compilerOptions }; +export { + compilerOptions, + tsconfig_default as default +}; + +---------- /out/typescript-enum.js ---------- +// typescript-enum.tsx +import { jsxDEV } from "react/jsx-dev-runtime"; +var Foo = /* @__PURE__ */ ((Foo2) => { + Foo2[Foo2["foo"] = /* @__PURE__ */ jsxDEV("div", {}, void 0, false, { + fileName: "typescript-enum.tsx", + lineNumber: 1, + columnNumber: 25 + })] = "foo"; + return Foo2; +})(Foo || {}); +export { + Foo +}; + +---------- /out/typescript-namespace.js ---------- +// typescript-namespace.tsx +import { jsxDEV } from "react/jsx-dev-runtime"; +var Foo; +((Foo2) => { + Foo2.foo = /* @__PURE__ */ jsxDEV("div", {}, void 0, false, { + fileName: "typescript-namespace.tsx", + lineNumber: 1, + columnNumber: 41 + }); +})(Foo || (Foo = {})); +export { + Foo +}; + ================================================================================ TestJSXImportMetaProperty ---------- /out/factory.js ---------- diff --git a/internal/bundler_tests/snapshots/snapshots_tsconfig.txt b/internal/bundler_tests/snapshots/snapshots_tsconfig.txt index 796d7207824..5db4ed23092 100644 --- a/internal/bundler_tests/snapshots/snapshots_tsconfig.txt +++ b/internal/bundler_tests/snapshots/snapshots_tsconfig.txt @@ -664,17 +664,17 @@ console.log(/* @__PURE__ */ jsxDEV(Fragment, { children: [ fileName: "Users/user/project/entry.tsx", lineNumber: 2, columnNumber: 19 - }, this), + }), /* @__PURE__ */ jsxDEV("div", {}, void 0, false, { fileName: "Users/user/project/entry.tsx", lineNumber: 2, columnNumber: 25 - }, this) + }) ] }, void 0, true, { fileName: "Users/user/project/entry.tsx", lineNumber: 2, columnNumber: 17 -}, this)); +})); ================================================================================ TestTsconfigReactJSXWithDevInMainConfig @@ -686,17 +686,17 @@ console.log(/* @__PURE__ */ jsxDEV(Fragment, { children: [ fileName: "Users/user/project/entry.tsx", lineNumber: 2, columnNumber: 19 - }, this), + }), /* @__PURE__ */ jsxDEV("div", {}, void 0, false, { fileName: "Users/user/project/entry.tsx", lineNumber: 2, columnNumber: 25 - }, this) + }) ] }, void 0, true, { fileName: "Users/user/project/entry.tsx", lineNumber: 2, columnNumber: 17 -}, this)); +})); ================================================================================ TestTsconfigRemoveUnusedImports diff --git a/internal/js_parser/js_parser.go b/internal/js_parser/js_parser.go index 0c9f8298b94..fd8f9c1bda8 100644 --- a/internal/js_parser/js_parser.go +++ b/internal/js_parser/js_parser.go @@ -95,6 +95,7 @@ type parser struct { tsEnums map[ast.Ref]map[string]js_ast.TSEnumValue constValues map[ast.Ref]js_ast.ConstValue propMethodValue js_ast.E + propDerivedCtorValue js_ast.E propMethodDecoratorScope *js_ast.Scope // This is the reference to the generated function argument for the namespace, @@ -671,6 +672,7 @@ type fnOrArrowDataVisit struct { isGenerator bool isInsideLoop bool isInsideSwitch bool + isDerivedClassCtor bool isOutsideFnOrArrow bool shouldLowerSuperPropertyAccess bool } @@ -11526,6 +11528,13 @@ func (p *parser) visitClass(nameScopeLoc logger.Loc, class *js_ast.Class, defaul p.nameToKeepIsFor = property.ValueOrNil.Data } + // Propagate whether we're in a derived class constructor + if class.ExtendsOrNil.Data != nil && !property.Flags.Has(js_ast.PropertyIsComputed) { + if str, ok := property.Key.Data.(*js_ast.EString); ok && helpers.UTF16EqualsString(str.Value, "constructor") { + p.propDerivedCtorValue = property.ValueOrNil.Data + } + } + property.ValueOrNil = p.visitExpr(property.ValueOrNil) } @@ -13131,10 +13140,23 @@ func (p *parser) visitExprInOut(expr js_ast.Expr, in exprIn) (js_ast.Expr, exprO }}) // "__self" - if p.fnOrArrowDataParse.isThisDisallowed { - args = append(args, js_ast.Expr{Loc: expr.Loc, Data: js_ast.EUndefinedShared}) - } else { - args = append(args, js_ast.Expr{Loc: expr.Loc, Data: js_ast.EThisShared}) + __self := js_ast.Expr{Loc: expr.Loc, Data: js_ast.EThisShared} + { + if p.fnOnlyDataVisit.shouldReplaceThisWithInnerClassNameRef { + // Substitute "this" if we're inside a static class context + p.recordUsage(*p.fnOnlyDataVisit.innerClassNameRef) + __self.Data = &js_ast.EIdentifier{Ref: *p.fnOnlyDataVisit.innerClassNameRef} + } else if !p.fnOnlyDataVisit.isThisNested && p.options.mode != config.ModePassThrough { + // Replace top-level "this" with "undefined" if there's an output format + __self.Data = js_ast.EUndefinedShared + } else if p.fnOrArrowDataVisit.isDerivedClassCtor { + // We can't use "this" here in case it comes before "super()" + __self.Data = js_ast.EUndefinedShared + } + } + if _, ok := __self.Data.(*js_ast.EUndefined); !ok { + // Omit "__self" entirely if it's undefined + args = append(args, __self) } } @@ -14883,7 +14905,10 @@ func (p *parser) visitExprInOut(expr js_ast.Expr, in exprIn) (js_ast.Expr, exprO nameToKeep = p.nameToKeep } - p.visitFn(&e.Fn, expr.Loc, visitFnOpts{isClassMethod: e == p.propMethodValue}) + p.visitFn(&e.Fn, expr.Loc, visitFnOpts{ + isClassMethod: e == p.propMethodValue, + isDerivedClassCtor: e == p.propDerivedCtorValue, + }) name := e.Fn.Name // Remove unused function names when minifying @@ -16152,7 +16177,8 @@ func (p *parser) handleIdentifier(loc logger.Loc, e *js_ast.EIdentifier, opts id } type visitFnOpts struct { - isClassMethod bool + isClassMethod bool + isDerivedClassCtor bool } func (p *parser) visitFn(fn *js_ast.Fn, scopeLoc logger.Loc, opts visitFnOpts) { @@ -16162,6 +16188,7 @@ func (p *parser) visitFn(fn *js_ast.Fn, scopeLoc logger.Loc, opts visitFnOpts) { p.fnOrArrowDataVisit = fnOrArrowDataVisit{ isAsync: fn.IsAsync, isGenerator: fn.IsGenerator, + isDerivedClassCtor: opts.isDerivedClassCtor, shouldLowerSuperPropertyAccess: fn.IsAsync && p.options.unsupportedJSFeatures.Has(compat.AsyncAwait), } p.fnOnlyDataVisit = fnOnlyDataVisit{