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

[system] Introduce theme scope for using multiple design systems #36664

Merged
merged 53 commits into from
Apr 11, 2023

Conversation

siriwatknp
Copy link
Member

@siriwatknp siriwatknp commented Mar 28, 2023

Continuation of #31392 (comment)

Problem

The style engine we supports are emotion and styled-components. Both of them use a singleton React context for theming, here is the singleton of emotion. This means if you are using multiple UI libs that uses emotion theming, the theme will override each other in the tree.

<LibAThemeProvider><div>
       <LibBThemeProvider><SomeAComponent /> // will get theme from lib B which could cause error
     </div>

Existing workaround

Joy UI provides this guide to use with Material UI by merging themes into one but the feedbacks are not good because the intensive setup and confusion of the merging process.

New Solution

This PR introduces the theme scope for Material UI and Joy UI for using theme together or with other UI libraries that depend on emotion/styled-components.

All you need to do is wrap the theme inside a special identifier that the library use.

import { THEME_IDENTIFIER, createTheme } from '@mui/material/styles';

const muiTheme = createTheme();

// the ThemeProvider should be from the other library
<ThemeProvider theme={{ …otherTheme, [THEME_IDENTIFIER]: muiTheme }}>

All of these user facing APIs (styled, sx, useTheme, useThemeProps), basically every APIs that can interact with theme will lookup for the special identifier first and return the corresponded theme.

This means there is no breaking change and all of the logics will remain the same if THEME_IDENTIFIER is not used.

Proof that it works

Todos

  • Agree on the THEME_ID export name, $$material for Material UI and $$joy for Joy UI.
  • Add guide on how to use with emotion dependants
  • Update Joy UI guide for using with Material UI

@siriwatknp siriwatknp added package: material-ui Specific to @mui/material package: joy-ui Specific to @mui/joy proof of concept Studying and/or experimenting with a to be validated approach package: system Specific to @mui/system labels Mar 28, 2023
@mui-bot
Copy link

mui-bot commented Mar 28, 2023

Netlify deploy preview

@material-ui/core: parsed: +0.15% , gzip: +0.21%
@material-ui/system: parsed: +0.66% , gzip: +0.76%
@mui/material-next: parsed: +7.49% , gzip: +7.51%
@mui/joy: parsed: +0.18% , gzip: -0.05% 😍

Bundle size report

Details of bundle changes

Generated by 🚫 dangerJS against bcab4ca

Copy link
Member

@cherniavskii cherniavskii left a comment

Choose a reason for hiding this comment

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

This is so exciting!
We just discussed the pain of theme merging yesterday and this solution looks awesome! 🎉


