theme-ui icon indicating copy to clipboard operation
theme-ui copied to clipboard

colors from outside of theme are not recomputed on initial render after Gatsby SSR

Open ehowey opened this issue 3 years ago • 12 comments

Describe the bug

Minimal Repo: https://github.com/ehowey/color-mode-bug Relevant code: https://github.com/ehowey/color-mode-bug/blob/main/src/pages/index.js Live site: https://condescending-blackwell-39cd84.netlify.app/

Previously I could use code like the following to dynamically update images in dark mode, e.g. setting invert(1) to invert an image for dark mode. This code is still working fine in development mode but is breaking production mode. I have created a minimal repo and a deploy to Netlify to test this. You can see in the console log it is properly catching the mode as dark but for some reason the text and images are not updating their css based on the color mode.

To test this make sure you are visiting the live site in incognito mode as it will correctly change the colors and image on the second load once the cache is set.

const [mode] = useColorMode()
const isDark = mode === "dark"

<GatsbyImage sx={{ filter: isDark ? "invert(1)" : "none"}} />
<Theme.p sx={{color: isDark ? "tomato" : "cornflowerblue"}}>Some text </Themed.p>

Expected behavior Previously isDark could be used like a boolean flag in the CSS to toggle different colors based on the color mode.

ehowey avatar Mar 26 '21 03:03 ehowey

Thanks for the issue and the reproduction @ehowey!

hasparus avatar Mar 26 '21 04:03 hasparus

This is definitely something I broke while fixing color modes flash — previously we always started with default color mode and you could see the colors (except background and text) blink, but in this case you need the flash.

When the website SSRs, "mode" is undefined, so isDark is false. On client side we update all CSS custom properties, so colors from your theme update.

Based on the quick fiddling I gave it today, sx is recomputed on the client on rehydration, but it doesn't affect the HTML. After changing color to a function with console.logs I can see it returns "tomato", but the text is still blue. :thinking: After setting key to Math.random, text is tomato.

This code is still working fine in development mode but is breaking production mode.

BTW You can set flags.DEV_SSR to true in your Gatsby config to see bugs like this in developent.

As a quick workaround, you could add the color to the theme, but I realize this is less than ideal.

hasparus avatar Mar 26 '21 05:03 hasparus

Thanks for looking into this. Just as an FYI - this shows up in development even with the DEV_SSR flag set to true, so not a reliable indicator of whether it is working correctly.

ehowey avatar Mar 27 '21 01:03 ehowey

@ehowey Here is my fix for this.

const SSRFix = colorMode === undefined ? undefined : 'light'
const trueColorMode = colorMode === SSRFix ? 'light' : 'dark'

That way, it'll work both locally and with SSR.

@hasparus Any way we can have the default color mode show up as undefined always? That way it would be consistent everywhere and we could just update the snippet in the docs.

fcisio avatar Apr 08 '21 19:04 fcisio

@hasparus Any way we can have the default color mode show up as undefined always? That way it would be consistent everywhere and we could just update the snippet in the docs.

The default should be undefined always. Isn't it? When we don't know anything about the color mode, we can only have undefined there.

Though, in the browser we read it from localStorage and light/dark preferences synchronously, so you will most likely have a color mode on initial render.

hasparus avatar Apr 08 '21 20:04 hasparus

I'll do some more tests, and make more complete specs.

But so far, when is set initialColorModeName: 'light' I get undefined after SSR / light in local.

fcisio avatar Apr 08 '21 21:04 fcisio

Just wondering where this stands on the priority list of fixes, or is it a bigger deal/longer term and I should implement the work around mentioned above?

ehowey avatar Apr 26 '21 01:04 ehowey

Just wondering where this stands on the priority list of fixes, or is it a bigger deal/longer term and I should implement the work around mentioned above?

Not sure! We've fixed Fast Refresh support & #1672 should improve performance here, but if you'd like to take on fixing this, we'd love to review/get it released quickly.

lachlanjc avatar Apr 26 '21 15:04 lachlanjc

@ehowey we already have a PR fixing this, so testing, possible fixes and releasing shouldn't take more than a few days

hasparus avatar Apr 27 '21 20:04 hasparus

Thank-you! Sorry I didn't mean for you all to feel rushed. I really appreciate all of your work on Theme-UI!

ehowey avatar Apr 28 '21 02:04 ehowey

No worries :) Though, I'd advise to go with the workaround.

hasparus avatar Apr 28 '21 19:04 hasparus

Hey @ehowey, just FYI. This make take us some more time.

hasparus avatar May 04 '21 07:05 hasparus