[docs] Fix useStorageState regressions #41223

Janpot · 2024-02-21T14:17:49Z

Fix hydration mismatches in the theme toggle. The implementation didn't deal well with mode being defined during hydration, even though it had logic to stabilize values during hydration already. I'm just rendering an empty disabled button until it's mounted, as that seems to have been the intended behavior already. This is the simplest solution to avoid us from stabilizing every possible prop during mounting.
Fix tooltip warning on disabled theme toggle button
To address [system] Catch localStorage unavailability #34027 ~~I propose we just fall back to the SSR implementation, which is a simple React.useState~~ brought back the try/catch to handle all the ways localStorage can be disabled.

@siriwatknp Instead of switching them in the dom, I assume it could be possible to use css vars to toggle the visibility of these two icons. I can't seem to immediately find how to do this though. Opportunity to improve the docs?

Preview: https://deploy-preview-41223--material-ui.netlify.app/

mui-bot · 2024-02-21T14:20:22Z

Netlify deploy preview

https://deploy-preview-41223--material-ui.netlify.app/

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against e217b3d

packages/mui-utils/src/useLocalStorageState/useLocalStorageState.ts

Janpot · 2024-02-21T15:21:26Z

I noticed that many of these hooks define 'use client'. This seems a bit useless. I can understand that for our components we do this as a subset of the properties can be used to transfer data from server to client, but with these hooks I don't see any possible use. Perhaps we should mark these with import 'client-only' instead?

For our components we could consider exporting a separate 'use client' version that only defines the subset of the properties that are serializable. This could take away some of the confusion that happens when they are imported in server components.

oliviertassinari · 2024-02-21T15:50:16Z

I noticed that many of these hooks define 'use client'

@Janpot I think I get the point but do you have an example?

How I understand things work:

This could take away some of the confusion that happens when they are imported in server components.

Which confusion?

For our components we could consider exporting a separate 'use client' version that only defines the subset of the properties that are serializable

This makes me think of #39010 (comment). I believe we need a theme.js in the server bundle, and one in the client bundle (which runs serverside for the initial request).

Janpot · 2024-02-21T16:23:22Z

How I understand things work:

Yep, seems about right. So the question would be why is e.g. useOnMount.ts marked with 'use client'? You can't call it as a hook in server components. It might make more sense to mark it with import 'client-only', to have it fail at build time. Not super important, just a thought I had reading through this code.

Which confusion?

The file is marked as a server/client boundary ('use client'). The types of the components allow for assigning non-serializable values. e.g. they happily autocomplete and build but fail at runtime

'use server'
import { Box } from "@mui/material";

export default function Home() {
  return (
    // This fails at runtime
    <Box sx={{ background: (theme) => theme.palette.background.paper }}>
      Hello
    </Box>
  );
}

Perhaps it makes sense to make two versions of the components:

one that defines only the serializable subset of the properties in its types and that defines 'use client' and that proxies its calls to the next one
one that defines all properties and that adds import 'client-only'

That way either the build fails (you imported something that has import 'client-only' on the server). Or the types don't suggest you can put a function there.

Just some shower thoughts though. Nothing ground-breaking, I just tend to treat types as documentation, and it would be helpful if they prevent me from writing invalid code. Haven't found any comment of the React core team on how libraries should deal with this in the ideal scenario.

oliviertassinari · 2024-02-24T23:59:42Z

packages/mui-system/src/cssVars/useCurrentColorScheme.ts