const Box = createBox<Theme>({
identifier: IDENTIFIER,
Copy link
Member

Choose a reason for hiding this comment

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

Maybe call it themeIdentifier instead of a generic identifier? Or even shorten it to themeId?

Copy link
Member Author

Choose a reason for hiding this comment

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

sounds good to me.

@oliviertassinari
Copy link
Member

oliviertassinari commented Mar 29, 2023

For developers calling emotion or styled-components directly, without going through @mui/system, this will be breaking, right? I think that it's a question of how many developers will be impacted: <0.1% (all good) or >10% (oops, probably an urgent revert)?

@siriwatknp
Copy link
Member Author

siriwatknp commented Mar 29, 2023

For developers calling emotion or styled-components directly, without going through @mui/system, this will be breaking, right? I think that it's a question of how many developers will be impacted: <0.1% (all good) or >10% (oops, probably an urgent revert)?

I think it should be <0.1%.

You have to enable a prop to turn on theme scoping so if this PR is merged, the existing projects will still work as it is before even you use emotion/styled-components directly.

It will break once you turn on this feature and you read the theme from emotion:

import styled from '@emotion/styled';

const Foo = styled('div')(({ theme }) => ({ color: theme.palette.primary.main }))

<ThemeProvider enableThemeScope>
  <Foo /> // Error: cannot read `primary` of undefined
</ThemeProvider

However, the fix is easy by changing styled to @mui/material/styles

- import styled from '@emotion/styled';
+ import { styled } from '@mui/material/styles';

Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

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

Should we simplify the API and remove the enableThemeScope prop and just teach developers that if they are using different design systems, they can add our design systems (Material Design and Joy's) under the key THEME_INDENTIFIER. With this, developers will have the complete control in terms of what should go in which theme - this is basically what you have in Material UI + Chakra UI codesandbox. I find it a bit complex and confusing what exactly will be merged where if the enableThemeScope prop is used.

packages/mui-material/src/styles/useThemeProps.js Outdated Show resolved Hide resolved
@@ -52,14 +52,19 @@ function ThemeProvider(props) {
}

const theme = React.useMemo(() => {
const output = outerTheme === null ? localTheme : mergeOuterLocalTheme(outerTheme, localTheme);
let internalTheme = localTheme;
Copy link
Member

Choose a reason for hiding this comment

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

Should we move this logic inside the ThemeProvider in @mui/system? This package was created initially only to serve as a common context for @mui/system & @mui/styles, it shouldn't contain anything that is specific to only one of the packages.

if (identifier && typeof localTheme !== 'function') {
internalTheme = localTheme[identifier] || localTheme;
}
const output =
Copy link
Member

Choose a reason for hiding this comment

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

This step is a bit confusing to me. Mainly because we are merging the two themes, regardless of whether we use the identifier. This means that the theme inside the identifier will have the merged theme from context together with the user-defined theme. As the newly define theme overrides anyway it should be fine, but I am not sure if this is the expected behavior. For e.g. I can access mixins from MD in Joy UI's theme.

@siriwatknp
Copy link
Member Author

Should we simplify the API and remove the enableThemeScope prop and just teach developers that if they are using different design systems, they can add our design systems (Material Design and Joy's) under the key THEME_INDENTIFIER. With this, developers will have the complete control in terms of what should go in which theme - this is basically what you have in Material UI + Chakra UI codesandbox. I find it a bit complex and confusing what exactly will be merged where if the enableThemeScope prop is used.

Agree, having one way of doing this is better.

@siriwatknp
Copy link
Member Author

@mnajdova I think it is ready for review. Latest changes:

  • Add tests to ensure that Material UI, Joy UI and other 3rd-party libraries can work together.
  • Add tests to ensure that @mui/styles does not break when theme scoping is enabled (however, @mui/styles will always look for Material UI theme, not Joy UI, which I think it is fine since it is a legacy package)
  • Update "using Joy and Material UI" guide (looks a lot simpler)
  • Add using with Theme UI and Chakra UI docs for Material UI

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 7, 2023
@siriwatknp siriwatknp requested a review from mnajdova April 10, 2023 05:34
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 10, 2023
@siriwatknp siriwatknp requested a review from mnajdova April 11, 2023 03:26
Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

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

SO EXCITED about this 🎉 Let's ship it!

@jakub-stastny
Copy link
Contributor

jakub-stastny commented Apr 19, 2023

From reading the guide, it might not be obvious to everyone that they need to upgrade their MUI, especially if they like me installed MUI only a few weeks back because THEME_ID and other things are completely new. I was grep'ing through the code, couldn't find it, nothing on Google either, only when I searched through GH I saw it added just now.

TLDR; if this doesn't work for you, you need to update MUI.

@siriwatknp
Copy link
Member Author

From reading the guide, it might not be obvious to everyone that they need to upgrade their MUI, especially if they like me installed MUI only a few weeks back because THEME_ID and other things are completely new. I was grep'ing through the code, couldn't find it, nothing on Google either, only when I searched through GH I saw it added just now.

TLDR; if this doesn't work for you, you need to update MUI.

Good point, thanks for the feedback. I will update the page to include "You need >= v5.12.0"

@siriwatknp
Copy link
Member Author

siriwatknp commented Apr 20, 2023

@jakub-stastny speaking of this, would you like to submit a PR to add it to the docs? including this as well https://mui.com/material-ui/guides/styled-engine/#theme-scoping

@jakub-stastny
Copy link
Contributor

@siriwatknp let me see if I can manage to find a minute on the weekend.

@jakub-stastny
Copy link
Contributor

@siriwatknp #36973

@samuelsycamore
Copy link
Contributor

Is there a reason why the documentation for this was added to the "Styled engine" page (rather than creating a new doc)? IMO this topic is separate from that of using styled-components in place of Emotion, which is what "Styled engine" is mainly concerned with. I've proposed splitting these into two separate docs in #37774 (as well as changing the name of the original doc to make it more informative).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: joy-ui Specific to @mui/joy package: material-ui Specific to @mui/material package: styles Specific to @mui/styles. Legacy package, @material-ui/styled-engine is taking over in v5. package: system Specific to @mui/system proof of concept Studying and/or experimenting with a to be validated approach
Projects
Status: Recently completed
Development

Successfully merging this pull request may close these issues.

7 participants