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

[icons] Replace @mui/material dependency with @mui/system v2 #35652

Closed
wants to merge 15 commits into from

Conversation

mnajdova
Copy link
Member

@mnajdova mnajdova commented Dec 28, 2022

This is an alternative to #35637. The goal is still to remove the @mui/material dependency from @mui/icons-material, but the difference is that instead of the default system's theme, the icons package would depend on Material Design's theme, trough a new @mui/md-theme package. This new package would be used both in @mui/material and @mui/icons-material. It would allow the sx prop to be used in the same manner in the icons as the other Material UI components without the need for a ThemeProvider. This would allow us to ship the changes without introducing any breaking changes.

@mnajdova mnajdova added package: icons Specific to @mui/icons enhancement This is not a bug, nor a new feature labels Dec 28, 2022
@mui-bot
Copy link

mui-bot commented Dec 28, 2022

Messages
📖 Netlify deploy preview: https://deploy-preview-35652--material-ui.netlify.app/

Details of bundle changes

Generated by 🚫 dangerJS against f80e402

@mnajdova mnajdova marked this pull request as ready for review December 28, 2022 13:31
@mnajdova mnajdova requested review from a team and oliviertassinari and removed request for a team December 28, 2022 13:46
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Dec 30, 2022
@oliviertassinari
Copy link
Member

oliviertassinari commented Jan 1, 2023

I feel that this direction dodges what the problem is about. I mean, we could do this diff, and say problem solved:

diff --git a/packages/mui-icons-material/package.json b/packages/mui-icons-material/package.json
index 34c835b353..963a502dac 100644
--- a/packages/mui-icons-material/package.json
+++ b/packages/mui-icons-material/package.json
@@ -46,17 +46,25 @@
     "typescript": "tslint -p tsconfig.json \"src/**/*.{ts,tsx}\""
   },
   "peerDependencies": {
-    "@mui/material": "^5.0.0",
+    "@emotion/react": "^11.5.0",
+    "@emotion/styled": "^11.3.0",
     "@types/react": "^17.0.0 || ^18.0.0",
     "react": "^17.0.0 || ^18.0.0"
   },
   "peerDependenciesMeta": {
     "@types/react": {
       "optional": true
+    },
+    "@emotion/react": {
+      "optional": true
+    },
+    "@emotion/styled": {
+      "optional": true
     }
   },
   "dependencies": {
-    "@babel/runtime": "^7.20.7"
+    "@babel/runtime": "^7.20.7",
+    "@mui/material": "^5.0.0"
   },
   "devDependencies": {
     "fs-extra": "^10.1.0",

Or what would be missing?

@mnajdova
Copy link
Member Author

mnajdova commented Jan 2, 2023

I feel that this direction dodges what the problem is about.

Agree, but if we want to resolve #35637 (comment) (and have a non-breaking change solution for removing the dependency of @mui/material) it's the only way to go. I am happy to go with #35637 in v6, but till then this may be a "good enough" workaround, that will enable @mui/joy users to use @mui/icons-material without the need for yarn resolutions.

@oliviertassinari
Copy link
Member

The point I want to get to is. What problem do we solve with this PR? I struggle to understand it.

From what I understand, we remove the dependency on @mui/material but we keep all the bundle sizes of it (it seems, but I don't know for sure because we don't test one icon in https://mui-dashboard.netlify.app/size-comparison?circleCIBuildNumber=467572&baseRef=master&baseCommit=e9d8fa07e1ff979bcaf92837b131cd9e292f0161&prNumber=35652). Which makes me wonder: How is it different?

@mnajdova
Copy link
Member Author

mnajdova commented Jan 3, 2023

From what I understand, we remove the dependency on @mui/material but we keep all the bundle sizes of it (it seems, but I don't know for sure because we don't test one icon in https://mui-dashboard.netlify.app/size-comparison?circleCIBuildNumber=467572&baseRef=master&baseCommit=e9d8fa07e1ff979bcaf92837b131cd9e292f0161&prNumber=35652). Which makes me wonder: How is it different?

The problem we are fixing here is, easing up the usage of @mui/icons-material with @mui/joy, without breaking changes. Basically making this not necessary - https://mui.com/joy-ui/guides/using-icon-libraries/. In v6, we can replace this with #35637

@siriwatknp
Copy link
Member

siriwatknp commented Jan 3, 2023

Thanks for the POC.

I'd say we should go with #35637 as a starting point (we could do it in v6) because the issue is just about sx prop on the icons and wrapping them with ThemeProvider fixes the issue. This PR's approach will hurt Joy UI and MD3 in terms of bundle size.

We could add a doc that says @mui/icons-material needs a ThemeProvider to work with sx prop. If we got too many complaints from the community, we can still consider with this PR's approach.

unstable_composeClasses as composeClasses,
} from '@mui/utils';
import { createStyled, shouldForwardProp, useThemeProps as systemUseThemeProps } from '@mui/system';
import { defaultTheme } from '@mui/md-theme';
Copy link
Member

Choose a reason for hiding this comment

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

This part increases the unnecessary bundle size for Joy UI and MD3.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, it's unfortunate necessary thing for making sure we don't introduce breaking changes. Jun, if you feel like you are ok with having https://mui.com/joy-ui/guides/using-icon-libraries/ till v6, I am happy to go with the other PR 👍

@oliviertassinari
Copy link
Member

oliviertassinari commented Jan 9, 2023

Basically making this not necessary - https://mui.com/joy-ui/guides/using-icon-libraries/

@mnajdova Ahhh, right. So f we look one level lower, it's about the theming structure that is not compatible between Joy UI and Material UI? I assume that if we were to find a way to isolate the theme context, we could then drop https://mui.com/joy-ui/guides/using-joy-ui-and-material-ui-together/, it would work be default, we could close the issue about Chakra UI inconsistency, and we could get Material UI icons works with Joy UI, by default. However, it would have the wrong theming values.

I'd say we should go with #35637

@siriwatknp This makes more sense to me as well.


If we are looking for more options, where say option 1 is #35637, and option 2. is #35652. I think that option 3 would be SVG export #35637 (comment). Option 4 would be option 3 but with a Babel/SWC plugin to keep the same DX: https://fontawesome.com/docs/web/use-with/react/add-icons#set-up-the-babel-configs.

@mnajdova
Copy link
Member Author

Alright, I am closing this. We can explore further #35637 & #35637 (comment) once we are closer to v6's release.

@mnajdova mnajdova closed this Jan 10, 2023
@oliviertassinari
Copy link
Member

oliviertassinari commented Jan 12, 2023

Option 5 could be to say: don't use SVG icons but font icons. #32846 make this a more appealing option. Option 6, which I think would still be better than nothing is fixing the theme isolation with emotion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement This is not a bug, nor a new feature package: icons Specific to @mui/icons PR: out-of-date The pull request has merge conflicts and can't be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants