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

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

Open
ehowey opened this issue Mar 26, 2021 · 12 comments
Labels
bug Something isn't working SSR

Comments

@ehowey
Copy link

ehowey commented Mar 26, 2021

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.

@hasparus
Copy link
Member

hasparus commented Mar 26, 2021

Thanks for the issue and the reproduction @ehowey!

@hasparus hasparus self-assigned this Mar 26, 2021
@hasparus hasparus changed the title useColorMode not properly initializing in production site using v0.6 and Gatsby colors from outside of theme are not recomputed on initial render after Gatsby SSR Mar 26, 2021
@hasparus
Copy link
Member

hasparus commented Mar 26, 2021

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. 🤔
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 hasparus added the SSR label Mar 26, 2021
@ehowey
Copy link
Author

ehowey commented Mar 27, 2021

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.

@fcisio
Copy link
Collaborator

fcisio commented Apr 8, 2021

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

@hasparus hasparus removed their assignment Apr 8, 2021
@hasparus
Copy link
Member

hasparus commented Apr 8, 2021

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

@fcisio
Copy link
Collaborator

fcisio commented Apr 8, 2021

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.

@ehowey
Copy link
Author

ehowey commented Apr 26, 2021

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?

@lachlanjc
Copy link
Member

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.

@hasparus
Copy link
Member

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

@ehowey
Copy link
Author

ehowey commented Apr 28, 2021

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

@hasparus
Copy link
Member

hasparus commented Apr 28, 2021

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

@hasparus
Copy link
Member

hasparus commented May 4, 2021

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working SSR
Projects
None yet
Development

No branches or pull requests

4 participants