Expose CSS Variables for SSR to avoid FOUC #160

narinluangrath · 2022-07-26T23:02:57Z

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:

Modify the given theme prop to replace hard-coded properties with CSS variable placeholders (e.g. backgroundColor: '#000000' --> backgroundColor: var(--backgroundColor)).
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.
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

~~Add Docusaurus example to README~~ Fix FOUC of Prism Theme facebook/docusaurus#7867
Add Next.js example to README

…object

narinluangrath · 2022-07-26T23:08:33Z

src/components/__tests__/__snapshots__/Highlight.test.js.snap

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


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 · 2022-07-27T00:27:59Z

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

Context: FormidableLabs/prism-react-renderer#160

narinluangrath · 2022-08-01T06:41:55Z

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

gksander

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.

gksander · 2022-08-03T14:47:29Z

src/utils/generateScriptForSSR.js

+  `
+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();


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

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

gksander · 2022-08-03T14:56:08Z

README.md

+// theme you wish to render on the initial page load
+const getThemeIdFuncStr = `
+  () => (
+    window.localStorage.get('color-mode') === 'dark'


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.

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

gksander · 2022-08-03T14:58:44Z

README.md

+  setPreBodyComponents(
+    <script dangerouslySetInnerHTML={{ __html: codeToRunOnClient }} />
+  );


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.

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

Added a note about CSP.

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. :)

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!

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.

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. :)

gksander

LGTM

slorber

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.

slorber · 2022-08-10T10:54:47Z

src/components/Highlight.js

+      // 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
+          : {},


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?

@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 · 2022-08-11T23:31:34Z

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

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

lex111

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

nluangrath-godaddy added 8 commits July 25, 2022 22:13

chore(Highlight.js): fix

a450431

improv(utils): add themeWithCssVariables function

f4d294a

improv(themeToDict.js): add optional third arg for root styles

75c8cdf

test(Highlight.test.js): add additional data attribute for raw style …

c659b2c

…object

fix(themeToDict.js): got messed up by prettier

81b606f

refactor(Highlight.js): use css variables as placeholders

fe5f390

improv: add id field to PrismTheme type

98b52fb

feat: add SSR / FOUC support

aff1614

narinluangrath requested review from gksander and jpdriver July 26, 2022 23:03

narinluangrath commented Jul 26, 2022

View reviewed changes

fix(generateScriptForSSR.js): cant use nullish collesing or whatever

520a8eb

narinluangrath pushed a commit to narinluangrath/docusaurus that referenced this pull request Aug 1, 2022

improv(docusaurus-theme-classic): fix FOUC

de45681

Context: FormidableLabs/prism-react-renderer#160

narinluangrath mentioned this pull request Aug 1, 2022

Fix FOUC of Prism Theme facebook/docusaurus#7867

Draft

3 tasks

nluangrath-godaddy added 6 commits August 1, 2022 09:27

chore(index.d.ts): update type declaration file

595ab5e

chore: make PrismTheme.id required, variable renaming

ef2f8e7

fix(generateScriptForSSR.js): must quote strings in strinigified js

49da00a

fix(generateScriptForSSR): root isnt a global variable u dummy

82040cc

fix(generateScriptForSSR.js): illegal return u dummy

30b2d1e

docs(README.md): add Next.js example

9d7954a

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

chore(README.md): use string interpolation

efc23ca

gksander reviewed Aug 3, 2022

View reviewed changes

narinluangrath added 3 commits August 3, 2022 12:32

docs(README.md): clarify SSR docs

dcd4681

improv(generateScriptForSSR.js): wrap script with try/catch

bcff206

docs(README.md): add comment about CSP

ad4aae6

gksander approved these changes Aug 4, 2022

View reviewed changes

slorber reviewed Aug 10, 2022

View reviewed changes

lex111 reviewed Oct 28, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose CSS Variables for SSR to avoid FOUC #160

Expose CSS Variables for SSR to avoid FOUC #160

narinluangrath commented Jul 26, 2022 •

edited

Loading

narinluangrath Jul 26, 2022

jpdriver commented Jul 27, 2022

narinluangrath commented Aug 1, 2022

gksander left a comment

gksander Aug 3, 2022

narinluangrath Aug 3, 2022

narinluangrath Aug 3, 2022

gksander Aug 3, 2022

narinluangrath Aug 3, 2022

narinluangrath Aug 3, 2022

gksander Aug 3, 2022

narinluangrath Aug 3, 2022

narinluangrath Aug 3, 2022

ZimChristine Aug 3, 2022

narinluangrath Aug 4, 2022

gksander Aug 4, 2022

ZimChristine Aug 4, 2022

gksander left a comment

slorber left a comment

slorber Aug 10, 2022

narinluangrath Aug 11, 2022 •

edited

Loading

narinluangrath commented Aug 11, 2022

lex111 left a comment

Expose CSS Variables for SSR to avoid FOUC #160

Are you sure you want to change the base?

Expose CSS Variables for SSR to avoid FOUC #160

Conversation

narinluangrath commented Jul 26, 2022 • edited Loading

TODO

Choose a reason for hiding this comment

jpdriver commented Jul 27, 2022

narinluangrath commented Aug 1, 2022

gksander left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gksander left a comment

Choose a reason for hiding this comment

slorber left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

narinluangrath Aug 11, 2022 • edited Loading

Choose a reason for hiding this comment

narinluangrath commented Aug 11, 2022

lex111 left a comment

Choose a reason for hiding this comment

narinluangrath commented Jul 26, 2022 •

edited

Loading

narinluangrath Aug 11, 2022 •

edited

Loading