-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
fix #334: support automatic JSX runtime #2349
Conversation
I should note that I tested this successfully on a large internal codebase that uses the automatic runtime. |
Heads up that I'm currently traveling. Given that this is a big PR, I will not be able to look at this until I get back. |
@evanw Thanks for the heads up — safe travels. I thought about splitting the work into smaller chunks, but I couldn't think of a natural way of doing it. If there's anything I can do to make reviewing this easier when you get back, please let me know. |
This will be a huge improvement. I have always felt squishy about the inject and react-shim. I am using it on a large project that uses mui and the jsx from @emotion/react.
|
Thank you so much for this! 🙏 |
@evanw want to bump this PR, just in case you may be back from traveling! 🗺️ |
# Conflicts: # internal/js_parser/js_parser.go
internal/js_parser/js_parser.go
Outdated
p.recordUsage(ref) | ||
return p.handleIdentifier(loc, &js_ast.EIdentifier{Ref: ref}, identifierOpts{ | ||
wasOriginallyIdentifier: true, | ||
matchAgainstDefines: true, // Allow defines to rewrite imported JSX symbols |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added this after resolving conflicts because it seems like if we support rewriting Fragment and createElement, we should support rewriting jsx, jsxs, and jsxDEV.
Didn't get to this for this release cycle, sorry. Hoping to look at this soon (this week). Don't worry about resolving merge conflicts. I can do that when I process this PR. |
* Improve line/column performance by not repeatedly scanning backward * Column numbers are supposed to be 1-based to match Babel, not 0-based * Column numbers should be in UTF-16 code units to match Babel, not bytes
The define feature only applies to globals and "automatic" JSX symbols are never globals, so this never did anything.
I find it confusing to have both `--jsx` and `--jsx-runtime` since the underlying state is really only one of three settings. This merges both of them, which is similar to what TypeScript does. I have kept the `--jsx-development` option separate because the desire is to be able to override TypeScript's settings. But I have shortened it to `--jsx-dev` to make specifying it on the command line easier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have gone through all of the code in this PR and made various changes. Most of them are minor bug fixes or tweaks. I will also take care of updating the documentation about all of this on esbuild's website.
One bigger change to call out is the removal of the --jsx-runtime
setting. I merged it into the existing --jsx
option, which was changed into a three-state value. This is similar to how TypeScript specifies this, and seemed simpler conceptually. As a bonus, it's also shorter to specify it on the command line. So the --jsx
setting now has three values:
--jsx=transform
(the default)--jsx=preserve
--jsx=automatic
I also wanted to say that this PR was really well done! It's obvious that a lot of thought and care went into this. I really appreciate your clear documentation about the changes in the PR description, the clean and straightforward implementation, and all of the added tests.
Yup - great to see this! #312 next? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will also take care of updating the documentation about all of this on esbuild's website.
Thank you 🙇
One bigger change to call out is the removal of the
--jsx-runtime
setting.
This makes complete sense. I was trying to avoid changes to existing features/flags as much as possible, but this is clearly better.
I also wanted to say that this PR was really well done! It's obvious that a lot of thought and care went into this. I really appreciate your clear documentation about the changes in the PR description, the clean and straightforward implementation, and all of the added tests.
Thank you for being receptive to this change and for taking the time to review this PR so thoroughly. And, more broadly, thank you for esbuild itself — it is a significant achievement in web tooling. I've learned as much about the web platform just from your release notes as I have from any other source.
|
||
expectParseErrorJSXAutomatic(t, d, "<a key/>", "<stdin>: ERROR: Please provide an explicit key value. Using \"key\" as a shorthand for \"key={true}\" is not allowed.\n<stdin>: NOTE: The property \"key\" was defined here:\n") | ||
expectParseErrorJSXAutomatic(t, d, "<div __self={self} />", "<stdin>: ERROR: Duplicate \"__self\" prop found. Both __source and __self are set automatically by esbuild. These may have been set automatically by a plugin.\n<stdin>: NOTE: The property \"__self\" was defined here:\n") | ||
expectParseErrorJSXAutomatic(t, d, "<div __source=\"/path/to/source.jsx\" />", "<stdin>: ERROR: Duplicate \"__source\" prop found. Both __source and __self are set automatically by esbuild. These may have been set automatically by a plugin.\n<stdin>: NOTE: The property \"__source\" was defined here:\n") | ||
|
||
// Line/column offset tests. Unlike Babel, TypeScript sometimes points to a | ||
// location other than the start of the element. I'm not sure if that's a bug | ||
// or not, but it seems weird. So I decided to match Babel instead. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
// some buggy JavaScript toolchains such as the TypeScript compiler convert | ||
// ESM into CommonJS by replacing "import" statements inline without doing | ||
// any hoisting, which is incorrect. See the following issue for more info: | ||
// https://github.com/microsoft/TypeScript/issues/16166. Since JSX-related |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh wow, I didn't know about this and didn't consider that esbuild output would be used as input to tsc
. Thank you for catching this.
internal/js_parser/js_parser.go
Outdated
@@ -1573,7 +1573,6 @@ func (p *parser) importJSXSymbol(loc logger.Loc, jsx JSXImport) js_ast.Expr { | |||
p.recordUsage(ref) | |||
return p.handleIdentifier(loc, &js_ast.EIdentifier{Ref: ref}, identifierOpts{ | |||
wasOriginallyIdentifier: true, | |||
matchAgainstDefines: true, // Allow defines to rewrite imported JSX symbols |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, I was clearly missing context here.
It’s marvelous. I set it to automatic and it did everything perfectly even with jsxImportSource to emotion/ MUI.
… On Jul 28, 2022, at 11:20 AM, Evan Wallace ***@***.***> wrote:
Merged #2349 into master.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you commented.
|
Reduces the amount of boilerplate to render the UI and makes it easier to respond to state changes (e.g. machine getting authorized, netmap changing, etc.) Preact adds ~13K to our bundle size (5K after Brotli) thus is a neglibible size contribution. We mitigate the delay in rendering the UI by having a static placeholder in the HTML. Required bumping the esbuild version to pick up evanw/esbuild#2349, which makes it easier to support Preact's JSX code generation. Fixes #5137 Fixes #5273 Signed-off-by: Mihai Parparita <mihai@tailscale.com>
Reduces the amount of boilerplate to render the UI and makes it easier to respond to state changes (e.g. machine getting authorized, netmap changing, etc.) Preact adds ~13K to our bundle size (5K after Brotli) thus is a neglibible size contribution. We mitigate the delay in rendering the UI by having a static placeholder in the HTML. Required bumping the esbuild version to pick up evanw/esbuild#2349, which makes it easier to support Preact's JSX code generation. Fixes #5137 Fixes #5273 Signed-off-by: Mihai Parparita <mihai@tailscale.com>
fixes #334
fixes #718
fixes #1172
fixes #2318
This adds support for the new (automatic) JSX runtime to esbuild for both the build and transform APIs.
New CLI flags and API options:
--jsx-runtime
,jsxRuntime
—"automatic"
or"classic"
(default)--jsx-development
,jsxDevelopment
— toggles development mode for the automatic runtime--jsx-import-source
,jsxImportSource
— Overrides the root import for runtime functions (default"react"
)New JSX pragmas:
@jsx
— sets the runtime ("automatic"
or"classic"
)@jsxImportSource
— sets the import source (only valid with automatic runtime)@jsxFragment
and@jsxFactory
are only valid with classic runtime.Implementation details:
Most of the work to support the automatic runtime happens in the second parsing pass. Here is a high level overview of the logic when
automatic
runtime is enabled:Fragment
needs to be imported (from the appropriate runtime import)key
prop and extract it from the props argument, since it's now passed directly to a jsx functionjsxDEV
— development mode (from{importSource}/jsx-dev-runtime
)jsxs
— production mode, static children (from{importSource}/jsx-runtime
)jsx
— production mode, non-static children (from{importSource}/jsx-runtime
)createElement
— fallback mode* (from{importSource}
)At the end of parsing, if any JSX symbols had recorded usage, the appropriate import statement is generated for only those symbols that were actually used within the file. These are generated as external imports, unlike esbuild's runtime.
* Fallback mode — If a component includes spread props followed by a
key
prop,{...props} key={1}
, Babel triggers a fallback mode tocreateElement
. Apparently this is a temporary special case that will be removed once spreading "key" from props is no longer supported. It looks like the TypeScript compiler also implements this special case, so it seemed prudent to be compatible here.Development mode:
Aside from importing a different function from a different source (
{ jsxDEV } from 'react/jsx-dev-runtime'
), development mode also passes more arguments, one of which is the "source" argument. This contains the current filename, line number, and column number mapping to the original source location of the element.I used the LineColumnTracker to extract those details from the element's location, which will probably incur a minor performance cost that should be documented for the
jsxDevelopment
option. The alternative was to simply omit those details, but that would defeat the purpose of supporting development mode.TSConfig resolving:
Along with accepting the new options directly via CLI or API, I implemented option inference from tsconfig compiler options.
"jsx": "preserve"
or"jsx": "react-native"
→ preserve"jsx": "react"
→ classic runtime"jsx": "react-jsx"
→ automatic runtime"jsx": "react-jsxdev"
→ automatic runtime and development mode enabledIt also reads the value of
"jsxImportSource"
from tsconfig if specified.For
react-jsx
it's important to note that it doesn't implicitly setjsxDevelopment
to false. This is to support the case where a user setsreact-jsx
in their tsconfig but then toggles development mode directly in esbuild, e.g., by environment variable or some other means.esbuild vs Babel vs TS vs...
There are a few differences between the various technologies that implement automatic JSX runtimes. Though esbuild generally follows TypeScript's behavior when there is a disagreement, I chose to follow Babel in cases where it might help the user catch invalid or undesirable behavior.
Here are the notable differences:
Element has
__source
or__self
props:Element has an "implicit true" key prop, e.g.
<a key />
:Element has spread children, e.g.
<a>{...children}</a>