ThemeProvider: Fixes mismatch in rendered output for theming with server side rendering #1868

siddharthkp · 2022-02-17T10:49:37Z

Improves SSR for theming
Fixes Flash component renders differently in server vs client #1773
Does not completely fix but reduces the surface area of https://github.com/github/docs-internal/pull/24123
TODO: Update ThemeProvider docs

There's a lot going on here visually, which is hard to explain in text, so here's a Loom (12mins)

Bug

When using colorMode=auto with ThemeProvider,

<ThemeProvider colorMode="auto">
  <Flash variant="warning">Warning Flash</Flash>
</ThemeProvider>

colorMode resolves differently on the server and client:

function getSystemColorMode() {
  if (typeof window !== 'undefined' && window.matchMedia?.('(prefers-color-scheme: dark)')?.matches) {
    return 'night'
  } else {
    return 'day'
  }
}

On the client, we have access to the system preferences, so "auto" resolves to the OS settings. On the server, however, we don't have any of that so it always resolves to day. So if you have dark mode on your OS (or it's after sunset on automatic) you get different resolvedColorMode leading to the infamous warning of mismatch between server and client.

In development environment, this would simply break the page. On production, the error would be suppressed and the page would be stuck on what the server provided (always day/light mode).

Before:

color-mode-auto-before.mov

Short term fix (this pull request):

The short term fix for this to:

Remove the mismatch of output between client and server: Pass the resolvedColorMode from the server to the client and use that as the initial color mode. diff
Apply the correct theme after resolving color mode on client: Run an useEffect on the client to see if the resolvedColorMode on the client should be different than what we initially set and fix it. diff

I have made this feature opt in instead of the default so that we don't inject anything into the DOM by default, would like some feedback if that makes sense and what the prop should be called:

<ThemeProvider colorMode="auto" preventSSRMismatch>
  <Flash variant="warning">Warning Flash</Flash>
</ThemeProvider>

After:

color-mode-auto-after.mov

Long term fix (not in this PR):

The core of the problem is that we are not able to respond the client's resolved color mode instantly because we need to wait for the javascript bundle to download and generate new styles on runtime.

The fix for that would be to move towards css variables in components which can respond to changes in theme instantly without waiting for javascript.
To trigger that change, we would need to ship a tiny chunk of javascript embedded in the initial server rendered html that updates the color mode the moment it reaches the browser.

changeset-bot · 2022-02-17T10:49:39Z

🦋 Changeset detected

Latest commit: 1a30ee2

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@primer/react	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

github-actions · 2022-02-17T10:52:34Z

size-limit report 📦

Path	Size
dist/browser.esm.js	62.78 KB (+0.25% 🔺)
dist/browser.umd.js	63.17 KB (+0.26% 🔺)

siddharthkp · 2022-02-17T11:16:11Z

src/ThemeProvider.tsx

@@ -17,6 +17,7 @@ export type ThemeProviderProps = {
  colorMode?: ColorModeWithAuto
  dayScheme?: string
  nightScheme?: string
+  preventServerClientMismatch?: boolean


Start here, Step: 1/5. Add opt-in feature to prevent output mismatch between server and client. If it's a client only application, they don't need to bother.

next step 2/5 →

siddharthkp · 2022-02-17T11:19:05Z

src/ThemeProvider.tsx

+        {children}
+        {props.preventServerClientMismatch ? (
+          <script dangerouslySetInnerHTML={{__html: `__PRIMER_RESOLVED_SERVER_COLOR_MODE='${resolvedColorMode}'`}} />
+        ) : null}


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.

next step 3/5 ↑

siddharthkp · 2022-02-17T11:21:38Z

src/ThemeProvider.tsx

  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)


Step 4/5: For rehydration on the client, if there was a resolvedColorModePassthrough passed on from the server, just use that instead of resolving color mode on the client. This will ensure consistent output between client and server.

last step 5/5 →

siddharthkp · 2022-02-17T11:22:43Z

src/ThemeProvider.tsx

+  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
+  )


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 →

siddharthkp · 2022-02-17T11:25:34Z

src/ThemeProvider.tsx

+  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(() => {
+            // 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)
+          })
+        }
+
+        resolvedColorModePassthrough.current = null
+      }
+    },
+    [colorMode, systemColorMode]
+  )


Step 5/5: After mount - If the resolvedColorMode passed from the server and the resolvedColorMode resolved on the client do not match, update the color mode to respect the client's settings (example: If the color mode is set to "auto" and it resolved to "night" on the client side but was sent as "day" from the server)

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 colorMode to what the application has set, so that the UI can respond to changes in OS settings immediately.

colebemis

This is a great short-term fix! Thanks for the thorough PR description and comments. They were really helpful ❤️

colebemis · 2022-02-17T22:56:19Z

Let's update the ThemeProvider docs in this PR too

siddharthkp · 2022-02-21T14:53:36Z

@colebemis Added docs, also renamed the prop to a nicer preventSSRMismatch

pksjce · 2022-02-22T03:59:24Z

Great explanation of the issue! I've seen similar fixes for rehydration issues where we pass data btw server and client through the html.
One thing is I don't understand the need for a boolean. I guess you want to use it as a feature flag?
Either ways great job! ✅

siddharthkp · 2022-02-22T09:42:03Z

One thing is I don't understand the need for a boolean. I guess you want to use it as a feature flag?

Yep, making it opt-in instead of enabling it by default for the imperfect short term solution.

rezrah · 2022-02-22T09:59:16Z

src/ThemeProvider.tsx

+            // 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)


Would this stop working when we eventually upgrade to React 18 with its batched updates?

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?

I honestly don't know, something to watch out for when the time comes 🤔

The fix would be to use flushSync then. See "What if I don’t want to batch?" under reactwg/react-18#21

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.

rezrah · 2022-02-22T09:59:50Z

src/ThemeProvider.tsx

+      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(() => {


Do you need to clear this timeout?

Don't think so :) This is only called once, ever.

rezrah · 2022-02-22T10:09:01Z

Agree with the others. Great PR @siddharthkp.. really appreciated the detailed descriptions too 🙇

colebemis

🚢

docs/content/theming.md

Co-authored-by: Cole Bemis <colebemis@github.com>

mikesurowiec · 2022-02-23T22:31:52Z

@siddharthkp great job on this! I really appreciate the fix 🙏

siddharthkp added 2 commits February 16, 2022 18:18

wip commit

ecfcc38

remove demo logs

17d658a

Add changelog

4affc1f

github-actions bot deployed to Preview February 17, 2022 11:04 View deployment

Merge branch 'main' into improve-ssr-theming

0d5fc5a

github-actions bot deployed to Preview February 17, 2022 11:08 View deployment

Make this feature opt-in

fb41bac

github-actions bot deployed to Preview February 17, 2022 11:13 View deployment

siddharthkp commented Feb 17, 2022

View reviewed changes

siddharthkp marked this pull request as ready for review February 17, 2022 11:32

siddharthkp requested review from a team, pksjce and colebemis and removed request for pksjce February 17, 2022 11:32

siddharthkp self-assigned this Feb 17, 2022

siddharthkp added bug Something isn't working react labels Feb 17, 2022

siddharthkp modified the milestone: FY22 - Q3 Feb 17, 2022

siddharthkp changed the title ~~Improve ssr theming~~ Fixes mismatch for theming with server side rendering Feb 17, 2022

siddharthkp changed the title ~~Fixes mismatch for theming with server side rendering~~ Fixes mismatch in rendered output for theming with server side rendering Feb 17, 2022

colebemis approved these changes Feb 17, 2022

View reviewed changes

rename + add docs for preventSSRMismatch

b381898

Merge branch 'main' into improve-ssr-theming

792c56e

github-actions bot deployed to Preview February 21, 2022 14:53 View deployment

siddharthkp requested a review from colebemis February 21, 2022 14:53

siddharthkp assigned colebemis Feb 21, 2022

pksjce approved these changes Feb 22, 2022

View reviewed changes

Merge branch 'main' into improve-ssr-theming

3f8e6c0

github-actions bot deployed to Preview February 22, 2022 09:42 View deployment

rezrah reviewed Feb 22, 2022

View reviewed changes

colebemis approved these changes Feb 22, 2022

View reviewed changes

docs/content/theming.md Outdated Show resolved Hide resolved

siddharthkp and others added 2 commits February 23, 2022 12:46

copy change for docs

54b3590

Co-authored-by: Cole Bemis <colebemis@github.com>

Merge branch 'main' into improve-ssr-theming

1a30ee2

github-actions bot deployed to Preview February 23, 2022 11:46 View deployment

siddharthkp enabled auto-merge (squash) February 23, 2022 11:46

github-actions bot deployed to Preview February 23, 2022 11:46 View deployment

siddharthkp merged commit f3f4255 into main Feb 23, 2022

siddharthkp deleted the improve-ssr-theming branch February 23, 2022 11:52

primer-css mentioned this pull request Feb 23, 2022

Release Tracking #1859

Merged

siddharthkp mentioned this pull request Mar 1, 2022

ThemeProvider: Fix theming with SSR with CSRF safe approach #1913

Merged

siddharthkp mentioned this pull request Mar 8, 2022

ThemeProvider: Fix setColorMode #1936

Merged

siddharthkp mentioned this pull request May 12, 2022

Create contributing-docs/debt.md #2071

Closed

3 tasks

siddharthkp changed the title ~~Fixes mismatch in rendered output for theming with server side rendering~~ ThemeProvider: Fixes mismatch in rendered output for theming with server side rendering Jun 3, 2022

aprendendofelipe mentioned this pull request Jun 5, 2022

feat(dark-mode): adiciona suporte ao dark mode filipedeschamps/tabnews.com.br#377

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ThemeProvider: Fixes mismatch in rendered output for theming with server side rendering #1868

ThemeProvider: Fixes mismatch in rendered output for theming with server side rendering #1868

siddharthkp commented Feb 17, 2022 •

edited

Loading

changeset-bot bot commented Feb 17, 2022 •

edited

Loading

github-actions bot commented Feb 17, 2022 •

edited

Loading

siddharthkp Feb 17, 2022 •

edited

Loading

siddharthkp Feb 17, 2022 •

edited

Loading

siddharthkp Feb 17, 2022 •

edited

Loading

siddharthkp Feb 17, 2022 •

edited

Loading

siddharthkp Feb 17, 2022 •

edited

Loading

colebemis left a comment

colebemis commented Feb 17, 2022

siddharthkp commented Feb 21, 2022

pksjce commented Feb 22, 2022

siddharthkp commented Feb 22, 2022

rezrah Feb 22, 2022

rezrah Feb 22, 2022

siddharthkp Feb 22, 2022 •

edited

Loading

rezrah Feb 22, 2022

rezrah Feb 22, 2022

siddharthkp Feb 22, 2022

rezrah commented Feb 22, 2022

colebemis left a comment

mikesurowiec commented Feb 23, 2022

ThemeProvider: Fixes mismatch in rendered output for theming with server side rendering #1868

ThemeProvider: Fixes mismatch in rendered output for theming with server side rendering #1868

Conversation

siddharthkp commented Feb 17, 2022 • edited Loading

Bug

Short term fix (this pull request):

Long term fix (not in this PR):

changeset-bot bot commented Feb 17, 2022 • edited Loading

🦋 Changeset detected

github-actions bot commented Feb 17, 2022 • edited Loading

size-limit report 📦

siddharthkp Feb 17, 2022 • edited Loading

Choose a reason for hiding this comment

siddharthkp Feb 17, 2022 • edited Loading

Choose a reason for hiding this comment

siddharthkp Feb 17, 2022 • edited Loading

Choose a reason for hiding this comment

siddharthkp Feb 17, 2022 • edited Loading

Choose a reason for hiding this comment

siddharthkp Feb 17, 2022 • edited Loading

Choose a reason for hiding this comment

colebemis left a comment

Choose a reason for hiding this comment

colebemis commented Feb 17, 2022

siddharthkp commented Feb 21, 2022

pksjce commented Feb 22, 2022

siddharthkp commented Feb 22, 2022

rezrah Feb 22, 2022

Choose a reason for hiding this comment

rezrah Feb 22, 2022

Choose a reason for hiding this comment

siddharthkp Feb 22, 2022 • edited Loading

Choose a reason for hiding this comment

rezrah Feb 22, 2022

Choose a reason for hiding this comment

rezrah Feb 22, 2022

Choose a reason for hiding this comment

siddharthkp Feb 22, 2022

Choose a reason for hiding this comment

rezrah commented Feb 22, 2022

colebemis left a comment

Choose a reason for hiding this comment

mikesurowiec commented Feb 23, 2022

siddharthkp commented Feb 17, 2022 •

edited

Loading

changeset-bot bot commented Feb 17, 2022 •

edited

Loading

github-actions bot commented Feb 17, 2022 •

edited

Loading

siddharthkp Feb 17, 2022 •

edited

Loading

siddharthkp Feb 17, 2022 •

edited

Loading

siddharthkp Feb 17, 2022 •

edited

Loading

siddharthkp Feb 17, 2022 •

edited

Loading

siddharthkp Feb 17, 2022 •

edited

Loading

siddharthkp Feb 22, 2022 •

edited

Loading