[internal] Add exports field to packages#41596
Conversation
DiegoAndai
added
the
internal
Netlify deploy previewhttps://deploy-preview-41596--material-ui.netlify.app/ @mui/joy/MenuList: parsed: +0.90% , gzip: +1.02% Bundle size reportDetails of bundle changes (Toolpad) |
scripts/copyFiles.mjs
Outdated
| type: 'array', | ||
| default: [], | ||
| }) | ||
| .option('skipExportsField', { |
There was a problem hiding this comment.
This might be a temporal flag until we close #35233, which we're doing next. Depending on how the icons package is handled it might or might not be necessary.
DiegoAndai
changed the title
| ]); | ||
| } | ||
|
|
||
| if (process.env.MUI_ADD_IMPORT_EXTENSIONS === 'true') { |
There was a problem hiding this comment.
Is there a reason to not always add the extensions?
There was a problem hiding this comment.
The regressions and rollup (umd) builds use this config file as well, and those break if we add the extensions.
When we remove the umd build, I could test if the issue with the regressions build is fixable, and we can remove this check. Does that make sense?
There was a problem hiding this comment.
Ideally al tests use builds that are as close to the real build as possible
There was a problem hiding this comment.
That makes sense, I'll look into adapting the regressions builds as well 👍🏼
There was a problem hiding this comment.
Hey @Janpot, I investigated the issue a bit. The regressions build uses babel.config.js to compile the packages as well as other files, like the docs data files and the regressions tests files. It also points to the src of the packages instead of the build (see the regressions webpack config and the base webpack config).
I was able to use the packages builds instead of the src code by doing
diff --git a/test/regressions/webpack.config.js b/test/regressions/webpack.config.js
index b4472eedd1..c9d02bb705 100644
--- a/test/regressions/webpack.config.js
+++ b/test/regressions/webpack.config.js
@@ -64,6 +64,20 @@ module.exports = {
// Exclude polyfill and treat 'zlib' as an empty module since it is not required. next -> gzip-size relies on it.
zlib: false,
},
+ alias: {
+ ...webpackBaseConfig.resolve.alias,
+ '@mui/material': path.resolve(__dirname, '../../packages/mui-material/build'),
+ '@mui/icons-material': path.resolve(__dirname, '../../packages/mui-icons-material/build/esm'),
+ '@mui/lab': path.resolve(__dirname, '../../packages/mui-lab/build'),
+ '@mui/styled-engine': path.resolve(__dirname, '../../packages/mui-styled-engine/build'),
+ '@mui/styled-engine-sc': path.resolve(__dirname, '../../packages/mui-styled-engine-sc/build'),
+ '@mui/styles': path.resolve(__dirname, '../../packages/mui-styles/build'),
+ '@mui/system': path.resolve(__dirname, '../../packages/mui-system/build'),
+ '@mui/private-theming': path.resolve(__dirname, '../../packages/mui-private-theming/build'),
+ '@mui/base': path.resolve(__dirname, '../../packages/mui-base/build'),
+ '@mui/utils': path.resolve(__dirname, '../../packages/mui-utils/build'),
+ '@mui/joy': path.resolve(__dirname, '../../packages/mui-joy/build'),
+ },
},But even after doing that I still had to filter out some files and not add the file extensions to them:
diff --git a/babel.config.js b/babel.config.js
index 12d09a03c2..2d3ba4a891 100644
--- a/babel.config.js
+++ b/babel.config.js
@@ -103,10 +103,6 @@ module.exports = function getBabelConfig(api) {
]);
}
- if (process.env.MUI_ADD_IMPORT_EXTENSIONS === 'true') {
- plugins.push(['babel-plugin-add-import-extension', { extension: useESModules ? 'mjs' : 'js' }]);
- }
-
return {
assumptions: {
noDocumentAll: true,
@@ -119,6 +115,12 @@ module.exports = function getBabelConfig(api) {
exclude: /\.test\.(js|ts|tsx)$/,
plugins: ['@babel/plugin-transform-react-constant-elements'],
},
+ {
+ exclude: /(\/test\/regressions|packages\/mui-docs|docs)/,
+ plugins: [
+ ['babel-plugin-add-import-extension', { extension: useESModules ? 'mjs' : 'js' }],
+ ],
+ },
],
env: {
coverage: {This is because the files in those folders do not have extensions, as we're not adding them. We could add them, but I think this is outside of this PR's scope.
In conclusion, I would do
- Maintain the
MUI_ADD_IMPORT_EXTENSIONSflag for this PR, adding the extensions only when building usingbuild.mjs - Remove the UMD build in a separate PR
- Create an issue to use the package builds instead of
srcfor the regression tests. In that issue's PR, we could revisit removingMUI_ADD_IMPORT_EXTENSIONSand modifying the regression infrastructure accordingly.
What do you think?
There was a problem hiding this comment.
@Janpot @cherniavskii I'm wondering how these changes will impact X's script: https://github.com/mui/mui-x/blob/next/scripts/copyFiles.mjs.
- We'll have to do
createPackageFile(true)to skip theexportsfield - For the
modulefield.js=>.mjschange, should I add a flag as well?
There was a problem hiding this comment.
hmm, X will have to adopt these changes at well, but likely not before the next major. in the meantime I think we'll need a compatibility mode indeed.
DiegoAndai
added
the
breaking change
|
Hey @Janpot! A couple of things: I added instructions for possible breaking changes of custom configurations:
Is there anything else we should add to that? I'm wondering, do we need to add the "exports": {
"./modern": {
"types": "./modern/index.d.ts",
"default": "./modern/index.mjs",
},
"./modern/*": {
"types": "./modern/*/index.d.ts",
"default": "./modern/*/index.mjs",
},
".": {
"types": "./index.d.ts",
"import": "./index.mjs",
"default": "./node/index.js"
},
"./*": {
"types": "./*/index.d.ts",
"import": "./*/index.mjs",
"default": "./node/*/index.js"
}
}To support https://next.mui.com/material-ui/guides/minimizing-bundle-size/#modern-bundle? We won't need it for the |
I don't think so, this seems clear to me.
If we want to keep supporting the modern bundle it we will likely need to do that. We can add a
This will also need to go in the migration guide then If instead we want to keep supporting the modern bundle through aliasing we will have to add exports for them in the package.json "exports": {
"./modern": {
...
},
"./modern/Button": {
...
}
} But I'd avoid that as per https://next.mui.com/material-ui/guides/minimizing-bundle-size/#how-to-use-custom-bundles |
|
So, if I understand correctly, the options are: 1. Add a "exports": {
".": {
"types": "./index.d.ts",
"modern": "./modern/index.mjs"
"import": "./index.mjs",
"default": "./node/index.js"
},
"./*": {
"types": "./*/index.d.ts",
"modern": "./modern/*/index.mjs",
"import": "./*/index.mjs",
"default": "./node/*/index.js"
}
}
2. Add "exports": {
"./modern": {
"types": "./modern/index.d.ts",
"default": "./modern/index.mjs",
},
"./modern/*": {
"types": "./modern/*/index.d.ts",
"default": "./modern/*/index.mjs",
},
".": {
"types": "./index.d.ts",
"import": "./index.mjs",
"default": "./node/index.js"
},
"./*": {
"types": "./*/index.d.ts",
"import": "./*/index.mjs",
"default": "./node/*/index.js"
}
}
I would opt for 1., I don't think the breaking change is too bad. If we had the |
Janpot
left a comment
Janpot
left a comment
There was a problem hiding this comment.
Reverting my approval to avoid accidental merge before issues have been solved, mainly:
github-actions
bot
added
the
PR: out-of-date
github-actions
bot
removed
the
PR: out-of-date
DiegoAndai
force-pushed
the
add-exports-field
branch
from
217a8bd to
c930e86
Compare
github-actions
bot
added
the
PR: out-of-date
|
To be clear, the compatibility mode is an opt-out, not an opt-in, right? Would it be hard to make this an opt-in instead? That way we loosely couple the migration of X to this system from the merge of this PR. Otherwise this PR seems good. On the changes of the modern bundle I'd like to put this to attention of @michaldudak and @mnajdova to be aware of this and a final blessing. |
|
One concern that I have is that the "modern" condition may be used by another package as well, and since we can't control conditions per import, configuring a bundler to use "modern" will change imports in both MUI and 3rd party packages. Having the condition more specific (like "mui-modern") could reduce the risk of this problem. |
Exactly. We can make it opt-in 👍🏼 I think it will be safer. I'll implement it.
I'll implement this as well. I'll let you know when this changes are ready for review. |
github-actions
bot
removed
the
PR: out-of-date
|
@Janpot ready for review 😊 |
|
@samuelsycamore I added you for copy review of the migration guide and updated bundle instructions. |
|
I'm doing some final testing. Please don't merge yet. |
Janpot
left a comment
Janpot
left a comment
There was a problem hiding this comment.
As per Slack DMs, revoking my review again until we've verified that this works on X/base/Toolpad
DiegoAndai
force-pushed
the
add-exports-field
branch
from
b44c9cb to
7442410
Compare
DiegoAndai
force-pushed
the
add-exports-field
branch
from
ea5bb0e to
fb7a4ff
Compare
|
|
||
| Read more about the `exports` field in the [Node.js documentation](https://nodejs.org/api/packages.html#exports). | ||
|
|
||
| This change limits the exported modules to the root import and one level deep imports. |
There was a problem hiding this comment.
| This change limits the exported modules to the root import and one level deep imports. | |
| This change limits the exported modules to root imports and those that are one level deep. |
| Read more about the `exports` field in the [Node.js documentation](https://nodejs.org/api/packages.html#exports). | ||
|
|
||
| This change limits the exported modules to the root import and one level deep imports. | ||
| If you were importing from deeper levels, you will need to update your imports: |
There was a problem hiding this comment.
| If you were importing from deeper levels, you will need to update your imports: | |
| If you were previously importing from deeper levels, you must update your imports as shown below: |
| ``` | ||
|
|
||
| You might have to update your bundler configuration to support the new structure. | ||
| Following are some common use cases that require changes: |
There was a problem hiding this comment.
| Following are some common use cases that require changes: | |
| Here are some common use cases that require changes: |
|
|
||
| #### Importing CJS | ||
|
|
||
| If you were importing from `/node` as a workaround, this is no longer necessary as the `exports` field maps CJS to the correct files. |
There was a problem hiding this comment.
| If you were importing from `/node` as a workaround, this is no longer necessary as the `exports` field maps CJS to the correct files. | |
| If you were previously importing from `/node` as a workaround, this is no longer necessary because the `exports` field maps CJS to the correct files. |
| #### Using the modern bundle | ||
|
|
||
| The way the modern bundle should be imported has changed. | ||
| Previously, you would alias `@mui/material` to `@mui/material/modern` in your bundler configuration. |
There was a problem hiding this comment.
| Previously, you would alias `@mui/material` to `@mui/material/modern` in your bundler configuration. | |
| Previously you would alias `@mui/material` to `@mui/material/modern` in your bundler configuration. |
|
|
||
| The ESM code, previously under the `esm/` build, has been moved to the root of the package. | ||
| The CommonJS code, previously on the root, has been moved to the `node/` build. | ||
| The `exports` field has been added to the `@mui/system/package.json` file to improve the ESM and CJS builds split: |
There was a problem hiding this comment.
| The `exports` field has been added to the `@mui/system/package.json` file to improve the ESM and CJS builds split: | |
| The `exports` field has been added to the `@mui/system/package.json` file to improve the split between ESM and CJS builds: |
|
|
||
| ### Added exports field to package.json | ||
|
|
||
| The `exports` field has been added to the `@mui/material/package.json` file to improve the ESM and CJS builds split: |
There was a problem hiding this comment.
| The `exports` field has been added to the `@mui/material/package.json` file to improve the ESM and CJS builds split: | |
| The `exports` field has been added to the `@mui/material/package.json` file to improve the split between ESM and CJS builds: |
|
|
||
| Read more about the `exports` field in the [Node.js documentation](https://nodejs.org/api/packages.html#exports). | ||
|
|
||
| This change limits the exported modules to the root import and one level deep imports. |
There was a problem hiding this comment.
same as the suggestions in the other doc above
| ### Modern bundle | ||
|
|
||
| ::: | ||
| The modern bundle targets the latest released versions of evergreen browsers (Chrome, Firefox, Safari, Edge). |
There was a problem hiding this comment.
| The modern bundle targets the latest released versions of evergreen browsers (Chrome, Firefox, Safari, Edge). | |
| The modern bundle targets the latest released versions of the most popular browsers: Chrome, Firefox, Safari, and Edge. |
|
|
||
| ::: | ||
| The modern bundle targets the latest released versions of evergreen browsers (Chrome, Firefox, Safari, Edge). | ||
| This can be used to make separate bundles targeting different browsers. |
There was a problem hiding this comment.
| This can be used to make separate bundles targeting different browsers. | |
| This can be used to create separate bundles that target different browsers. |
github-actions
bot
added
the
PR: out-of-date
|
Closing this PR as per: #30671 (comment). |
|
|
||
| await createModulePackages({ from: srcPath, to: buildPath }); | ||
| await createModulePackages({ from: srcPath, to: buildPath, exportFormat }); |
There was a problem hiding this comment.
@DiegoAndai When you have a moment, you should try whether our problem with @mui/material-nextjs goes away if we remove this line.
| await createModulePackages({ from: srcPath, to: buildPath, exportFormat }); | |
| // await createModulePackages({ from: srcPath, to: buildPath, exportFormat }); |
I had a recent experience in Toolpad where ESM/CJS got mixed up in _document.js which got fixed after I removed the internal package.json files. They become obsolete once we move to the "exports" field and keeping them seems to confuse some bundlers
oliviertassinari
changed the title
5 participants
Add exports field add
.mjsextension to ESM files on the packages that use the common pipelinebuild.mjs/copyFiles.mjsexcept for:@mui/icons-material: Will be handled in [ESM] @mui/icons-material move to pure ESM #35233@mui/codemod: Custom structure@mui/docs: No exports@mui/downloads-tracker: No exportsBesides that, fix/adapt other configurations and imports to this new structure.