-
-
Notifications
You must be signed in to change notification settings - Fork 32.3k
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
[docs] Modularize Imports for Nextjs #35457
Conversation
mui#35450 Signed-off-by: Ahmed Hany <[email protected]>
|
Gave this a quick try on Toolpad and this breaks on imports like: import { Box, styled } from '@mui/material' As there is no |
@Janpot
|
Could you add it as Option 3, after a Babel plugin? |
docs/data/material/guides/minimizing-bundle-size/minimizing-bundle-size.md
Outdated
Show resolved
Hide resolved
…ndle-size.md Co-authored-by: Michał Dudak <[email protected]> Signed-off-by: Ahmed Hany <[email protected]>
Good catch, @Janpot. I see that the Babel plugins also fail to import |
/** @type {import('next').NextConfig} */ | ||
const nextConfig = { | ||
... | ||
experimental: { |
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.
It doesn't seems to be under experimental
in https://nextjs.org/docs/advanced-features/compiler#modularize-imports
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.
It's recently promoted
'@mui/material': { | ||
transform: '@mui/material/{{member}}' | ||
}, |
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.
I wouldn't do this, this is probably overkill. There are also cases where it doesn't work because the code is not consistent enough.
@@ -194,6 +194,31 @@ Modify your `package.json` commands: | |||
} | |||
``` | |||
|
|||
If you are using Next.js >= v12.1.1, you can make use of their [Modularize Imports](https://nextjs.org/docs/advanced-features/compiler#modularize-imports) feature. |
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.
This section of the docs starts to be quite long. I think that it could be good to change the headers to help navigate the content.
I think that it would be great to update the |
@dev-ahmedhany, to sum up - if you'd like to continue with this PR, please remove the instructions for |
@michaldudak done |
Thanks. Please merge in the latest master, this should resolve the failing checks. Also, as @oliviertassinari pointed out, it would be great to update the examples to use this feature (/examples/nextjs and /examples/nextjs-with-typescript). |
How about @mui/x-data-grid example ? hit some error with the config below: modularizeImports: {
'@mui/x-data-grid': {
transform: '@mui/x-data-grid/{{member}}',
},
'@mui/x-date-pickers': {
transform: '@mui/x-date-pickers/{{member}}',
},
}, |
@zigang93 These are already one npm package per "component" (not in the React definition, from the perspective of end-users), there isn't as much value to modularizing imports as with icons.
In any case, we also need to modularize all the internal imports: #35840. |
@oliviertassinari Yeah, I think most of the data grid's code is imported anyway, there are only a few components/hooks that are optional (like |
@cherniavskii I'm thinking of mui/mui-x#7358, and #35840 for the data grid. |
I wonder what we should do with this PR considering vercel/next.js#50900. |
Fixes #51872. We were exploring in mui/material-ui#35457 the option to move the `modularizeImports` config to our Next.js examples to fix mui/material-ui#35450 however, we never got to complete the work. We are not yet in a position where we can apply modularizeImports to `@mui/material`. We still have folders to create to make it work. Maybe we should close mui/material-ui#35457? Co-authored-by: JJ Kasper <[email protected]>
If I understand it correctly, this is applied by default by Next.js and doesn't require additional configuration. I believe we can close this PR. |
I reverted one of those: vercel/next.js#51953 |
This PR is related only to icons-material, so the code you reverted doesn't affect it, does it? |
@michaldudak Agree. I also agree that we can close this PR. However, the opportunity I see is we could open a new issue so we can scale this to all the MUI npm packages, beyond |
Closes #35450
Signed-off-by: Ahmed Hany [email protected]