[icons] Add exports field to package.json #43624
Conversation
Netlify deploy previewhttps://deploy-preview-43624--material-ui.netlify.app/ Bundle size report |
Should we open an issue for those? Add the "breaking change" label, so in the next major, we can easily review those. |
|
@Janpot, it seems like you've covered all bases regarding the risks of releasing this in a minor version 🙌🏼 , so I'm on board with releasing it 👌🏼. Just to make sure, did you test that we won't encounter the same issue that made us revert #26656 in the past? Do our tests cover this case now? Finally, what do you think about waiting a couple of weeks before releasing this? Right now we're receiving a lot of feedback from the v6 stable, so I'm thinking we could hold this until the end of September. This way we avoid overloading the team in case we need to guide people through this change. Does this make sense to you? |
That PR changed the icons package to an ESM-only package which wasn't supported at the time by Next.js. This PR doesn't remove commonjs from the package, even though Next.js now supports ESM-only packages. I've smoke tested this PR against latest stable Next.js and I didn't see problems. The
We have some manual bundling tests to test module resolution, but it looks like they're broken. Will look into them when I can.
It's up to the team. I'm happy to do it either way. |
| "import": "./esm/*.js", | ||
| "require": "./*.js" | ||
| }, | ||
| "./esm/*": "./esm/*.js", |
There was a problem hiding this comment.
Why has this been added? For backward compatibility ?
There was a problem hiding this comment.
Exactly, someone might have added
resolve: {
alias: [
{
find: /^@mui\/icons-material\/(.*)/,
replacement: '@mui/icons-material/esm/$1',
},
],
},to their vite config. It means under the hood their bundler is changing @mui/icons-material/Add to @mui/icons-material/esm/Add before the package.json resolution algorithm has even kicked in. If we don't add an export for this, that module request will follow the ./* => ./esm/*.js export and try to access ./esm/esm/Add.js (* == esm/Add). When we add this export, we special case the ./esm path to not add an extra esm segment (* == Add). We can remove this in a major version.
Same problem for users of https://github.com/avocadowastaken/babel-plugin-direct-import?tab=readme-ov-file#example
edit: you can test this with the pigment vite app. removing the export and putting back the vite config should break its build
|
I'm removing the |
|
It causes this issue with server side rendering: |
Currently esm requests for subpath module specifiers always resolve to the commonjs file. This PR adds an exports field to the package.json to correct the import. We don't change package layout, so anyone using a bundler that doesn't support the exports field should be unaffected.
Adding backwards compatible exports for specific setups we recommended in the past:
Solving this by adding
./esmconditions.Closes #35233
Verified that it fixes the reproduction of #35233 (comment)
May also fully or partly fix:
In the Next major version we can consider following changes:
./esm/*and./esm/*.jsexports., it's a footgun, we could just disallow importing from it altogether. It would reduce package size as well to be able to remove the barrel file. (minus 2.3MB)./utilsOpened a ticket here: #43629