-
Notifications
You must be signed in to change notification settings - Fork 152
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
base: master
Are you sure you want to change the base?
Changes from all commits
a450431
f4d294a
75c8cdf
c659b2c
81b606f
fe5f390
98b52fb
aff1614
520a8eb
595ab5e
ef2f8e7
49da00a
82040cc
30b2d1e
9d7954a
efc23ca
dcd4681
bcff206
ad4aae6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,6 +3,7 @@ | |
import React, { Component, type Node } from "react"; | ||
import normalizeTokens from "../utils/normalizeTokens"; | ||
import themeToDict, { type ThemeDict } from "../utils/themeToDict"; | ||
import themeWithCssVariables from "../utils/themeWithCssVariables"; | ||
|
||
import type { | ||
Language, | ||
|
@@ -16,6 +17,7 @@ import type { | |
PrismLib, | ||
PrismTheme, | ||
PrismToken, | ||
StyleObj, | ||
} from "../types"; | ||
|
||
type Props = { | ||
|
@@ -30,6 +32,13 @@ class Highlight extends Component<Props, *> { | |
prevTheme: PrismTheme | void; | ||
prevLanguage: Language | void; | ||
themeDict: ThemeDict | void; | ||
state = { | ||
isFirstRender: true, | ||
}; | ||
|
||
componentDidMount() { | ||
this.setState({ isFirstRender: false }); | ||
} | ||
|
||
getThemeDict = (props: Props): ThemeDict | void => { | ||
if ( | ||
|
@@ -42,10 +51,13 @@ class Highlight extends Component<Props, *> { | |
|
||
this.prevTheme = props.theme; | ||
this.prevLanguage = props.language; | ||
|
||
const themeDict = props.theme | ||
? themeToDict(props.theme, props.language) | ||
: undefined; | ||
let themeDict; | ||
if (props.theme) { | ||
// Replace CSS Values with CSS Variable placeholders | ||
// This is necessary for SSR support | ||
const { theme, variables } = themeWithCssVariables(props.theme); | ||
themeDict = themeToDict(theme, props.language, variables); | ||
} | ||
return (this.themeDict = themeDict); | ||
}; | ||
|
||
|
@@ -79,7 +91,7 @@ class Highlight extends Component<Props, *> { | |
return output; | ||
}; | ||
|
||
getStyleForToken = ({ types, empty }: Token) => { | ||
getStyleForToken = ({ types, empty }: Token): StyleObj | void => { | ||
const typesSize = types.length; | ||
const themeDict = this.getThemeDict(this.props); | ||
|
||
|
@@ -92,7 +104,6 @@ class Highlight extends Component<Props, *> { | |
} | ||
|
||
const baseStyle = empty ? { display: "inline-block" } : {}; | ||
// $FlowFixMe | ||
const typeStyles = types.map((type) => themeDict[type]); | ||
return Object.assign(baseStyle, ...typeStyles); | ||
}; | ||
|
@@ -162,7 +173,14 @@ class Highlight extends Component<Props, *> { | |
return children({ | ||
tokens, | ||
className: `prism-code language-${language}`, | ||
style: themeDict !== undefined ? themeDict.root : {}, | ||
// 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 | ||
: {}, | ||
Comment on lines
+176
to
+183
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 It's better to have the wrong theme style than no style at all? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @slorber Good questions On line 40, we set 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. |
||
getLineProps: this.getLineProps, | ||
getTokenProps: this.getTokenProps, | ||
}); | ||
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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. :)There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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. :)