-
Notifications
You must be signed in to change notification settings - Fork 616
ThemeProvider: Fixes mismatch in rendered output for theming with server side rendering #1868
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
Changes from all commits
ecfcc38
17d658a
4affc1f
0d5fc5a
fb41bac
b381898
792c56e
3f8e6c0
54b3590
1a30ee2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
--- | ||
'@primer/react': patch | ||
--- | ||
|
||
Fixes a bug for theming with server side rendering where the output of the server and client mismatch [#1773](https://github.com/primer/react/issues/1773) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,6 +17,7 @@ export type ThemeProviderProps = { | |
colorMode?: ColorModeWithAuto | ||
dayScheme?: string | ||
nightScheme?: string | ||
preventSSRMismatch?: boolean | ||
} | ||
|
||
const ThemeContext = React.createContext<{ | ||
|
@@ -47,21 +48,50 @@ export const ThemeProvider: React.FC<ThemeProviderProps> = ({children, ...props} | |
|
||
// Initialize state | ||
const theme = props.theme ?? fallbackTheme ?? defaultTheme | ||
|
||
const resolvedColorModePassthrough = React.useRef( | ||
// eslint-disable-next-line @typescript-eslint/ban-ts-comment | ||
// @ts-ignore This custom variable does not exist on window because we set it outselves | ||
typeof window !== 'undefined' ? window.__PRIMER_RESOLVED_SERVER_COLOR_MODE : undefined | ||
) | ||
|
||
const [colorMode, setColorMode] = React.useState(props.colorMode ?? fallbackColorMode ?? defaultColorMode) | ||
const [dayScheme, setDayScheme] = React.useState(props.dayScheme ?? fallbackDayScheme ?? defaultDayScheme) | ||
const [nightScheme, setNightScheme] = React.useState(props.nightScheme ?? fallbackNightScheme ?? defaultNightScheme) | ||
const systemColorMode = useSystemColorMode() | ||
const resolvedColorMode = resolveColorMode(colorMode, systemColorMode) | ||
const resolvedColorMode = resolvedColorModePassthrough.current || resolveColorMode(colorMode, systemColorMode) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Step 4/5: For rehydration on the client, if there was a |
||
const colorScheme = chooseColorScheme(resolvedColorMode, dayScheme, nightScheme) | ||
const {resolvedTheme, resolvedColorScheme} = React.useMemo( | ||
() => applyColorScheme(theme, colorScheme), | ||
[theme, colorScheme] | ||
) | ||
|
||
// this effect will only run on client | ||
React.useEffect( | ||
function updateColorModeAfterServerPassthorugh() { | ||
const resolvedColorModeOnClient = resolveColorMode(colorMode, systemColorMode) | ||
|
||
if (resolvedColorModePassthrough.current) { | ||
// if the resolved color mode passed on from the server is not the resolved color mode on client, change it! | ||
if (resolvedColorModePassthrough.current !== resolvedColorModeOnClient) { | ||
window.setTimeout(() => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you need to clear this timeout? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't think so :) This is only called once, ever. |
||
// override colorMode to whatever is resolved on the client to get a re-render | ||
setColorMode(resolvedColorModeOnClient) | ||
// immediately after that, set the colorMode to what the user passed to respond to system color mode changes | ||
setColorMode(colorMode) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would this stop working when we eventually upgrade to React 18 with its batched updates? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also just curious, this feels like a potential race condition. Rather than relying on the same effect to trigger the second rerender, could this have its own? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I honestly don't know, something to watch out for when the time comes 🤔 The fix would be to use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I suspect we (and everyone else) won't have a choice to begin with. We've been working around it for years now. In longer term, I imagine we'd want to enable it though. |
||
}) | ||
} | ||
|
||
resolvedColorModePassthrough.current = null | ||
} | ||
}, | ||
[colorMode, systemColorMode] | ||
) | ||
Comment on lines
+70
to
+89
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Step 5/5: After mount - If the Note: After 'fixing" the color mode, we switch over to "client mode". To do this, we unset the ref set in step 3 and reset the |
||
|
||
// Update state if props change | ||
React.useEffect(() => { | ||
setColorMode(props.colorMode ?? fallbackColorMode ?? defaultColorMode) | ||
}, [props.colorMode, fallbackColorMode]) | ||
}, [props.colorMode, resolvedColorMode, fallbackColorMode]) | ||
|
||
React.useEffect(() => { | ||
setDayScheme(props.dayScheme ?? fallbackDayScheme ?? defaultDayScheme) | ||
|
@@ -86,7 +116,12 @@ export const ThemeProvider: React.FC<ThemeProviderProps> = ({children, ...props} | |
setNightScheme | ||
}} | ||
> | ||
<SCThemeProvider theme={resolvedTheme}>{children}</SCThemeProvider> | ||
<SCThemeProvider theme={resolvedTheme}> | ||
{children} | ||
{props.preventSSRMismatch ? ( | ||
<script dangerouslySetInnerHTML={{__html: `__PRIMER_RESOLVED_SERVER_COLOR_MODE='${resolvedColorMode}'`}} /> | ||
) : null} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Step 2/5: If the prop is present, inject a tiny script into server-rendered html to set a variable for the client side ThemeProvider to read after rehydration. |
||
</SCThemeProvider> | ||
</ThemeContext.Provider> | ||
) | ||
} | ||
|
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Step 3/5: On the client side, check if there was a variable set by the server, pick it up and set it in a
ref
. We will use this later.next step 4/5 →