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

React strict mode support for the useSSRSafeId hook #3963

Closed
wants to merge 1 commit into from

Conversation

ciffelia
Copy link

@ciffelia ciffelia commented Jan 26, 2023

Supersedes #3637. Fixes #2231. Related to #779.

This pull request adds options to fix hydration errors in React strict mode caused by the useSSRSafeId hook. The idea for this fix was suggested in the discussion on #3637 (comment). I created this separate pull request because I rewrote most part of the code I wrote in in the previous pull request.

In React 18, users should specify the "useId" mode and pass React.useId to the SSRProvider. The useSSRSafeId hook will then use React.useId internally.

import React from 'react'
import {SSRProvider} from '@react-aria/ssr';

<React.StrictMode>
  <SSRProvider mode="useId" useId={React.useId}>
    <App />
  </SSRProvider>
</React.StrictMode>

On React 16 or 17, users should set the strictMode property to process.env.NODE_ENV !== 'production'. Then the useSSRSafeId hook increments ctx.current twice on the server side to mimic the behavior of the strict mode on the client. This idea was suggested by @LucasUnplugged on #3637 (comment).

import React from 'react'
import {SSRProvider} from '@react-aria/ssr';

<React.StrictMode>
  <SSRProvider strictMode={process.env.NODE_ENV !== 'production'}>
    <App />
  </SSRProvider>
</React.StrictMode>

✅ Pull Request Checklist:

  • Included link to corresponding React Spectrum GitHub Issue.
  • Added/updated unit tests and storybook for this change (for new code or code which already has tests).
  • Filled out test instructions.
  • Updated documentation (if it already exists for this component).
  • Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

Run unit tests.

🧢 Your Project:

pixiv/charcoal

@ciffelia ciffelia force-pushed the fix-useid-in-react-strict-mode branch from 6ea3ff8 to 0038471 Compare January 26, 2023 06:26
@ciffelia ciffelia marked this pull request as ready for review January 26, 2023 07:28
@ciffelia ciffelia changed the title React strict mode support for useId hook React strict mode support for useSSRSafeId hook Jan 26, 2023
@ciffelia ciffelia changed the title React strict mode support for useSSRSafeId hook React strict mode support for the useSSRSafeId hook Jan 26, 2023
@devongovett
Copy link
Member

I'm a bit hesitant about adding a new strictMode prop because it might be challenging to know if you are in strict mode. Easy enough if you do it at the application level, but a component library with an SSRProvider inside it might not know whether the parent app is in strict mode. So I have an alternative proposal in #3980. It's a bit gross but might work at least as a fallback for React 16/17 if we switch to React.useId for 18. Let me know what you think, and thanks again for your work on this!

@devongovett devongovett closed this Feb 3, 2023
@ciffelia
Copy link
Author

Thank you for providing the alternative. I think #3980 is an ideal approach for React 16 and 17. Streaming rendering is not widely used at the moment, but in the future it would be nice to implement an option to switch to React.useId for React 18.

@ciffelia ciffelia deleted the fix-useid-in-react-strict-mode branch February 10, 2023 08:30
@LucasUnplugged
Copy link

@ciffelia @devongovett I haven't fully wrapped my head around the code of this provider, but couldn't we do something like this, to check for useId (with a typecast if need be)?

if ("useId" in React) {
  // Use `React.useId`
} else {
  // Use React 17/16 fallback
}

What's not super clear to me is how to reconcile that with this PR vs with #3980, but I'm just curious why we couldn't use the above to get React 18 support.

@snowystinger
Copy link
Member

Please see last paragraph of the description for #3980

@LucasUnplugged
Copy link

Please see last paragraph of the description for #3980

As mentioned before, we may still want to switch to React.useId for React 18 to handle other edge cases I'm not aware of with the new streaming renderer, though this should continue to work as before if you put an SSRProvider around every Suspense boundary. Switching that would change the format of the returned ids though, so might break people's unit tests (especially snapshots), so I am hesitant. Thoughts appreciated.

I'm missing the connection to my comment, @snowystinger. I'm reading that as:

  1. We would like to use useId for React 18.
  2. We're not sure how to do that, and are open to suggestions.
  3. The current workaround is both chore-filled, and could break tests.

I'm just confused why we can't just check for the presence of useId in React as a simple way to address that need.

(My bad if I misunderstood what you're referring to)

@snowystinger
Copy link
Member

No worries, I should've expanded on it.
We do want to use useId for React 18. We know a way to do it, as you've outlined, but it could break tests for users (snapshot tests specifically), so we are hesitant to do it and are looking for input.

@LucasUnplugged
Copy link

Ah, got it, thanks! I misunderstood that "know a way to do it" part. I'll mull it over and see if I have any suggestions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SSR – Mismatched server / client ids when running in StrictMode
4 participants