react feat(ThemeProvider): add custom `script` prop to override `dangerouslySetInnerHTML`

Closes https://github.com/github/primer/issues/1049

This PR introduces an approach to avoid using dangerouslySetInnerHTML if it's supported in the SSR framework.

Specifically, it introduces a new prop script that allows consumers to provide a built-in Script component. This component can then use the built-in way for frameworks to render trusted content with <script> tags such that their content is not sanitized.

Note: by default any content of a <script> tag will be sanitized and will result in a hydration DOM mismatch error.

This approach is opt-in, so any existing usage will continue to use the dangerouslySetInnerHTML path. For teams looking to use script there should be an example in our docs (will add in if this is the approach we want to take!) that looks like the following:

import Script from 'next/script'
import {ThemeProvider, ThemeScriptProps} from '@primer/react/ThemeProvider'

function ThemeScript(props: ThemeScriptProps) {
  return <Script {...props} strategy="beforeInteractive" />
}

export default function App({Component, pageProps}) {
  return (
    <ThemeProvider preventSSRMismatch script={ThemeScript}>
      <Component {...pageProps} />
    </ThemeProvider>
  )
}

Alternatives

Outside of providing a script prop for interop, we could also render an element that is not a script and place content within there. For example:

<SCThemeProvider theme={resolvedTheme}>
  {children}
  {props.preventSSRMismatch ? (
    <div id="__PRIMER_DATA__" hidden>
      {JSON.stringify({resolvedServerColorMode: resolvedColorMode})}
    </div>
  ) : null}
</SCThemeProvider>

This approach will not receive the same sanitization as a <script> would and would be accessible during hydration by the component.

I'm not quite sure what the drawbacks of this approach would be. The only discernible different to me is that this element would be rendered where the ThemeProvider is used, where-as a Script from next.js with stategy="beforeInteractive" would be placed in <head>.

Would love feedback on these different approaches! There is also a Next.js example in this PR that can be used to try out the different patterns 👀

Sep 14 '22 21:09 joshblack

⚠️ No Changeset found

Latest commit: ab8cc754b23b2682cf9b1ea6e7c97a87fc4ff340

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Sep 14 '22 21:09 changeset-bot[bot]

size-limit report 📦

Path	Size
dist/browser.esm.js	77.16 KB (+0.08% 🔺)
dist/browser.umd.js	77.77 KB (+0.09% 🔺)

Sep 14 '22 21:09 github-actions[bot]

@colebemis love these questions 🔥

Should we add documentation for the script prop in the Theming section of the Primer React docs?

Definitely 👍

Will the SSR approaches we're exploring in Dotcom be compatible with this approach?

Would there be a good point-of-contact or channel I could reach out in to double-check?

Would using CSS variables for theming eliminate the need for this script entirely?

I'm not sure, I think if you need to know the theme during rendering then you'll need some kind of bridge to keep things in sync. If you can keep things CSS-only then I think you'd be 100% good to go without it, but if you ever need to change what you render depending on the theme (like illustration) I think you'll end up needing this.

The bridge can definitely take on different shapes though, probably the simplest are a class name (.theme-<theme-name>) or a data-attribute (data-theme="theme-name"). I bet you could even use a CSS Custom Property for the theme value too. Each of these would still need to be outside of the scope of the react tree in order to be read during hydration to determine the theme.

Hope I understood that question right, let me know if that makes sense or if I misunderstood anything!

Sep 28 '22 17:09 joshblack

Came here to say I really like the alternative 😅 I was hoping to implement that because it solves multiple problems (no dangerouslySetInnerHTML and no API for framework specific scripts)

Would using CSS variables for theming eliminate the need for this script entirely?

Yes, I do think we won't have this problem if/when decisions are encoded as component tokens because primer/react or styled-components would not be aware of theming implementation anymore

Sep 29 '22 15:09 siddharthkp

@siddharthkp sounds good! Is the other approach switching it from a <script> to something like a <div> and reading from it? Happy to make that change 👍

Sep 29 '22 16:09 joshblack