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

[docs] Fix useStorageState regressions #41223

Merged
merged 28 commits into from
Mar 18, 2024
Merged

[docs] Fix useStorageState regressions #41223

merged 28 commits into from
Mar 18, 2024

Conversation

Janpot
Copy link
Member

@Janpot Janpot commented Feb 21, 2024

Addresses #41096 (comment)

  • 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/

@Janpot Janpot added regression A bug, but worse docs Improvements or additions to the documentation labels Feb 21, 2024
@mui-bot
Copy link

mui-bot commented Feb 21, 2024

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

@Janpot Janpot marked this pull request as ready for review February 21, 2024 14:50
@Janpot
Copy link
Member Author

Janpot commented Feb 21, 2024

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
Copy link
Member

oliviertassinari commented Feb 21, 2024

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:

SCR-20240221-oups

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 #42750. 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
Copy link
Member Author

Janpot commented Feb 21, 2024

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.

// 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) {
Copy link
Member

Choose a reason for hiding this comment

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

Moved this to wrap the maximum possible scope.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

@oliviertassinari oliviertassinari Feb 26, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@Janpot Janpot Feb 26, 2024

Choose a reason for hiding this comment

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

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

Copy link
Member

@oliviertassinari oliviertassinari Feb 26, 2024

Choose a reason for hiding this comment

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

@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.

Comment on lines -282 to +288
return () => media.removeListener(handler);
return () => {
media.removeListener(handler);
};
Copy link
Member

Choose a reason for hiding this comment

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

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

Comment on lines +262 to +263
// Early exit, nothing changed.
if (currentState.systemMode === systemMode) {
Copy link
Member

Choose a reason for hiding this comment

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

We would render twice, unnecessary.

Comment on lines -251 to +259
mode,
systemMode,
setMode,
lightColorScheme,
darkColorScheme,
allColorSchemes,
colorScheme,
darkColorScheme,
lightColorScheme,
mode,
setColorScheme,
allColorSchemes,
setMode,
systemMode,
Copy link
Member

Choose a reason for hiding this comment

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

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

Comment on lines +63 to +65
function CssVarsProvider(props) {
const {
children,
Copy link
Member

Choose a reason for hiding this comment

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

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Convention

Comment on lines +150 to +157
// 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;
}
Copy link
Member

Choose a reason for hiding this comment

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

Avoid redundant renders

Comment on lines 186 to 187
// TODO: have the value come from useColorScheme();
// paletteMode: nextPaletteMode,
Copy link
Member

Choose a reason for hiding this comment

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

This explains why code is gone.

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

Copy link
Member

@oliviertassinari oliviertassinari Feb 25, 2024

Choose a reason for hiding this comment

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work docs Improvements or additions to the documentation regression A bug, but worse
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants