sjdemartini / mui-tiptap

A Material UI (MUI) styled WYSIWYG rich text editor, using Tiptap
MIT License
320 stars 44 forks source link

Change Remaining @mui/icons-material imports to Direct Path Imports #154

Closed cwierzbicki00 closed 1 year ago

cwierzbicki00 commented 1 year ago

Describe the bug

On some platforms, such as Nextjs 13, an issue emerges due to the large bundle size of mui-tiptap / it's import method for some of "@mui/icons-material". For most of the codebase, direct path imports are used. There are some areas where the tree shaking imports are used, which bloat the package and are strangely linked to issues on Nextjs 13.

To Reproduce

Not possible to reproduce without a full Nextjs 13 deployment on Vercel. Not necessarily relevant, either.

Steps to reproduce the behavior:

  1. Deploy any NextJS13 application to Vercel which uses API features.
  2. Attempt to call the API by some page action.
  3. Receive 504 timeout errors, like discussed in (https://github.com/vercel/next.js/discussions/47237)

Expected behavior

The inclusion of mui-tiptap should safely import the icons from @mui/icons-material using the direct path. Then, the application runs smoothly.

Screenshots

Simply need to change from image to this image

System (please complete the following information):

Additional context

Regardless, this will reduce the bundle size and is considered best practice. (See mui docs https://mui.com/material-ui/icons/#usage )

The imports should be changed to direct path imports unless the techniques specified here are used https://mui.com/material-ui/guides/minimizing-bundle-size/#option-two-use-a-babel-plugin

It is my first time contributing on OSS, so please forgive any mishaps. I am a really big fan of this project and am impressed with the design & codebase. I will attempt to open a PR for this issue shortly.

sjdemartini commented 1 year ago

Thank you for reporting and detailing this @cwierzbicki00! This is something that had been on my radar vaguely but I hadn't pursued making changes yet. I suppose per https://mui.com/material-ui/guides/minimizing-bundle-size/#when-and-how-to-use-tree-shaking, we should be updating not just the @mui/icons-material paths but also the @mui/material component paths as well. Those can definitely be done separately though. I'll take a look at your PR shortly, thanks again!

sjdemartini commented 1 year ago

Resolved via https://github.com/sjdemartini/mui-tiptap/pull/155, and now enforcing with an eslint rule via https://github.com/sjdemartini/mui-tiptap/pull/159.

And as mentioned in https://github.com/sjdemartini/mui-tiptap/pull/155#issuecomment-1721534645, we'll stick with the top-level named imports still for @mui/material, since the issue doesn't seem to arise there and it's a better development experience.