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] Disable theme recalculation as default behavior #45405

Merged
merged 17 commits into from
Mar 11, 2025

Conversation

siriwatknp
Copy link
Member

@siriwatknp siriwatknp commented Feb 25, 2025

Part of #44534 to improve the docs performance

Motivation

To skip the unnecessary rerender when mode changes.

Prior, components always rerender even though the stylesheet does not change. This is because the theme.palette.* changes based on mode trigger all components that uses Emotion styled to rerender (for the docs, it means the whole page).

Screen.Recording.2568-02-25.at.16.04.19.mov

This means if switching between modes does not cause the theme to be recreated, most components does not need to rerender.

Solution

Disable theme recalculation to be the default behavior when using CSS theme variables together with built-in light and dark color schemes.

To opt out from this default behavior, set forceThemeRerender prop to ThemeProvider.

This is result:

Screen.Recording.2568-02-25.at.16.03.36.mov

Sorry, something went wrong.

@mui-bot
Copy link

mui-bot commented Feb 25, 2025

@siriwatknp siriwatknp added package: system Specific to @mui/system package: material-ui Specific to @mui/material needs cherry-pick The PR should be cherry-picked to master after merge labels Feb 25, 2025
Copy link
Member

@aarongarciah aarongarciah left a comment

Choose a reason for hiding this comment

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

Some thoughts:

  • Although I understand the reasons to introduce this, it feels like we are introducing a potential foot gun. Maybe there's no other way? This serves our exact use case but most apps in the wild won't benefit from this due to different theme values for light/dark.
  • Could this be automated somehow?
  • How are we planning to document this?

@siriwatknp
Copy link
Member Author

This serves our exact use case but most apps in the wild won't benefit from this due to different theme values for light/dark.

I think apps (built with Material UI) that want high performance and move closer to CSS would want the same. If you already use CSS variables and theme.vars, the different values live in CSS. This PR turns off the different values in JS because it's not needed.

At this moment, I agree with you that most apps does not need this but I think the docs need to stay ahead to be an example of how to get the best performance of Material UI.

Could this be automated somehow?

Not sure what context of "automated" you are referring to but I think having a prop to enable this behavior is the softest approach given the big number of Material UI users.

Or we could make it more aggressive by saying if CSS variables is used with multi color schemes, the theme won't change.

I'd prefer the softer approach.

How are we planning to document this?

I think this can be added to the CSS theme variable page as part of optimization. Let me draft the section and get your review.

@aarongarciah
Copy link
Member

Or we could make it more aggressive by saying if CSS variables is used with multi color schemes, the theme won't change.

I'd seriously consider this, now or in a future major, after studying all the pros/cons. It's one of the big benefits of using CSS vars for theming: the browser does the heavy lifting. Also, CSS vars are opt-in so it'd only "affect" users using CSS vars if we decide to change the behavior.

At this moment, I agree with you that most apps does not need this but I think the docs need to stay ahead to be an example of how to get the best performance of Material UI.

We should try to make perf great out of the box, not opt-in, if possible. Staying ahead in our docs is very important but trying to have great perf for all CSS vars users is even better. Introducing yet another option makes things more complex and the ThemeProvider with or without CSS vars is complex enough already e.g. it's already ready difficult to understand everything that's going on when reading https://mui.com/material-ui/customization/css-theme-variables/overview/, especially by someone unfamiliar with Material UI.

I'm not very familiar with this whole implementation, just wanted to give some high-level feedback.

@siriwatknp
Copy link
Member Author

siriwatknp commented Feb 28, 2025

I'd seriously consider this, now or in a future major, after studying all the pros/cons. It's one of the big benefits of using CSS vars for theming: the browser does the heavy lifting. Also, CSS vars are opt-in so it'd only "affect" users using CSS vars if we decide to change the behavior.

What's your thought on this @DiegoAndai since beta has launched? if we do this it will be a behavior change from v6 -> v7.

I am fine with the aggressive approach too. One benefit of it is help identify the UI that haven't used theme.var.* when switching between modes.

"We should try to make perf great out of the box, not opt-in" @aarongarciah . This could be one of the selling point for aggressive approach.

