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

Expose CSS Variables for SSR to avoid FOUC #160

Open
wants to merge 19 commits into
base: master
Choose a base branch
from

Conversation

narinluangrath
Copy link
Contributor

@narinluangrath narinluangrath commented Jul 26, 2022

Fixes #149 .

This solution is inspired by the article that @jpdriver linked in the above issue. There are no breaking changes. The solution works as follow:

  1. Modify the given theme prop to replace hard-coded properties with CSS variable placeholders (e.g. backgroundColor: '#000000' --> backgroundColor: var(--backgroundColor)).
  2. On the server, inject a script tag that defines those CSS variables (e.g. root.style.setProperty('--backgroundColor', '#000000)). These CSS variables will only be used for the first render.
  3. On the client, after the first render, override those CSS variables to use the theme stored in React state.

It's a little confusing, but I think it's the simplest solution to solve this tricky problem. I'm very much open to suggestions or even rewriting the entire PR. It will probably help your understanding if you read this PR commit by commit.

TODO

>
<span
class="token punctuation"
data-style="{\\"color\\":\\"#6c6783\\"}"
style="color: rgb(108, 103, 131);"
data-style="{\\"color\\":\\"var(--punctuation-color)\\"}"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This snapshot is incorrect. We didn't actually delete the style attribute from this span like the diff suggests. react-test-renderrer just flips out when you include CSS variable references like var(--my-variable) and decides not to render the style attribute. If you inspect the DOM in a working example, you'll see the style attribute.

@jpdriver
Copy link
Contributor

this is really exciting @narinluangrath -- thank you so much for picking this up! one thing i would say from a demo / example perspective and to help us all get on the same page with the review, we should probably add a Docusaurus-based example.

this work was called out as something we specifically need to add to help those folks out (and in turn Formidable's own Docusaurus sites!)

would be great if we can all see what that would look like in practice

narinluangrath pushed a commit to narinluangrath/docusaurus that referenced this pull request Aug 1, 2022
@narinluangrath
Copy link
Contributor Author

this is really exciting @narinluangrath -- thank you so much for picking this up! one thing i would say from a demo / example perspective and to help us all get on the same page with the review, we should probably add a Docusaurus-based example.

this work was called out as something we specifically need to add to help those folks out (and in turn Formidable's own Docusaurus sites!)

would be great if we can all see what that would look like in practice

@jpdriver facebook/docusaurus#7867

@narinluangrath narinluangrath marked this pull request as ready for review August 2, 2022 17:07
Copy link
Contributor

@gksander gksander left a comment

Choose a reason for hiding this comment

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

Overall, this looks great to me. I don't have much context on this though, so I'm hoping to also solicit some of @jpdriver's expertise here just to make sure I'm not overlooking anything.

Comment on lines 10 to 29
`
const themeId = (${getThemeIdFuncStr})();

const root = document.documentElement;

${themes
.map(
(theme) =>
`if (themeId === '${theme.id || ""}') {
${Object.entries(themeWithCssVariables(theme).variables)
.map(
([key, value]) =>
// $FlowFixMe
`root.style.setProperty('${key}', '${value || ""}');`
)
.join("\n" + " ".repeat(2))}
}`
)
.join("\n\n")}
`.trim();
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to wrap this in a try/catch so that if anything happens here, we don't have unexpected side-effects? (Like, I'd hate for something to boop here and then crash the whole page or something – but maybe that's not a realistic scenario.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, a try/catch wouldn't hurt! I'll add it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

README.md Outdated
// theme you wish to render on the initial page load
const getThemeIdFuncStr = `
() => (
window.localStorage.get('color-mode') === 'dark'
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to add a note about how this should match up with how the end-user implements storing the selected theme ID? E.g., if they use window.localStorage.get('color-mode') to derive color theme ID, then they'll need to update that localStorage key whenever their theme choice changes?

I can just imagine someone reading this, implementing this exact code, and then wondering why they're still seeing FOUC.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah! That's a great call out. I'll add some more example code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Comment on lines +421 to +423
setPreBodyComponents(
<script dangerouslySetInnerHTML={{ __html: codeToRunOnClient }} />
);
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if we have any way around this, but if end-user is using any sort of Content Security Policy header to, say, restrict inline scripts – then this will likely fail. I don't think very many end-users would be doing that, but just wanted to point that out as a potential failure point here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes, I totally didn't think about that. I'll add a warning about it and a way to work with CSP.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a note about CSP.

Choose a reason for hiding this comment

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

Should we sanitize codeToRunOnClient? I see that we are sometimes getting theme values from localStorage, which means the theme data isn't guaranteed to be as expected.

Forgive me if this comment is naive or intrusive. I am in the middle of security training which is why the dangers of dangerouslySetInnerHTML are at the front of my mind. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @ZimChristine, thanks for the question! I think this code is fine because I believe the worst thing that could happen is we set the wrong CSS variables, but I would love to have a second opinion!

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll chime in here. I think this is a very valid question! I think in this case, we'll be okay – since the code that we're "dangerously" setting is generated from the site developer and not from users. Generally, injection attacks occur when you fail to sanitize user-provided data. In this case, the code we're "injecting" here is provided explicitly by the site developer, and is not really dynamic – so if there were some sort of shenanigans going on, it'd be coming from the site developer themselves, and there's not much we can do about that.

Choose a reason for hiding this comment

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

Right on! Thanks for the reply. Agreed that for the current usage this dangerously set html is protected from an injection attack; glad for the dialogue. :)

Copy link
Contributor

@gksander gksander left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@slorber slorber left a comment

Choose a reason for hiding this comment

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

Really looking forward to integrate this in Docusaurus and get rid of the code block hydration FOUC :)


Was wondering: why not always using CSS variables in the first place, instead of just for the 1st render? I don't like too much the idea of having 2 different paths for 1st render vs others, the UX could easily get out of sync over time.

Doesn't it make sense to create a unique "prism-css-variable" theme and create themes that just set the appropriate CSS variables.

Instead of using inline styles everywhere, it would be possible to use classes + set CSS variables once at the top? It might also give a lighter DOM structure.

Comment on lines +176 to +183
// Omit loading CSS variable declarations during the first render.
// That way, the consumer can override the CSS variable declarations
// via `generateScriptTagForSSR` for the very first render. After that
// client side CSS variables will be used.
style:
themeDict !== undefined && !this.state.isFirstRender
? themeDict.root
: {},
Copy link
Contributor

Choose a reason for hiding this comment

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

just wondering, wouldn't this be a problem for users that are not using generateScriptForSSR yet?

It's better to have the wrong theme style than no style at all?

Copy link
Contributor Author

@narinluangrath narinluangrath Aug 11, 2022

Choose a reason for hiding this comment

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

@slorber Good questions

On line 40, we set this.state.isFirstRender to true immediately. For my initial testing, the state change has been fast enough that I never even see the unstyled content (even without generateScriptForSSR).

Let me know if you have follow up questions. This piece of code is rather awkward, but it's the best solution I could find.

@narinluangrath
Copy link
Contributor Author

@slorber

Was wondering: why not always using CSS variables in the first place, instead of just for the 1st render? I don't like too much the idea of having 2 different paths for 1st render vs others, the UX could easily get out of sync over time.

So we actually use CSS variables for both the 1st render and the subsequent client side renders. The different between the first render and the second is where the CSS variables are defined. In the first render (assuming you're using generateScriptForSSR) they are defined in a <script /> that you've injected into the <body />. For the subsequent renders, those CSS variables are overwritten in the style attribute of top level Prism HTML element (<pre />, in the screenshot below).

image

Doesn't it make sense to create a unique "prism-css-variable" theme and create themes that just set the appropriate CSS variables.

Instead of using inline styles everywhere, it would be possible to use classes + set CSS variables once at the top? It might also give a lighter DOM structure.

Let me think about this and get back to you. Just to be clear, this PR uses inline styles and CSS variables (you don't have to pick one or the other).

Copy link
Contributor

@lex111 lex111 left a comment

Choose a reason for hiding this comment

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

Nice feature 🎉 , any progress to merge this PR soon?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

expose CSS variables to avoid FOUC during SSR
7 participants