material-ui icon indicating copy to clipboard operation
material-ui copied to clipboard

[docs] Modularize Imports for Nextjs

Open dev-ahmedhany opened this issue 2 years ago • 4 comments

https://github.com/mui/material-ui/issues/35450

Signed-off-by: Ahmed Hany [email protected]

dev-ahmedhany avatar Dec 13 '22 10:12 dev-ahmedhany

Messages
:book: Netlify deploy preview: https://deploy-preview-35457--material-ui.netlify.app/

No bundle size changes

Generated by :no_entry_sign: dangerJS against 4ddd6dec915ac85e0471871658f9f2d41daec9ae

mui-bot avatar Dec 13 '22 10:12 mui-bot

Gave this a quick try on Toolpad and this breaks on imports like:

import { Box, styled } from '@mui/material'

As there is no @mui/material/styled. Perhaps it should only be done for @mui/icons-material?

Janpot avatar Dec 13 '22 16:12 Janpot

@Janpot but @mui/material is also at the docs

'babel-plugin-direct-import',
      { modules: ['@mui/material', '@mui/icons-material'] },

dev-ahmedhany avatar Dec 13 '22 16:12 dev-ahmedhany

Could you add it as Option 3, after a Babel plugin?

michaldudak avatar Dec 14 '22 10:12 michaldudak

Good catch, @Janpot. I see that the Babel plugins also fail to import styled from @mui/material. I suppose your suggestion to apply this only to @mui/icons-material makes the most sense. CC @mui/core

michaldudak avatar Dec 21 '22 15:12 michaldudak

I think that it would be great to update the /examples so that developers get this optimization by default.

oliviertassinari avatar Jan 08 '23 21:01 oliviertassinari

@dev-ahmedhany, to sum up - if you'd like to continue with this PR, please remove the instructions for @mui/material imports (only leave @mui/icons-material). Also, remove the experimental field and update the Next version to 13.1.

michaldudak avatar Jan 09 '23 11:01 michaldudak

@michaldudak done

dev-ahmedhany avatar Jan 12 '23 04:01 dev-ahmedhany

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).

michaldudak avatar Jan 12 '23 08:01 michaldudak

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 avatar Mar 03 '23 06:03 zigang93

How about @mui/x-data-grid example

@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.

  • https://bundlephobia.com/package/@mui/[email protected] the main component is 70% of the bundle size. I think that it makes more sense to look into tree-shaking. Right now, modularizing imports makes little sense. cc @cherniavskii
  • https://bundlephobia.com/package/@mui/[email protected] has more tree-shaking opportunities, and we took advantage of it to support deep imports. If you have there specific errors with the date pickers, please open an issue on https://github.com/mui/mui-x. cc @flaviendelangle

In any case, we also need to modularize all the internal imports: https://github.com/mui/material-ui/issues/35840.

oliviertassinari avatar Mar 05 '23 18:03 oliviertassinari

bundlephobia.com/package/@mui/[email protected] the main component is 70% of the bundle size. I think that it makes more sense to look into tree-shaking. Right now, modularizing imports makes little sense.

@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 GridToolbar). Regarding the tree-shaking, do you have any specific issues in mind?

cherniavskii avatar Mar 22 '23 17:03 cherniavskii

Regarding the tree-shaking, do you have any specific issues in mind?

@cherniavskii I'm thinking of https://github.com/mui/mui-x/issues/7358, and https://github.com/mui/material-ui/issues/35840 for the data grid.

oliviertassinari avatar Mar 22 '23 22:03 oliviertassinari

I wonder what we should do with this PR considering https://github.com/vercel/next.js/pull/50900.

oliviertassinari avatar Jun 29 '23 00:06 oliviertassinari

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.

michaldudak avatar Aug 09 '23 07:08 michaldudak

I reverted one of those: https://github.com/vercel/next.js/pull/51953

oliviertassinari avatar Aug 09 '23 10:08 oliviertassinari

This PR is related only to icons-material, so the code you reverted doesn't affect it, does it?

michaldudak avatar Aug 11 '23 08:08 michaldudak

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 @mui/icons-material. It could be done with special rules, like in https://github.com/vercel/next.js/blob/261db496f7cf04b70f078a6eae0c8baf6fe5c238/packages/next/src/server/config.ts#L738. I didn't have time in https://github.com/vercel/next.js/pull/51953 to do it right, there was an urgency to fix it before it was rolled from their canary release to stable. Then, the question is is the Next.js job to keep this up to date? Is there a similar opportunity with Vite, should we update the Vite example as well? At the end of the day, it's about improving the DX.

oliviertassinari avatar Aug 13 '23 17:08 oliviertassinari