Add jsx-runtime entry for the new automatic JSX transform
#34221
Conversation
kevin940726
added
[Package] Element
|
Size Change: +491 B (0%) Total Size: 1.04 MB
ℹ️ View Unchanged
|
kevin940726
requested review from
ajitbohra,
gziolo,
nerrad and
ntwb
as code owners
|
Not sure if it's safe to update it in RN too. Might want to gather some feedbacks before doing so. |
| @@ -225,7 +225,7 @@ | |||
| "build:packages": "npm run build:package-types && node ./bin/packages/build.js", | |||
| "build:package-types": "node ./bin/packages/validate-typescript-version.js && tsc --build", | |||
| "build:plugin-zip": "./bin/build-plugin-zip.sh", | |||
| "build": "npm run build:packages && wp-scripts build", | |||
| "build": "cross-env NODE_ENV=production npm run build:packages && wp-scripts build", | |||
There was a problem hiding this comment.
I'm not sure why we weren't doing this?
This let Babel knows that we're targeting production builds, and applies some prod-only enhancements. I guess it's because we didn't have any plugins that used this feature?
There was a problem hiding this comment.
I guess this was part of npm run build:packages internally no?
There was a problem hiding this comment.
npm run build:packages is also used by npm run dev, so I think no.
There was a problem hiding this comment.
We use different environments in npm run build:packages:
mainforbuildfoldersmoduleforbuild-modulefolders
I guess the current flow doesn't care whether it's a production or development env. We leave that part to webpack to decide.
As far as I can tell, the Babel preset for WordPress has a special handling only for the test env.
| "@wordpress/browserslist-config": "file:../browserslist-config", | ||
| "@wordpress/element": "file:../element", |
There was a problem hiding this comment.
I don't think we need this as a dependency. Peer dependency, probably, but not a dependency.
There was a problem hiding this comment.
Right, I don't remember why we set them as dependencies. Maybe for developers' convenience because they would need to add @wordpress/element as a dependency when using JSX anyway. I guess it's fine to remove, we just should mention it in the changelog.
| }; | ||
| }, {} ), | ||
| } | ||
| ), |
There was a problem hiding this comment.
Changes here need to be backported to Core's webpack config later as well.
|
I think that my comment #34221 (comment) is still valid. Do we really need to proxy all those JSX helpers through |
There was a problem hiding this comment.
The current Babel setup we have is very complex so we need to ensure that:
- we can use all React development features when running
npm run dev, so packages should be transpiled usingjsx-dev-runtime - we use production version when running
npm run buildthat is used when publishing packages to npm
|
We discussed this PR during the Core JS bi-weekly meeting this Tuesday: https://wordpress.slack.com/archives/C5UNMSU4R/p1635865732011000 I repeated my own concerns shared earlier:
We checked with @Mamaduka a few React component libraries to see how they consume JSX. It looks like all the examples we exercised still import I'm even more inclined to keep all that JSX handling external to gutenberg/lib/client-assets.php Lines 203 to 219 in 098278b It would simplify the configuration in several places:
|
I think gutenberg is unique in a way that it's both a collection of libraries and an app. Each of the packages is a npm package, and it should have This got me thinking that maybe the current way of doing things isn't ideal, we should instead separate apps and libraries from each others and apply different optimizations. But that's for another discussion.
I failed to understand how this would work. I don't think we should just tell the developers to use |
|
I tend to agree that |
|
I've been playing with tools like swc or esbuild and if we ever decide to move away from babel, we'll be facing an issue with the current way we do JSX.
I'm not entirely certain what's the best way forward for libraries using these tools (and there are already a ton). Any thoughts? |
|
The new JSX runtime is fine for classic apps that bundle the React library and the JSX runtime into one webpack bundle, but for our setup where we externalize the While previously we could enqueue one The current PR can successfully externalize What if we did the following. One, ship a new import element from 'react/jsx-runtime';to be externalized to Two, in Third, configure Babel/SWC transpilation so that import { jsxs } from '@wordpress/element';
jsxs( 'div', null, 'hello' );I.e., there is no It seems that the best we can get with Babel/SWC configuration is to insert an import: import { jsxs } from '@wordpress/element/jsx-runtime';because the Regarding SWC: if it can't support some JSX scenario we need, then learning Rust and submitting a patch sounds like a lot of fun, especially over the holidays? 🙂 Regarding libraries: in a few years, there will be probably many more libraries that use the new transform and import from |
|
@jsnajdr very interesting ideas. I'm wondering if that can seamlessly work with the two-step build process that we have:
It means that in the WordPress packages shipped to npm we could have the following references to JSX runtime: import { jsxs } from '@wordpress/element/jsx-runtime';or In the step where webpack comes into play, we can set new externals (for now in Dependency Extraction Webpack Plugin) for I'm also curious how you would structure |
If the It becomes non-elegant only when you start externalizing all these packages and shipping them as
It will be simply a reexport from Yes, there will be actually two JSX runtimes in the bundle: the classic But what if we transpiled JSX with the default config, importing from I see only one reason to not do that: when there is a package that declares a and doesn't declare but a module that's outside but that's not strictly guaranteed. When externalizing to |
gziolo
added
[Status] In Progress
|
Given that it has been quite some time since this pull request received any attention, I'm curious if a decision here was ever reached @gziolo? Being able to use the JSX transform would be a nice convenience in place of importing from |
As far as I remember, @jsnajdr shared some interesting ideas that could work for the project. It's mostly that the PR didn't get any updates since then. There is no need to import |
There's a reason why React decided to ship these bindings from a sub-path (
import { jsxs } from '@wordpress/element';
jsxs( 'div', null, 'hello' );I don't think this is possible from the official plugins in any transpiler. We could write custom plugins for them but it will just make maintenance harder as we'll have to keep it up-to-date whenever there's a change. AFAIK, all projects that leverage custom JSX import use the default setup (
Will applying the externalization rule in the webpack config work? Is there anything I'm missing 🤔 ? As I'll be away for some time, if anyone's interested in this PR, feel free to take it over or start a new PR! |
This is only true if you're using Babel.
Do you have any specific projects in mind? |
|
As an aside, I don't think we can actually externalize On another note, now that Node v14 is the current LTS, should we use a subpath export instead of having separate |
|
Oh, I don't mean actually externalizing |
|
Thanks for that clarification @kevin940726. It doesn't look like this PR aliases |
|
I would need to check with As far as I can tell, all of the other packages use of I think it might work with Webpack's module resolver, but, I'm not sure? I know it works with Webpack 5 because it supports subpath exports, but, I think otherwise it doesn't really matter. I'll put some time this week into digging further into all of this and playing with this PR both as the Gutenberg Plugin and as a consumed package downstream. |
I think so! I haven't tried that though 😅 .
I think ESM is a whole different story though. We still have a long way to go to have full ESM support in our packages. But good point for thinking of this earlier! 👍 |
|
Performance improvements are coming to JSX in React 19. If I understand correctly, the "new" JSX runtime would be more efficient. |
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Merged in #61692 |
Description
Related to #36142.
Part of the React 17 upgrade. Add
jsx-runtimeandjsx-dev-runtimeentries in@wordpress/elementto support the new JSX transform.We can deprecate our own fork of
@wordpress/babel-plugin-import-jsx-pragmaand just use the official@babel/preset-react(which includes@babel/plugin-transform-react-jsx) for JSX transformation. Usingjsx,jsxsandjsxDEVprovides more debugging helpers and could potentially decrease the bundle sizes or speed up performance in the long term. More importantly, it plays more nicely with other libraries and tooling in the React/JS community (For instance, build tooling likeSWCandesbuild, that don't support custom babel plugins).How has this been tested?
Types of changes
New feature
Checklist:
*.native.jsfiles for terms that need renaming or removal).