@DiegoAndai
Copy link
Member

This was discussed in the weekly meeting and the conclusion was:

If cssVariables is enabled in the theme, then freezeThemeValues is true by default. Users can turn it off in case they have to use theme.* (and not theme.var.*). We need to mention this in the migration guide.

@siriwatknp siriwatknp changed the title [system] Add option to freeze theme values [system] Disable theme recalculation as default behavior Mar 4, 2025
Signed-off-by: Siriwat K <siriwatkunaporn@gmail.com>
@siriwatknp siriwatknp marked this pull request as ready for review March 4, 2025 08:03
@siriwatknp
Copy link
Member Author

@DiegoAndai @aarongarciah I change the prop name to forceRecalculateTheme instead since the default behavior does not change the theme values between modes.

So users who want to opt out, they should use forceRecalculateTheme prop to ThemeProvider.

@siriwatknp siriwatknp removed the needs cherry-pick The PR should be cherry-picked to master after merge label Mar 6, 2025
}));
```

If you need to do runtime calculation, we recommend using CSS approach whenever possible.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
If you need to do runtime calculation, we recommend using CSS approach whenever possible.
If you need to do runtime calculations, we recommend using CSS instead of JS whenever possible.

Comment on lines 183 to 185
const { setMode } = useColorScheme();
setMode('dark');
console.log(theme.palette.mode); // 'light'
Copy link
Member

Choose a reason for hiding this comment

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

How would someone implement a color mode toggler if they can't use useColorScheme to get the current mode? 🤔 It's extremely confusing doing setMode('dark') and not getting the updated value.


Off-topic: I just realized the hook is named useColorScheme but it returns mode and setMode, why is that?

Copy link
Member Author

Choose a reason for hiding this comment

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

Those are separate thing, the mode you get from useColorScheme is a React state that ThemeProvider created internally.

When the mode state changes, ThemeProvider recalculate the theme as pass to Emotion theme context, causing all components to rerender (this is the previous behavior).

It's extremely confusing doing setMode('dark') and not getting the updated value.
This is not true, because when you call setMode('dark') the mode state always update. It's just that the theme now does not change.

May be a blog post or a doc about it might help. I think moving from JS theme to CSS vars theme is quite a shift, I will try to draft the doc about the transition.

Copy link
Member

Choose a reason for hiding this comment

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

If I want to build a light/dark toggle using useColorScheme or if I want to have conditional rendering based on the current color scheme (mode), how can I do it if the mode returned from useColorScheme doesn't return the updated value?

Copy link
Member Author

Choose a reason for hiding this comment

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

The mode returned from useColorScheme always updated (it's a React state). It's independent from the theme.

Copy link
Member

@aarongarciah aarongarciah Mar 10, 2025

Choose a reason for hiding this comment

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

I think there's a typo then? I was confused by the code snippet.

edit: not a typo (the code is technically correct) but confusing

Suggested change
const { setMode } = useColorScheme();
setMode('dark');
console.log(theme.palette.mode); // 'light'
const { setMode } = useColorScheme();
setMode('dark');
console.log(theme.palette.mode); // 'dark'

Copy link
Member

Choose a reason for hiding this comment

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

I mean, the snippet is correct (the logged value will be 'light' until the component re-renders) but it's confusing, especially in conjunction with the phrase preceding the code snippet ("The theme no longer changes between modes"). Can we change the snippet and the preceding phrase?

Copy link
Member Author

@siriwatknp siriwatknp Mar 10, 2025

Choose a reason for hiding this comment

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

I see, it's confusing indeed.

I updated the snippet and add more details to it on 83e9b82. The key here is to show that mode from useColorScheme is always updated with the toggle but the theme object does not change between modes.

…-infra/perf-cssvars
@siriwatknp siriwatknp merged commit 66badfb into mui:master Mar 11, 2025
22 checks passed
mnajdova pushed a commit to mnajdova/material-ui that referenced this pull request Mar 11, 2025
Signed-off-by: Siriwat K <siriwatkunaporn@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change package: material-ui Specific to @mui/material package: system Specific to @mui/system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants