-
-
Notifications
You must be signed in to change notification settings - Fork 32.4k
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
[core] Use React 18's JSX runtime for v5.x UMD builds #44815
Conversation
@Janpot do you think this approach might have some unwanted side effects? I tried to find a way to add a resolution through the rollup configuration but couldn't find any way to do it. |
fbe1dda
to
0535862
Compare
Netlify deploy previewhttps://deploy-preview-44815--material-ui.netlify.app/ Bundle size reportDetails of bundle changes (Toolpad) |
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.
Looks good. We can update it later if @Janpot proposes a better solution.
@aarongarciah I reworked this as @Janpot and I didn't want to commit to the previous approach 😅 I updated the description with the new approach, which is much cleaner. I also attached an |
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.
looks great. small suggestion to avoid reaching in the node_modules folder directly
After #44720, we forgot to keep using React 18 for the UMD builds. React 19 removed their UMD builds, so we need to use React 18 for it.
To fix this, this PR:
react18
dependency to themui-material
package, which is an alias forreact@^18.3.1
jsx/runtime
to thereact18
dependencyTo make this work, I had to update:
rollup-plugin-commonjs
(deprecated) to@rollup/plugin-commonjs
rollup-plugin-node-resolve
(deprecated) to@rollup/plugin-node-resolve
These are the updated packages that are recommended. Without this update, the fix still works, but with the updated packages, replacing
jsx/runtime
is much, much cleaner.This also means that
namedExports
is no longer required, as this has been fixed in@rollup/plugin-commonjs.
I smoke tested with this PR's build and three components:Smoke test: index.html.zip
This PR also fixes the UMD build test.