Skip to content
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

fix: change remaining mui-icons to path imports #155

Merged
merged 4 commits into from
Sep 15, 2023

Conversation

cwierzbicki00
Copy link
Contributor

Changed the import paths to ensure all mui/icons-material follow a direct import path.

Related is an issue which I just opened #154 .

This should solve the issue. I verified that the affected controls still work as intended.

Please let me know of any changes required. Thanks for your patience as this is my first open source PR. I did my best to follow the contribution guidelines and ran all checks.

Copy link
Owner

@sjdemartini sjdemartini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work on your first open-source PR @cwierzbicki00! Glad to have you contributing to mui-tiptap! 🎉

Overall this is looking great! It seems there are a few other icon paths that still also need updating:

  • src/controls/MenuButtonBold.tsx: import { FormatBold } from "@mui/icons-material";
  • src/controls/MenuButtonCode.tsx: import { Code } from "@mui/icons-material";
  • src/controls/MenuButtonIndent.tsx: import { FormatIndentIncrease } from "@mui/icons-material";
  • src/controls/MenuButtonOrderedList.tsx: import { FormatListNumbered } from "@mui/icons-material";
  • src/controls/MenuButtonSubscript.tsx: import { Subscript } from "@mui/icons-material";

Would you be able to handle those in this PR as well? Thanks for documenting the issue and for making the PR!

@cwierzbicki00
Copy link
Contributor Author

@sjdemartini Sorry, don't know how my search missed those. Thanks for the kind and thorough feedback.

I've corrected the remaining icon paths you've listed from

  • src/controls/MenuButtonBold.tsx: import { FormatBold } from "@mui/icons-material";
  • src/controls/MenuButtonCode.tsx: import { Code } from "@mui/icons-material";
  • src/controls/MenuButtonIndent.tsx: import { FormatIndentIncrease } from "@mui/icons-material";
  • src/controls/MenuButtonOrderedList.tsx: import { FormatListNumbered } from "@mui/icons-material";
  • src/controls/MenuButtonSubscript.tsx: import { Subscript } from "@mui/icons-material";

These are now resolved in my most recent commit in this PR. I also double checked on Github search and my VSCode global search. Hopefully that's all of them now.

@sjdemartini
Copy link
Owner

@cwierzbicki00 Thanks for updating this! Looks good to me!

I noticed that on this branch, running npx vite-bundle-visualizer to create a rollup bundle of the demo entrypoint now takes only ~12s, but it used to take roughly double that (20-something seconds), and though the final bundle was roughly the same size (with Vite/rollup), it did seem to be demonstrating the issue you mentioned deploying NextJS13 application to Vercel, where it enumerated each icon while transforming. e.g. it iterates through a whole bunch of these during the process (for icons not actually used by mui-tiptap):

transforming (3113) node_modules/.pnpm/@[email protected]_@[email protected]_@[email protected][email protected]/node_modules/@mui/icons-material/esm/DirectionsBoatTwoTone.js

and reports ✓ 12107 modules transformed. on the main branch, vs just ✓ 1538 modules transformed. on this branch. So that seems to suggest it'll indeed solve your problem and should help in consuming bundlers!


One interesting side note: it seems this change to use the icon-specific default exports rather than the top-level named exports actually increases the bundle size very slightly, at least when using rollup (per the bundle visualizer mentioned above).

Before (12.04 KB from icons-material)

image

After (19.04 KB from icons-material)

image

I guess there's some slight benefit when it uses the top-level named exports via ESM, vs default exports (via CJS presumably). I don't have much familiarity with why that size-difference would happen or if there's anything we can do about it, and I think your change here is the right way to go regardless to improve compatibility with deployments like Nextjs on Vercel. But per the issue you linked with Vercel, and what I've seen in briefly glancing at other open-source projects that use @mui packages, I think we can stick with top-level named exports from @mui/material and just go with this approach for @mui/icons-material. The issue seems to present itself only with the icons package, modern bundlers can properly tree-shake @mui/material without issue, and @mui/material named exports ends up being more convenient for development. I'll add a lint rule to enforce this import style for icons though after merging, to avoid regressions in the future!

Thanks again!

@sjdemartini sjdemartini merged commit f7c51c5 into sjdemartini:main Sep 15, 2023
@cwierzbicki00
Copy link
Contributor Author

@sjdemartini Thank you very much for the detailed response. I learned quite a bit through this issue and change. I really appreciate mui-tiptap, your diligence, and expertise. You have made my first contribution (although small) quite a valuable experience!

I agree 100% with your breakdown. I'll also be adding the lint rule to my projects.

Good luck and looking forward to continued use and collaboration!

@cwierzbicki00 cwierzbicki00 deleted the mui-icons-import branch September 15, 2023 16:29
sjdemartini added a commit that referenced this pull request Oct 13, 2023
Resolves #168

Seems this was due to a typo in
#155, so this bug (showing
the "indent" icon for the "unindent" button) affected v1.8.3.
sjdemartini added a commit that referenced this pull request Oct 13, 2023
Resolves #168

Seems this was due to a typo in
#155, so this bug (showing
the "indent" icon for the "unindent" button) affected v1.8.3.
sjdemartini added a commit that referenced this pull request Oct 13, 2023
Resolves #168

Seems this was due to a typo in
#155, so this bug (showing
the "indent" icon for the "unindent" button) affected v1.8.3.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants