[core] Support @mui/material@6 peer dependency
#14142
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| import materialPackageJson from '@mui/material/package.json'; | ||
| import { checkMaterialVersion } from 'test/utils/checkMaterialVersion'; | ||
| import packageJson from '../../package.json'; | ||
|
|
||
| checkMaterialVersion({ packageJson, materialPackageJson }); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| import materialPackageJson from '@mui/material/package.json'; | ||
| import { checkMaterialVersion } from 'test/utils/checkMaterialVersion'; | ||
| import packageJson from '../../package.json'; | ||
|
|
||
| checkMaterialVersion({ packageJson, materialPackageJson }); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| import materialPackageJson from '@mui/material/package.json'; | ||
| import { checkMaterialVersion } from 'test/utils/checkMaterialVersion'; | ||
| import packageJson from '../../package.json'; | ||
|
|
||
| checkMaterialVersion({ packageJson, materialPackageJson }); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| import materialPackageJson from '@mui/material/package.json'; | ||
| import { checkMaterialVersion } from 'test/utils/checkMaterialVersion'; | ||
| import packageJson from '../../package.json'; | ||
|
|
||
| checkMaterialVersion({ packageJson, materialPackageJson }); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@LukasTy
The next tests use material v6 and v5 for
@mui/utilsand@mui/system, so it's an identical situation.I hoped we could keep using v5 for
@mui/utilsand@mui/system, but this doesn't play nice with material v6.I can try it the other way around – using v6 of
@mui/utilsand@mui/systemwith both v5 and v6 of@mui/material.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe I've mentioned it—I have tried this inverse approach, results are the same... 🙈
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I'm not sure what to do then...
@mui/systemand@mui/utilsare direct dependencies, so we need to declare a specific version there (as opposed to@mui/materialwhich is a peer dependency).Any ideas?
cc @mui/core
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is
useRtlthe problem? AFAIK it hasn't been modified since it was introduced 🤔But yeah, Material UI v6 is not compatible with
@mui/utilsand@mui/systemv5 or vice-versa because of (at least) these v6 changes@mui/system:Unstable_Grid->Grid@mui/utils:useIsFocusVisiblein favor ofisFocusVisiblescrollLeftas it was only necessary for IE 11But independent of what MUI X sets its
@mui/utilsand@mui/systemdependencies to, if Material UI v5/v6 sets them to v5/v6, respectively, shouldn't those be used for the Material UI package? I might be wrong, I'm not a package manager resolution expert 😅@Janpot might be able to help us with this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is the difference:
@mui/material@5@mui/material@6useRtlreturnsfalseeven when RTL is turned on:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Firstly, sorry for being "late to the party".
I stumbled into this problem/realization just before finishing my work day and didn't add a comment. 🙈
It is somewhat wrong, because
@mui/systemis used directly in the packages.Such dependencies should be direct.
Besides, if my memory serves me well unless users will explicitly install
@mui/systemin their apps, they will always see a warning that peer dependency is missing after an installation (even though@mui/materialhas it as a direct dependency).I'm not sure we should, given the last comment.
Unless
@mui/materialalso does the same. 🙈I wanted to finish reading https://github.com/mui/pigment-css/issues/129 before jumping to any conclusions.
But it seems that the
RtlProvideris a now a bit of a problem to integrate the multiple major support with. 🙈There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about moving
RtlProviderinside@mui/private-theming(a dependency of MUI System) in v5 and pin the version to MUI system?This way the
RtlProviderwill be the same context.@mui/private-theminghas not been changed since v5 and I don't think it will change in the future.To summarize:
RtlProviderfrom System to private-theming in v5 and next branch (still reexportRtlProviderfrom System), then do the latest release.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 This is the right observation. If
@mui/systemcreates a context that's shared between multiple packages, all these packages must resolve their import to the exact same installed dependency. Same version is not even enough, it may be twice installed with the same version.I think so, yes. For
@mui/materialwe solve the problem by declaring a peer dependency. I believe it makes sense to do the same for@mui/system. A peer dependency is saying "I want to use this dependency, but I want to use the version that's already installed by the user" which is part of the behavior we're after. I don't believe there is any way to 100% guarantee we will be receiving the exact transitive dependency of a peer dependency.I don't think that will be necessary to solve the problem here. It can be the decider for which version of
@mui/systemis used.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, this is the issue, adding @mui/system as a peer dependency makes sense. It doesn't makes sense to move the RtlProvider to @mui/private-theming, as this package is aim to share the context between JSS and Emotion. Moving the context is not a solution, resolving to the same @mui/system instance is, so peer dependency is the only solution I see.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 one step closer to fix mui/material-ui#40594. We should treat our styling engine, like Shadcn experiences using Tailwind CSS. It's a dependency they are aware of.
Ideally, we would fix mui/material-ui#43443 though. I don't get why MUI System needs a singleton.
As for how this fits with Pigment CSS, regardless of the path we go with, we will need one dependency. The two possible directions:
Now MUI X should be copying the lab, so we should match with https://github.com/mui/material-ui/blob/71a7ff1355e8b6df873dd8440c5a13ae25987ef4/packages/mui-lab/package.json#L45.