[joy-ui][icons][system] Replace @mui/material dependency with @mui/system

[mnajdova]

closes #34489

issue: https://codesandbox.io/s/882s3v?file=/src/Demo.js

This PR aims to remove the @mui/material dependency from @mui/icons-material, by making the package depend on the @mui/system. This will allow us to use the package with @mui/joy much more easily, by just providing a style override for the SvgIcon without the need for yarn resolution.

TODOs:

• Update joy's default theme to add overrides for the SvgIcon - was done already
• Update joy's Using icon libraries guide (including typescript guide)
• Ensure that the icon is standalone with Material UI API fontSize and color (the typings should work by default). [tested]
• The icon should use overrides from Material UI ThemeProvider.
• Joy UI provides module augmentation to integrate with icons-material
• It should work with theme scoping

[mui-bot]

Netlify deploy preview

• docs/data/joy/integrations/icon-libraries/icon-libraries.md

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against 42fcc90

[mnajdova]

The argos difference comes from the usage of color in the sx prop without using ThemeProvider (as now the default theme for the SvgIcon is the system's default theme). If you open the demo in the the docs it have the expected colors, as it is wrapped with Material UI's ThemeProvider

This is one unfortunate breaking change. I wonder if it will really have an impact, as the users of the library will most likely have a ThemeProvider in their app, but anyway, it's worth noting it.

One way for fixing it, would be to duplicate the Material Design theme inside the @mui/icons-material and use that one as default theme or create a dedicated package for the theme itself that both @mui/material and @mui/icons-material can depend on. This is how it would look like: #35652

[oliviertassinari]

It makes sense to me for v6 👍.

[oliviertassinari]

I would imagine that SvgIcon will eventually go inside @mui/system because once we add a new set of icons for Joy UI 1. Will we duplicate SvgIcon again? 2. Won't we want to have Material UI users be able to import these new icons without loading Joy UI?

[mnajdova]

This was my first implementation too, but honestly SvgIcon in different libraries, will likely have different styles, this is why I've decided to move it to the icons package.

[oliviertassinari]

But then, maybe it means that we should solve #21251 and let the developers import SvgIcon from the right package?

import SvgIconMd from '@mui/material/SvgIcon';
import SvgIconJoy from '@mui/joy/SvgIcon';
import EditIcon from '@mui/icons-material/Edit';

<SvgIconMd>
  <EditIcon />
</SvgIconMd>

It's a fallback method of FontAwesome: https://fontawesome.com/docs/web/use-with/react/add-icons#add-individual-icons-explicitly.

[michaldudak]

I wouldn't put SvgIcon in the common package, at least in its current shape. It is tailored specifically for Material Design icons (sizes, viewport) and shouldn't be used with arbitrary icons (despite its generic name). If we create another icon set in the future, it can have its own SvgIcon. We could extract the common parts into a Base component if there is a lot of duplication between different packages' versions.

In general, I like the approach from #35652 a bit more. It's a change that shouldn't break anyone.

[oliviertassinari]

It is tailored specifically for Material Design icons (sizes, viewport)

I don't think that these are fundamentally specific to Material Design or Joy UI. We document how it can be used with random SVG path in https://mui.com/material-ui/icons/#font-awesome and are using it for the same use case as well. IMHO, the value of this component is to apply a few better default: attributes, a11y resets, CSS resets, that developer would want. It's also to provide the MUI System style helper.

[michaldudak]

We can publish a generic SvgIcon without the defaults tailored for Material Icons and icon set-specific versions so the sizes don't have to be overridden.

[mnajdova]

But then, maybe it means that we should solve #21251 and let the developers import SvgIcon from the right package?

import SvgIconMd from '@mui/material/SvgIcon';
import SvgIconJoy from '@mui/joy/SvgIcon';
import EditIcon from '@mui/icons-material/Edit';

<SvgIconMd>
  <EditIcon />
</SvgIconMd>

While this is great for a fallback method, I don't think this should be the default for a icons package we provide. I would rather have two icons packages for material & joy instead of this. However, compared to this, I think the PR's solution is still a good enough replacement.

+1 for exporting an SvgIcon for itegrating with third party icon sets.

[siriwatknp]

@mnajdova With the latest change:

• The sx prop works 99.99% without a ThemeProvider except when using functions from the theme, e.g. theme.palette.getContrastText(…) or theme.transition.create(…) because I think these are likely not used with icons to reduce the complexity.
• The theme augmentation works like this:

  declare module "@mui/icons-material/SvgIcon" {
     export interface SvgIconPropsFontSizeOverrides {
       // add your custom icon size name here
       "icon-xxl": true;
     }
   }

• Now works with Joy UI.

[mnajdova]

@siriwatknp your changes are somewhat in the middle between the initial aim of this PR and #35652 which I created back then as an alternative, I think it's worth exploring 👌

The sx prop works 99.99% without a ThemeProvider except when using functions from the theme, e.g. theme.palette.getContrastText(…) or theme.transition.create(…) because I think these are likely not used with icons to reduce the complexity.

I would disagree here, the sx prop can be defined as a callback so you can't really know what would people expect to exist in the theme (everything that they can otherwise access). We can argue how the ThemeProvider is almost always defined, but that is out of the question, we would be creating regression knowingly, which I don't think it's a good thing - this was the reason why back in #35652 I extracted the whole team.

The theme augmentation works like this

We should keep the old export for backward compatibility.

Now works with Joy UI.

👌

[siriwatknp]

We can argue how the ThemeProvider is almost always defined

🤔 I doubt this. I think one selling point of Material UI is being used without creating an empty theme. I think we should keep this PR non-breaking changes. Otherwise, this PR should be revisited in v6 since it is a breaking change.

If we want to continue this PR in v5, we can improve #35652 by writing a script that will read the Material UI default theme and copy those values as a default theme of Icons. As you can see, the default theme is more maintainable compared to #35652. We can also add a CI to run the script to help us detect changes in Material UI default theme so that the author does not forget to update icon default theme.

[mnajdova]

🤔 I doubt this. I think one selling point of Material UI is being used without creating an empty theme. I think we should keep this PR non-breaking changes. Otherwise, this PR should be revisited in v6 since it is a breaking change.

Exactly my point, this is why I mentioned it, we can't merge this as is right now.

We can also add a CI to run the script to help us detect changes in Material UI default theme so that the author does not forget to update icon default theme.

Regardless of whether we automate this process or have a separate package, the disadvantage is the same - we end up with the theme bundle if you use the Material UI icons package, regardless of whether you need it or not.

[zacronos]

It's unfortunate that this no longer seems to be part of the v6 roadmap -- the intent to remove the @mui/material dependency with v6 was part of the reasoning (on this alternate PR: #35652) for not moving forward with a different approach as a "for now" solution while waiting on this one.

If this is no longer going to be part of v6, maybe the alternatives suggested in #35652 should be reevaluated, so there will be some solution on the horizon, even if that involves some trade-offs?

To add a bit more context for why removing the dependency is important, one thing I haven't seen anyone mention is that with the status quo, it seems impossible to use JoyUI, Material Icons, and DataGrid (or any other MUI X components) together. Using JoyUI + Material Icons requires this resolution hack, and the resolution hack breaks MUI X. (Or, at least it does for users of pnpm; maybe this wouldn't be the case for a package manager that isn't trying to be so "smart"?)

The main reason for not doing something like #35652 seemed to be bundle size -- but personally I would prefer a bigger bundle that doesn't have interoperability problems, vs a smaller one that does.

[KG-khangelani]

It's unfortunate that this no longer seems to be part of the v6 roadmap -- the intent to remove the @mui/material dependency with v6 was part of the reasoning (on this alternate PR: #35652) for not moving forward with a different approach as a "for now" solution while waiting on this one.

If this is no longer going to be part of v6, maybe the alternatives suggested in #35652 should be reevaluated, so there will be some solution on the horizon, even if that involves some trade-offs?

To add a bit more context for why removing the dependency is important, one thing I haven't seen anyone mention is that with the status quo, it seems impossible to use JoyUI, Material Icons, and DataGrid (or any other MUI X components) together. Using JoyUI + Material Icons requires this resolution hack, and the resolution hack breaks MUI X. (Or, at least it does for users of pnpm; maybe this wouldn't be the case for a package manager that isn't trying to be so "smart"?)

The main reason for not doing something like #35652 seemed to be bundle size -- but personally I would prefer a bigger bundle that doesn't have interoperability problems, vs a smaller one that does.

It really is unfortunate, I was actually betting on it being done sooner. Now I'm starting to wonder what would it take to get this done🤔

[mnajdova]

@zacronos sorry for responding a bit late, I had a lot of notifications. If bundle size is not an issue for you, you can just use Joy UI, Material UI, Material UI Icons and the DataGrid altogether. Have you tried this? Is this not working at this moment?

[zacronos]

@mnajdova thanks for the response, late is better than never. 😄

I don't believe it works to use them all together, no -- though maybe I'm doing it wrong somehow?

In the yarn section of https://mui.com/joy-ui/integrations/icon-libraries/, there is a big warning box that says:

Because @mui/material is a required dependency of @mui/icons-material, you have to add a workaround with yarn resolutions:

{
  "dependencies": {
    "@mui/material": "npm:@mui/joy@latest"
  },
  "resolutions": {
    "@mui/material": "npm:@mui/joy@latest"
  }
}

After that, run yarn install in your terminal.

We are aware of this limitation and are considering removing the dependency. You can keep track of the progress in this issue.

Is that incorrect? If the only reason for using that workaround is for bundle size, then maybe they can all work together. When following those instructions as stated, however, it seems it is impossible to use MUIX components like DataGrid. I could remove the resolution hack, and see if everything works when simply installing all 4 dependencies, as you suggest.

[mnajdova]

Is that incorrect? If the only reason for using that workaround is for bundle size, then maybe they can all work together. When following those instructions as stated, however, it seems it is impossible to use MUIX components like DataGrid. I could remove the resolution hack, and see if everything works when simply installing all 4 dependencies, as you suggest.

You can skip this step if you are okay to have @mui/material installed and in the bundle too. This was exactly my point. Also, tree shaking works, so eventually you would end up with the theme and few utils that are used in the icons, which is still not ideal, but not the end of the world too :)

[zacronos]

Got it, thank you for the clarification @mnajdova! I'll give that a try next time I'm making changes in that area of the code.