-        // If the key is deleted, value will be null then reset color scheme to the default one.
-        if (event.key.endsWith('light')) {
-          setColorScheme({ light: value as SupportedColorScheme | null });
+    if (storageWindow) {


Moved this to wrap the maximum possible scope.

Is this operating on the same key as useLocalStorageState? Can't it just use the hook instead? I don't think the storage value should be manipulated by anything else, it will definitely lead to bugs.

Is this operating on the same key as useLocalStorageState?

It does, but the TODO comments I have left in the code are to move to a point where the mode is to be handled by useCurrentColorScheme and not useLocalStorageState.

The way I envisioned this was

// have these hooks available in the docs function usePaletteModeUserPreference() { const [mode, setMode] = useLocalStorageState('colorScheme', 'system'); return { mode, setMode }; } function useCurrentPaletteMode() { const { mode: userMode } = usePaletteModeUserPreference(); const systemColor = useSystemColor(); return userMode === 'system' ? systemColor : userMode; } // for theme provider const paletteMode = useCurrentPaletteMode() const theme = paletteMode === 'dark' ? darkTheme : lightTheme; // anywhere for theme switchers const { mode, setMode } = usePaletteModeUserPreference(); <Button active={mode === 'dark'} onClick={() => setMode('light')}>dark</Button> <Button active={mode === 'light'} onClick={() => setMode('dark')}>light</Button>

The nice thing about useLocalStorage is that it acts as global state, so you don't ever need to add a shared state variable somewhere and create a context for it. Just the hook works anywhere you use it, it handles state outside of React. This system worked very well for the Toolpad app theme switcher. I might be oversimplifying for the docs, I'd have to look deeper into it, there may be blockers to adopt the same way of doing a theme switcher in the core docs

@Janpot Yes but what I see is that the mode is a lot more integrated with MUI System once you use CSS Variables and Zero runtime than it was with Emotion (what Toolpad uses).

MUI System almost needs to completely own that logic. If it's possible to separate things then I'm 💯 for it. I had this feeling that hard to understand how things work because of how closely things are linked together but I don't really see how to simplify things. We have effectively built https://github.com/pacocoursey/next-themes.
cc @siriwatknp.

oliviertassinari · 2024-02-24T23:59:59Z

packages/mui-system/src/cssVars/useCurrentColorScheme.ts

-    return () => media.removeListener(handler);
+    return () => {
+      media.removeListener(handler);
+    };


I'm never clear on what's the return value of functions, makes it clear.

oliviertassinari · 2024-02-25T00:00:16Z

packages/mui-system/src/cssVars/useCurrentColorScheme.ts

+          // Early exit, nothing changed.
+          if (currentState.systemMode === systemMode) {


We would render twice, unnecessary.

oliviertassinari · 2024-02-25T00:00:39Z

packages/mui-system/src/cssVars/createCssVarsProvider.js

-        mode,
-        systemMode,
-        setMode,
-        lightColorScheme,
-        darkColorScheme,
+        allColorSchemes,
        colorScheme,
+        darkColorScheme,
+        lightColorScheme,
+        mode,
        setColorScheme,
-        allColorSchemes,
+        setMode,
+        systemMode,


Sorted to match the dependencies, it makes it easier to find missing value.

oliviertassinari · 2024-02-25T00:01:00Z

packages/mui-system/src/cssVars/createCssVarsProvider.js

+  function CssVarsProvider(props) {
+    const {
+      children,


For consistency in the codebase, makes it easy to see components to other stuff.

oliviertassinari · 2024-02-25T00:01:12Z

packages/mui-material-next/src/index.ts

@@ -1,5 +1,4 @@
 'use client';
-


oliviertassinari · 2024-02-25T00:01:26Z

docs/src/modules/components/ThemeContext.js

+        // No value changed
+        if (
+          (!action.payload.paletteMode || action.payload.paletteMode === state.paletteMode) &&
+          (!action.payload.direction || action.payload.direction === state.direction) &&
+          (!action.payload.paletteColors || action.payload.paletteColors === state.paletteColors)
+        ) {
+          return state;
+        }


Avoid redundant renders

oliviertassinari · 2024-02-25T00:01:51Z

docs/src/modules/components/ThemeContext.js

+        // TODO: have the value come from useColorScheme();
+        // paletteMode: nextPaletteMode,


This explains why code is gone.

oliviertassinari · 2024-02-25T00:02:35Z

docs/src/modules/components/MaterialYouUsageDemo.tsx

-function ModeSwitcher({ md2Mode }: { md2Mode: Mode }) {
-  const { setMode } = useColorScheme();
-  React.useEffect(() => {
-    setMode(md2Mode);
-  }, [md2Mode, setMode]);
-  return null;
-}
-


Either we use different modeStorageKey and attribute keys between Material UI and Material UI Next or we remove this.

Signed-off-by: Jan Potoms <2109932+Janpot@users.noreply.github.com>

…ling during hydration

…he right direction

Janpot · 2024-03-12T10:28:48Z

@oliviertassinari Thanks for the suggestions. I'm adding a few more changes:

The useLocalStorageState hook was changed to always return null during ssr, but the types weren't reflecting this (it returned string when initializer is defined and string | null when initalizer is not defined). I'm ok with the change, but removed the overloads so that the return type correctly reflects to possible values. See dd935db
I'm removing the changeTheme effect for the mode from the individual components and centralizing in ThemeContext in the existing effect that returns for paletteColors. See cd4e274
I'm adding a useColorSchemeShim to avoid duplicate logic and to ease future refactor towards useColorScheme. See cd4e274

oliviertassinari · 2024-04-16T00:10:22Z

@Janpot Nice, cleaner 👍

I introduced a regression:

Screen.Recording.2024-04-16.at.01.54.11.mov

https://deploy-preview-41223--material-ui.netlify.app/material-ui/getting-started/installation/

Fixed in #41917.

Janpot added regression A bug, but worse docs Improvements or additions to the documentation labels Feb 21, 2024

Janpot marked this pull request as ready for review February 21, 2024 14:50

Janpot requested review from siriwatknp and oliviertassinari February 21, 2024 14:50

oliviertassinari reviewed Feb 21, 2024

View reviewed changes

packages/mui-utils/src/useLocalStorageState/useLocalStorageState.ts Outdated Show resolved Hide resolved

Janpot requested a review from oliviertassinari February 21, 2024 15:29

This was referenced Feb 24, 2024

[system][material-ui] Stabilize CssVarsProvider #41070

Open

[utils] Port useLocalStorageState hook from Toolpad #41096

Merged

oliviertassinari force-pushed the storageRegs branch from a3b7125 to b8cec51 Compare February 24, 2024 23:56

oliviertassinari added the bug 🐛 Something doesn't work label Feb 24, 2024

oliviertassinari reviewed Feb 25, 2024

View reviewed changes

Janpot and others added 14 commits February 25, 2024 02:52

[utils] Port useLocalStorageState hook from Toolpad (mui#41096)

edc3d33

Signed-off-by: Jan Potoms <2109932+Janpot@users.noreply.github.com>

Fix useStorageState regressions

babbcf8

Update useLocalStorageState.ts

3111b22

Update ThemeModeToggle.tsx

d5c5b00

Update ThemeModeToggle.tsx

ee0901e

Update useLocalStorageState.ts

977f7ed

Update useLocalStorageState.ts

ac7599c

Update useLocalStorageState.ts

4038038

Update useLocalStorageState.ts

0f37e1f

convention; no blank space here

ebdf2bc

convention

39ef9ba

need to return null for the first render as we migh need special hand…

4bfc8f6

…ling during hydration

simplify the logic

6041a93

add no-ssr

c22dc5c

oliviertassinari added 8 commits February 25, 2024 02:52

early exit for duplicated work

f234945

fix lousy code hygiene

6e58a21

omg really

9f9ddc3

remove console logs

d49bc1d

group related logic

3f94ebf

leave comments to make sure future iteration will take steps toward t…

b6c1a28

…he right direction

good to go

c209835

fix ci

6c9b1e9

oliviertassinari force-pushed the storageRegs branch from b8cec51 to 6c9b1e9 Compare February 25, 2024 01:53

Merge branch 'heads/upstream/master' into storageRegs

a1a54fd

oliviertassinari approved these changes Mar 3, 2024

View reviewed changes

Janpot added 3 commits March 12, 2024 09:43

Merge branch 'heads/upstream/master' into storageRegs

c363951

Fix types

dd935db

update

cd4e274

Janpot added 2 commits March 12, 2024 11:44

Update comment

371978b

Update useLocalStorageState.ts

e217b3d

Janpot merged commit da74af0 into mui:master Mar 18, 2024
23 checks passed

Janpot deleted the storageRegs branch March 18, 2024 09:52

This was referenced Apr 16, 2024

[docs-infra] Fix code block layout shift #41917

Merged

[system] useColorScheme should return null during hydration #41918

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[docs] Fix useStorageState regressions #41223

[docs] Fix useStorageState regressions #41223

Janpot commented Feb 21, 2024 •

edited

mui-bot commented Feb 21, 2024 •

edited

Janpot commented Feb 21, 2024 •

edited

oliviertassinari commented Feb 21, 2024 •

edited

Janpot commented Feb 21, 2024 •

edited

oliviertassinari Feb 24, 2024

Janpot Feb 26, 2024

oliviertassinari Feb 26, 2024 •

edited

Janpot Feb 26, 2024 •

edited

oliviertassinari Feb 26, 2024 •

edited

oliviertassinari Feb 24, 2024

oliviertassinari Feb 25, 2024

oliviertassinari Feb 25, 2024

oliviertassinari Feb 25, 2024

oliviertassinari Feb 25, 2024

oliviertassinari Feb 25, 2024

oliviertassinari Feb 25, 2024

oliviertassinari Feb 25, 2024 •

edited

Janpot commented Mar 12, 2024 •

edited

oliviertassinari commented Apr 16, 2024 •

edited

		// Early exit, nothing changed.
		if (currentState.systemMode === systemMode) {

		// TODO: have the value come from useColorScheme();
		// paletteMode: nextPaletteMode,

[docs] Fix useStorageState regressions #41223

[docs] Fix useStorageState regressions #41223

Conversation

Janpot commented Feb 21, 2024 • edited

mui-bot commented Feb 21, 2024 • edited

Netlify deploy preview

Bundle size report

Janpot commented Feb 21, 2024 • edited

oliviertassinari commented Feb 21, 2024 • edited

Janpot commented Feb 21, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari Feb 26, 2024 • edited

Choose a reason for hiding this comment

Janpot Feb 26, 2024 • edited

Choose a reason for hiding this comment

oliviertassinari Feb 26, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari Feb 25, 2024 • edited

Choose a reason for hiding this comment

Janpot commented Mar 12, 2024 • edited

oliviertassinari commented Apr 16, 2024 • edited

Janpot commented Feb 21, 2024 •

edited

mui-bot commented Feb 21, 2024 •

edited

Janpot commented Feb 21, 2024 •

edited

oliviertassinari commented Feb 21, 2024 •

edited

Janpot commented Feb 21, 2024 •

edited

oliviertassinari Feb 26, 2024 •

edited

Janpot Feb 26, 2024 •

edited

oliviertassinari Feb 26, 2024 •

edited

oliviertassinari Feb 25, 2024 •

edited

Janpot commented Mar 12, 2024 •

edited

oliviertassinari commented Apr 16, 2024 •

edited