Skip to content

Commit

Permalink
fix #3454: crash with jsx-dev before super() call
Browse files Browse the repository at this point in the history
  • Loading branch information
@evanw
evanw committed Nov 19, 2023
1 parent 4a1e576 commit 6c4aa2c
Show file tree
Hide file tree
Showing 5 changed files with 340 additions and 12 deletions.
27 changes: 27 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(<div />)
}
}

// 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: "<stdin>",
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:
Expand Down
54 changes: 54 additions & 0 deletions internal/bundler_tests/bundler_default_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 <div/> }`,
"/class-this.jsx": `export class Foo { foo() { return <div/> } }`,
"/normal-constructor.jsx": `export class Foo { constructor() { this.foo = <div/> } }`,
"/derived-constructor.jsx": `export class Foo extends Object { constructor() { super(<div/>); this.foo = <div/> } }`,
"/normal-constructor-arg.jsx": `export class Foo { constructor(foo = <div/>) {} }`,
"/derived-constructor-arg.jsx": `export class Foo extends Object { constructor(foo = <div/>) { super() } }`,
"/normal-constructor-field.tsx": `export class Foo { foo = <div/> }`,
"/derived-constructor-field.tsx": `export class Foo extends Object { foo = <div/> }`,
"/static-field.jsx": `export class Foo { static foo = <div/> }`,
"/top-level-this-esm.jsx": `export let foo = <div/>; if (Foo) { foo = <Foo>nested top-level this</Foo> }`,
"/top-level-this-cjs.jsx": `exports.foo = <div/>`,
"/typescript-namespace.tsx": `export namespace Foo { export let foo = <div/> }`,
"/typescript-enum.tsx": `export enum Foo { foo = <div/> }`,
"/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,
},
},
},
},
})
}
220 changes: 220 additions & 0 deletions internal/bundler_tests/snapshots/snapshots_default.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 ----------
Expand Down
12 changes: 6 additions & 6 deletions internal/bundler_tests/snapshots/snapshots_tsconfig.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down

0 comments on commit 6c4aa2c

Please sign in to comment.