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

uniqueId() causes hydration warnings with server-side rendering #1392

Closed
jclem opened this issue Aug 24, 2021 · 4 comments · Fixed by #1409
Closed

uniqueId() causes hydration warnings with server-side rendering #1392

jclem opened this issue Aug 24, 2021 · 4 comments · Fixed by #1409
Assignees
Labels
bug Something isn't working

Comments

@jclem
Copy link
Contributor

jclem commented Aug 24, 2021

Describe the bug

When using server-side rendering, the uniqueId() function causes hydration warnings because the idSeed counter is globally shared for all renders for all users while it's reset to 10,000 on the client.

Additionally, this has the unintended side effect of leaking information about how many times a given page is visited (per rendering process, at least).

To Reproduce

Render any Primer component that uses uniqueId() using server-side rendering.

Expected behavior

The same ID is used on the server as on the client.

Screenshots

CleanShot 2021-08-24 at 13 10 38@2x

@colebemis
Copy link
Contributor

Added to our project board. Thank you!

@colebemis colebemis added the bug Something isn't working label Aug 25, 2021
@jfuchs jfuchs self-assigned this Sep 1, 2021
@jfuchs
Copy link
Contributor

jfuchs commented Sep 2, 2021

Looks like this isn't a new problem to React (facebook/react#4000, facebook/react#5867, reactjs/rfcs#32). I've looked around at a few solutions, and I like most of what React-aria does:

  • Use context to keep a counter in state, so you don't run into issues like a counter shared across renders
  • Allow nesting the provider for that context, which updates a separate counter of how deeply nested those providers are. That way you can wrap an additional provider around some asynchronously loaded part of the tree without worrying about server & client pulling their counters in a different order.
  • exposes everything in a handy hook

I do think there's a bug in their solution, for which I've opened adobe/react-spectrum#2277.

One option for us, if that issue gets fixed, is to pull in @react-aria/ssr as a dependency. Or we could roll something similar ourselves.

@jfuchs
Copy link
Contributor

jfuchs commented Sep 2, 2021

One other catch is that you have to be careful about where you include the provider in the tree — you have to place them to avoid ever having two components in the calling useSSRSafeId with the same ssr context in different sequences. E.g., this:

function Component() {
  const a = useFetch(...)
  const b = useFetch(...)

  return <>
    {a && <SubComponent data={a} />}
    {b && <SubComponent data={b} />}
  </>
}

function SubComponent() {
  const id = useSSRSafeId()
  return <div id={id} />
}

will be unsafe, but you can wrap each conditional call to SubComponent with a non-conditional call to SSRProvider to make it safe:

function Component() {
  const a = useFetch(...)
  const b = useFetch(...)

  return <>
    <SSRProvider>
      {a && <SubComponent data={a} />}
    </SSRProvider>
    <SSRProvider>
      {b && <SubComponent data={b} />}
    </SSRProvider>
  </>
}

@jclem
Copy link
Contributor Author

jclem commented Sep 9, 2021

This could also be fixed by allowing me to set IDs manually in many Primer components. For example, ActionList only allows me to use auto-generated IDs unless I take the much larger step of using a custom renderer everywhere for it. For example, requiring a unique ID for each ActionList item would put me in control. Even if the ID I provide needs to be used elsewhere, one could also use a template like ${idProp}-label on the ActionList item's label. I don't think this has to be quite so complex as maintaining counter state or anything.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants