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

Transpile JSX to namespace import in automatic runtime #20113

Closed
billyjanitsch opened this issue Oct 28, 2020 · 2 comments
Closed

Transpile JSX to namespace import in automatic runtime #20113

billyjanitsch opened this issue Oct 28, 2020 · 2 comments

Comments

@billyjanitsch
Copy link
Contributor

As mentioned in the blog post, the current automatic JSX runtime transform transpiles the following code:

<div height={10} />

To roughly:

import {jsx as _jsx} from 'react/jsx-runtime'

_jsx('div', {height: 10})

Unfortunately, the bundling story for this kind of import from a CJS module like 'react/jsx-runtime' isn't great, because Webpack needs to be conservative about inter-op. Webpack 4 ends up bundling it as something like this:

Object(R.jsx)('div', {height: 10})

Webpack 5 does slightly better:

(0, R.jsx)('div', {height: 10})

But these are both pretty unfortunate. The jsx family of functions never uses this so the context escape is unnecessary, but Webpack doesn't know that. This was raised on Twitter here.

However, this can actually be fixed by having the JSX transform create a namespace import instead of a named one. In other words, if it instead transpiled the above code to:

import * as _JSXRuntime from 'react/jsx-runtime'

_JSXRuntime.jsx('div', {height: 10})

Webpack 4 and 5 will both bundle that code to something like:

R.jsx('div', {height: 10})

Which is the desired output. It's smaller and more efficient than either of the alternatives.

(The only improvement from there would be to call jsx directly, but that isn't possible until the runtime is available as an ESM module and Webpack can inline and concatenate it. I won't get into that here, although it would be really nice. 🙂)

I know that this change would get made in the Babel repo rather than here, but I wanted to run it by the React team first. @gaearon and @lunaruan, do you see any reason not to make this change?

@gaearon
Copy link
Collaborator

gaearon commented Nov 10, 2020

Hi! It is intentional that the Babel transform compiles to the named import.

While I get the short-term motivation, in the longer run named imports are better because they are less powerful, which leaves different tools in the middle more opportunities to optimize. E.g. a bundler or a minifier can reason about the function alone, without worrying about the intermediate object (and whether it needs to be created, and then safely eliminated). It is harder to guarantee that the function name can be safely minified if we're using a namespace import.

Indeed, Webpack 4 output is suboptimal but we can live with it because this version won't be popular forever and the community will move on. Webpack 5 output seems mostly reasonable. (It's still not ideal but have there been any perf benchmarks demonstrating significant negative runtime impact?)

In the longer run we'll ship React as ESM and the problem will go away.
Potentially even as early as 18 (but no guarantees at this moment).

Let's not compromise on the long term in favor of short term. If someone would like to fix this in short term, it would be doable with a Babel plugin at the end of the chain — but let's keep the upstream clean.

@gaearon gaearon closed this as completed Nov 10, 2020
@billyjanitsch
Copy link
Contributor Author

Thanks, I appreciate the thoughts and context.

E.g. a bundler or a minifier can reason about the function alone, without worrying about the intermediate object (and whether it needs to be created, and then safely eliminated). It is harder to guarantee that the function name can be safely minified if we're using a namespace import. [...] Let's not compromise on the long term in favor of short term.

Yeah, the idea of not wanting to trade off on the long term makes sense. I'd viewed it as neutral in the long term because Rollup and Webpack 4/5 are quite aggressive about not materializing namespace imports from ESM modules into objects when inlining, so there's generally no property access passed to minifiers. But I see why you'd want to be extra cautious about this and/or allow for a wider range of bundlers, etc.

If someone would like to fix this in short term, it would be doable with a Babel plugin at the end of the chain

That's a great point, thanks. I'll add this to our preset. 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants