-
-
Notifications
You must be signed in to change notification settings - Fork 1.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 #14386
Conversation
Deploy preview: https://deploy-preview-14386--material-ui-x.netlify.app/ |
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
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.
Nice work! 💙
Sorry for a delayed review. 🙈
Does it fix #11016? 🤔
it should yes, though I didn't explicitly try it yet. |
@LukasTy For that we will also need to point its dependencies to the core v7 packages. Will see if I can do that in #16563 next week |
@Janpot This PR seems to cause issues to resolve the I started a reac-router project, and followed the charts installation guidelines. And moving the charts version from
T make it work, I needed to update the material version to |
@alexfauquette I think that is the idea; you need to update both |
Is there a workaround documented somewhere about how to use material v6? |
Oh, shoot, sorry for missing the fact that after the change you can no longer use older |
As I understood, v6 doesn't work with react-router as per mui/material-ui#45281 and other issues. So nothing should be broken that wasn't broken before. There is no workaround, under Node.js ESM the strictest resolution rules apply. Perhaps something is possible with resolve.alias (bit like mui/material-ui#35233 (comment) but different), but really users should just upgrade to Material UI v7, it should be a light migration. |
@alexfauquette I can make it run when adding // ./vite.config.ts
ssr: {
noExternal: [/^@mui\//],
}, Looks like it could be a valid workaround, but I didn't extensively test this. |
Dropping |
@cherniavskii It seems to only be broken for native Node.js ESM module resolution. It looks like bundlers can digest the X@8, core@6 combo fine. e.g. Next.js. The main runtime affected so far seems to be vite with ssr (maybe some testing setups?). This is a relatively narrow use case, and vite is exactly the runtime you want to prioritize moving to Core@7 because Core@6 is utterly broken on vite, it's the reason why we're focussing so much on package layout this release. For vite users who still have good reasons to stay on Core@6, but must use X@8, we have a workaround which comes down to just "don't use native Node.js ESM, just bundle the @mui packages". Reverting in #16871 |
I agree with Jan on this one. 🤔 |
I'm not necessarily against reverting, I understand we want to offer predictability. In any case, I wouldn't go for it if the X team is not fully behind as well. My main reservation would be, are we optimizing for the right user? i.e. do we know about any users that have good reasons not to upgrade core to 7, but must absolutely move X to 8? I have seen a few requests of users that want to move core to 7 and are asking for when the change comes to X as well. Maybe we should verify it on a few more platforms and see if we can come up with a workaround for each failing one. I'm thinking:
For anything vite, they will potentially run in any of the bugs listed in mui/material-ui#43938 |
Yes, it seems to work with the app router. Doesn't work with pages router though: https://stackblitz.com/edit/github-va8ctzeg-t6manklw?file=src%2Fpages%2Findex.js |
Bringing mui/material-ui#43264 to X. This is a breaking change.
Updates to the package layout:
exports
to route bundlers and runtimes to the correct moduleto do