Skip to content

feat(ThemeProvider): add custom script prop to override dangerouslySetInnerHTML - #2337

Closed
joshblack wants to merge 9 commits into
mainfrom
1049-update-theme-provider
Closed

feat(ThemeProvider): add custom script prop to override dangerouslySetInnerHTML#2337
joshblack wants to merge 9 commits into
mainfrom
1049-update-theme-provider

Conversation

@joshblack

@joshblack joshblack commented Sep 14, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

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 👀

@joshblack
joshblack requested review from a team and siddharthkp September 14, 2022 21:29
@changeset-bot

changeset-bot Bot commented Sep 14, 2022 •
edited
Loading

Copy link
Copy Markdown

⚠️ No Changeset found

Latest commit: ab8cc75

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

@github-actions

github-actions Bot commented Sep 14, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

size-limit report 📦

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

@joshblack
joshblack temporarily deployed to github-pages September 14, 2022 21:37 Inactive
@joshblack joshblack changed the title fix(ThemeProvider): remove use of dangerouslySetInnerHTML feat(ThemeProvider): add custom script prop to override dangerouslySetInnerHTML Sep 20, 2022
@joshblack
joshblack temporarily deployed to github-pages September 20, 2022 18:42 Inactive
@joshblack
joshblack temporarily deployed to github-pages September 20, 2022 18:50 Inactive
colebemis
colebemis approved these changes Sep 28, 2022
View reviewed changes

@colebemis colebemis left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This seems like a reasonable approach 👍

A few questions:

  • Would using CSS variables for theming eliminate the need for this script entirely?
  • Will the SSR approaches we're exploring in Dotcom be compatible with this approach? (This is probably a question for Web Systems)
  • Should we add documentation for the script prop in the Theming section of the Primer React docs?

@joshblack
joshblack temporarily deployed to github-pages September 28, 2022 17:30 Inactive
@joshblack

joshblack commented Sep 28, 2022

Copy link
Copy Markdown
Member Author

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

@joshblack
joshblack temporarily deployed to github-pages September 28, 2022 17:52 Inactive
@siddharthkp

siddharthkp commented Sep 29, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

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

@joshblack

joshblack commented Sep 29, 2022

Copy link
Copy Markdown
Member Author

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

@joshblack joshblack closed this Oct 20, 2022
@joshblack
joshblack deleted the 1049-update-theme-provider branch January 19, 2023 16:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@siddharthkp siddharthkp Awaiting requested review from siddharthkp
1 more reviewer
@colebemis colebemis colebemis approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@joshblack @siddharthkp @colebemis