From 20eb3b308a647ad49102e4236571208ff1cacc56 Mon Sep 17 00:00:00 2001 From: siriwatknp Date: Thu, 28 Nov 2024 16:12:38 +0700 Subject: [PATCH 1/2] prevent unnecessary rerender --- .../src/styles/ThemeProviderWithVars.test.js | 27 ++++++ .../src/cssVars/createCssVarsProvider.js | 92 ++++++++++--------- 2 files changed, 76 insertions(+), 43 deletions(-) diff --git a/packages/mui-material/src/styles/ThemeProviderWithVars.test.js b/packages/mui-material/src/styles/ThemeProviderWithVars.test.js index 57cc558313c691..13a0d583dd3ce8 100644 --- a/packages/mui-material/src/styles/ThemeProviderWithVars.test.js +++ b/packages/mui-material/src/styles/ThemeProviderWithVars.test.js @@ -408,4 +408,31 @@ describe('[Material UI] ThemeProviderWithVars', () => { fireEvent.click(screen.getByText('Dark')); }).not.toErrorDev(); }); + + it('theme should remain the same when ThemeProvider rerenders', () => { + const theme = createTheme({ cssVariables: true }); + + function Inner() { + const upperTheme = useTheme(); + const [count, setCount] = React.useState(0); + React.useEffect(() => { + setCount((prev) => prev + 1); + }, [upperTheme]); + return {count}; + } + function App() { + const [count, setCount] = React.useState(0); + return ( + + + + + ); + } + render(); + + fireEvent.click(screen.getByRole('button')); + + expect(screen.getByTestId('inner')).to.have.text('2'); // due to double rendering + }); }); diff --git a/packages/mui-system/src/cssVars/createCssVarsProvider.js b/packages/mui-system/src/cssVars/createCssVarsProvider.js index e7a2a21febb207..64ed9f9fc4037d 100644 --- a/packages/mui-system/src/cssVars/createCssVarsProvider.js +++ b/packages/mui-system/src/cssVars/createCssVarsProvider.js @@ -48,6 +48,9 @@ export default function createCssVarsProvider(options) { const useColorScheme = () => React.useContext(ColorSchemeContext) || defaultContext; + const defaultColorSchemes = {}; + const defaultComponents = {}; + function CssVarsProvider(props) { const { children, @@ -75,12 +78,12 @@ export default function createCssVarsProvider(options) { return typeof defaultTheme === 'function' ? defaultTheme() : defaultTheme; }, [themeProp]); const scopedTheme = initialTheme[themeId]; + const restThemeProp = scopedTheme || initialTheme; const { - colorSchemes = {}, - components = {}, + colorSchemes = defaultColorSchemes, + components = defaultComponents, cssVarPrefix, - ...restThemeProp - } = scopedTheme || initialTheme; + } = restThemeProp; const joinedColorSchemes = Object.keys(colorSchemes) .filter((k) => !!colorSchemes[k]) .join(','); @@ -126,42 +129,46 @@ export default function createCssVarsProvider(options) { colorScheme = ctx.colorScheme; } - // `colorScheme` is undefined on the server and hydration phase - const calculatedColorScheme = colorScheme || restThemeProp.defaultColorScheme; + const memoTheme = React.useMemo(() => { + // `colorScheme` is undefined on the server and hydration phase + const calculatedColorScheme = colorScheme || restThemeProp.defaultColorScheme; - // 2. get the `vars` object that refers to the CSS custom properties - const themeVars = restThemeProp.generateThemeVars?.() || restThemeProp.vars; + // 2. get the `vars` object that refers to the CSS custom properties + const themeVars = restThemeProp.generateThemeVars?.() || restThemeProp.vars; - // 3. Start composing the theme object - const theme = { - ...restThemeProp, - components, - colorSchemes, - cssVarPrefix, - vars: themeVars, - }; - if (typeof theme.generateSpacing === 'function') { - theme.spacing = theme.generateSpacing(); - } + // 3. Start composing the theme object + const theme = { + ...restThemeProp, + components, + colorSchemes, + cssVarPrefix, + vars: themeVars, + }; + if (typeof theme.generateSpacing === 'function') { + theme.spacing = theme.generateSpacing(); + } - // 4. Resolve the color scheme and merge it to the theme - if (calculatedColorScheme) { - const scheme = colorSchemes[calculatedColorScheme]; - if (scheme && typeof scheme === 'object') { - // 4.1 Merge the selected color scheme to the theme - Object.keys(scheme).forEach((schemeKey) => { - if (scheme[schemeKey] && typeof scheme[schemeKey] === 'object') { - // shallow merge the 1st level structure of the theme. - theme[schemeKey] = { - ...theme[schemeKey], - ...scheme[schemeKey], - }; - } else { - theme[schemeKey] = scheme[schemeKey]; - } - }); + // 4. Resolve the color scheme and merge it to the theme + if (calculatedColorScheme) { + const scheme = colorSchemes[calculatedColorScheme]; + if (scheme && typeof scheme === 'object') { + // 4.1 Merge the selected color scheme to the theme + Object.keys(scheme).forEach((schemeKey) => { + if (scheme[schemeKey] && typeof scheme[schemeKey] === 'object') { + // shallow merge the 1st level structure of the theme. + theme[schemeKey] = { + ...theme[schemeKey], + ...scheme[schemeKey], + }; + } else { + theme[schemeKey] = scheme[schemeKey]; + } + }); + } } - } + + return resolveTheme ? resolveTheme(theme) : theme; + }, [restThemeProp, colorScheme, components, colorSchemes, cssVarPrefix]); // 5. Declaring effects // 5.1 Updates the selector value to use the current color scheme which tells CSS to use the proper stylesheet. @@ -248,7 +255,7 @@ export default function createCssVarsProvider(options) { process.env.NODE_ENV === 'production' ? setMode : (newMode) => { - if (theme.colorSchemeSelector === 'media') { + if (memoTheme.colorSchemeSelector === 'media') { console.error( [ 'MUI: The `setMode` function has no effect if `colorSchemeSelector` is `media` (`media` is the default value).', @@ -270,7 +277,7 @@ export default function createCssVarsProvider(options) { setColorScheme, setMode, systemMode, - theme.colorSchemeSelector, + memoTheme.colorSchemeSelector, ], ); @@ -285,13 +292,12 @@ export default function createCssVarsProvider(options) { const element = ( - + {children} - {shouldGenerateStyleSheet && } + {shouldGenerateStyleSheet && ( + + )} ); From 3d5ff09432827873c1feb81549fca1a01c982527 Mon Sep 17 00:00:00 2001 From: siriwatknp Date: Fri, 29 Nov 2024 10:12:16 +0700 Subject: [PATCH 2/2] update test to not rely on count --- .../src/styles/ThemeProviderWithVars.test.js | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/packages/mui-material/src/styles/ThemeProviderWithVars.test.js b/packages/mui-material/src/styles/ThemeProviderWithVars.test.js index 13a0d583dd3ce8..e06a6d304d63dd 100644 --- a/packages/mui-material/src/styles/ThemeProviderWithVars.test.js +++ b/packages/mui-material/src/styles/ThemeProviderWithVars.test.js @@ -414,17 +414,21 @@ describe('[Material UI] ThemeProviderWithVars', () => { function Inner() { const upperTheme = useTheme(); - const [count, setCount] = React.useState(0); + const themeRef = React.useRef(upperTheme); + const [changed, setChanged] = React.useState(false); React.useEffect(() => { - setCount((prev) => prev + 1); + if (themeRef.current !== upperTheme) { + setChanged(true); + } }, [upperTheme]); - return {count}; + return changed ?
: null; } function App() { - const [count, setCount] = React.useState(0); + const [, setState] = React.useState({}); + const rerender = () => setState({}); return ( - + ); @@ -433,6 +437,6 @@ describe('[Material UI] ThemeProviderWithVars', () => { fireEvent.click(screen.getByRole('button')); - expect(screen.getByTestId('inner')).to.have.text('2'); // due to double rendering + expect(screen.queryByTestId('theme-changed')).to.equal(null); }); });