-
-
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
[code-infra] Update package layout for better ESM support #43264
base: master
Are you sure you want to change the base?
Conversation
Netlify deploy previewhttps://deploy-preview-43264--material-ui.netlify.app/ Menu: parsed: +3.73% , gzip: +3.83% 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.
A couple of questions:
- Why create a subfolder for ESM, instead of maintaining the current convention where the subfolder is for CJS and ESM is at the root?
- Can (should) we create tests for the package layout/build process? To ensure, for example, that the ATTW check is not broken, or that the package layout is not changed unknowingly.
To be able to mark all .js files in esm/ as ESM with a package.json containing
I also fixed up the existing bundler tests and added an extra case for the next.js issue we saw. |
Thanks for replying to my questions @Janpot. Everything looks good from my side. Feel free to mark this as ready for review whenever you're ready. |
@DiegoAndai Only problem left seems to be that the github action that builds |
What we can do is disable this app temporarily in this repo and move it to Pigment where I can take a look later to unblock. |
I think it would be better first to investigate if this is due to the So, there is no rush to take it out now, but let's work together to find the root cause. |
What's the average build time on master/other PRs ? Let me check this locally. |
I don't have the specifics for the The last 5 The last one on this PR was over 2 hours, the one before that was 14 minutes. There's definitely something going on. |
Just checked locally on both master and ^ this branch. |
@Janpot what was the issue? |
It's not solved yet, this action just took 1 hour to run. |
I tried removing pigment from the app, but kept as much as possible of the original pages and it reduces runtime drastically, see https://github.com/mui/material-ui/actions/runs/13055965609/job/36426957328?pr=45152 |
@Janpot which option should we follow:
? I worry that the increase in build time might uncover some edge cases with the new layout we haven't seen. But if you think it's more likely to be caused by Pigment's configuration, then we could unblock this and investigate separately. This would get the layout change to users faster, so we got feedback earlier.
I didn't understand this. |
addresses #44055
addresses #43980
addresses #44265
addresses #44180
addresses #44993
addresses #45018
Might close:
Add a new mode to the build:
Notes:
@mui/joy/node
bundle is included in the client bundle when Joy UI component is being imported into a RSC #37934To Do:
figure out theBlocked on [core] Removeexport *
usage in files that have a'use client'
pragma.'use client'
from index files and useAutocomplete reexport #41956@mui/material-pigment-css
@mui/material-pigment-css
and@mui/icons-material
esmExternals
from the docs (investigate) [docs] Remove esmExternals from the docs #43068Bundle size increase
The bundle size increase can be fully attributed to the inclusion of the following ESM/CJS interop helper in the webpack output:
Code
This is caused by the following import
material-ui/packages/mui-system/src/useMediaQuery/useMediaQuery.ts
Line 75 in eab1b9e
A potential switch to using
use-sync-external-store
has been explored in #43476 but ultimately it makes sense to keep it, it manifests in the webpack runtime.Resolutions
To try out the changes use following resolutions: