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

Implement workaround for hydration error on React strict mode #3637

Closed
wants to merge 3 commits into from

Conversation

ciffelia
Copy link

@ciffelia ciffelia commented Oct 13, 2022

This commit implements a workaround for hydration error during SSR running in strict mode (#2231, #779).
The workaround uses React.useId, which is introduced in React 18. On React 16 and 17, nothing changes with this commit (i.e. hydration error still occurs).

✅ 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:

yarn test:ssr

Note: I'm not sure if this test runs in react strict mode.

🧢 Your Project:

pixiv/charcoal

@ciffelia ciffelia changed the title Implement workaround for hydration error on React strict mode (#2231, #779) Implement workaround for hydration error on React strict mode Oct 13, 2022
…2231, adobe#779)

This commit implements a workaround for hydration error during SSR running in strict mode.
The workaround uses React.useId, which is introduced in React 18.
On React 16 and 17, nothing changes with this commit (i.e. hydration error still occurs).
@ciffelia ciffelia force-pushed the fix-ssr-on-react-18-strictmode branch from b1c4f4d to 8f116e3 Compare October 13, 2022 03:18
@ciffelia ciffelia marked this pull request as ready for review October 13, 2022 03:27
Copy link
Member

@snowystinger snowystinger left a comment

Choose a reason for hiding this comment

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

Thanks so much for starting this!

packages/dev/docs/pages/react-aria/ssr.mdx Outdated Show resolved Hide resolved
packages/dev/docs/pages/react-aria/ssr.mdx Outdated Show resolved Hide resolved
Co-authored-by: Robert Snow <snowystinger@gmail.com>
@ciffelia
Copy link
Author

I'll update react-spectrum/ssr.mdx as well.

- remove implementation details
- mention `identifierPrefix`
@devongovett
Copy link
Member

Any idea why the original implementation doesn't work in strict mode? Is the useMemo being called multiple times? Should we use a ref instead?

Switching to React.useId is a big change and possibly orthogonal to the original issue. Plus, it only fixes it for React 18. Would be good to see if we can fix the original implementation, and move to React.useId as a separate improvement.

@ciffelia
Copy link
Author

Yes, functions passed to useMemo are called twice in strict mode. This is done intentionally to detect side-effects of the functions: https://reactjs.org/docs/strict-mode.html#detecting-unexpected-side-effects

I don't think it is possible to generate consistent and unique IDs in hooks without having side-effects nor depending on React.useId. I looked into how other libraries work around this problem, and found out that they generates IDs client-side only if React.useId is unavailable.

@devongovett
Copy link
Member

devongovett commented Oct 15, 2022

Maybe possible with a ref. Something like this?

function useId() {
  // ...
  let ref = useRef(null);
  if (ref.current == null) {
    ref.current = "react-aria-" + ctx.current++ // ...
  }
  return ref.current
}

(Typed on mobile, sorry for any mistakes)

@ciffelia
Copy link
Author

ciffelia commented Oct 19, 2022

I see. I think that idea would work well on React 17, but would cause hydration errors in concurrent mode on React 18, where the client and the server may not render components in the same order.

@ciffelia
Copy link
Author

I believe React.useId is the only way to generate IDs that doesn't cause hydration errors on React 18 (that's why React.useId is introduced).
We may rewrite useIdForLegacyReact using useRef for React 16 and 17, but I think that should be done in another pull request.

@devongovett do you have any ideas?

@devongovett
Copy link
Member

I believe that is only a problem when using the brand new streaming server renderer, which is only for Server Components not traditional SSR. Also we still support React 16 and 17, so we should fix the issue there as well. I think the approach I posted above might work for 16, 17, and regular SSR in 18. We would need to switch to React.useId for Server Components support at some point, but I'm concerned that there will be side effects from such a change. React Aria's useId is pretty deeply embedded in our libraries and lots of things rely on how it works, so we will need to be extremely careful about changing it. I would prefer to start with fixing the initial issue with strict mode, and tackling Server Components separately.

@ciffelia
Copy link
Author

Thanks for the explanation. That makes sense. May I use the approach you posted above to check if it works? I would create a new pull request if successful.

@devongovett
Copy link
Member

Sure, that would be great thanks!

@ciffelia ciffelia force-pushed the fix-ssr-on-react-18-strictmode branch from 24e36e9 to 925383e Compare October 31, 2022 14:30
@ciffelia
Copy link
Author

I just tried the useRef approach, and unfortunately it turned out that doesn't seem to work. During the first render (hydration), useRef returns different objects for each call of our useSSRSafeId hook. That is, ref.current is reset after the first call, where we increment ctx.current.

This behavior doesn't seem to be documented clearly, but is described in facebook/react#22872 and facebook/react#18003 (comment).

@devongovett
Copy link
Member

Hmm, this pattern is shown in the docs here: https://reactjs.org/docs/hooks-faq.html#how-to-create-expensive-objects-lazily

@ciffelia
Copy link
Author

Although the docs implies ref.current is initialized only once, the initialization seems to run twice in strict mode.

https://codesandbox.io/s/loving-swirles-zc254v?file=/src/index.js

import { useRef, StrictMode } from "react";
import { createRoot } from "react-dom/client";

const rootElement = document.getElementById("root");
const root = createRoot(rootElement);

root.render(
  <StrictMode>
    <App />
  </StrictMode>
);

let cnt = 0;
function App() {
  const ref = useRef(null);
  if (ref.current === null) {
    ref.current = `id-${cnt++}`;
  }

  console.log(ref.current);
  return ref.current;
}

image

@LucasUnplugged
Copy link

@ciffelia: is the issue that the server-side correctly generates just one ID, and the client-side generates two?

Maybe this is a "if you can't beat them, join them" situation: could modify the server-side to first generate a throw-away ID, then generate one that gets assigned?

I guess ideally we'd want to also check if strict mode is disable before generating the first ID, if that's possible in the server-side (I'm assuming it is).

@ciffelia
Copy link
Author

@LucasUnplugged I don't think that's possible. The SSR server sends rendered HTML to a browser immediately after the first rendering completed. There's no way to generate IDs after the first render.

@LucasUnplugged
Copy link

@LucasUnplugged I don't think that's possible. The SSR server sends rendered HTML to a browser immediately after the first rendering completed. There's no way to generate IDs after the first render.

What I mean is, it sounds like the frontend runs the ID generation code twice, so the first instance has ID 2 in the frontend and ID 1 in SSR code. The second has ID 4 instead of ID 2, and so on.

So in that case, I'm suggesting the ID generation func be run twice in the SSR logic as well, like:

genID(); // Throw away
const actualID = genID; // Save this one

If I understood correctly, the mismatch in IDs is deterministic, in that the first ID is always the same; the second is always the same; etc. The only difference is the frontend is doubling them up, in strict mode.

@ciffelia
Copy link
Author

ciffelia commented Jan 16, 2023

@LucasUnplugged I was mistaken about the behavior of Strict Mode. Indeed, I think the idea you presented is possible. Unfortunately it seems to be impossible to check if the strict mode is enabled (at least in the render phase), so we need to add an option to enable that feature:

<StrictMode>
  <SSRProvider strictMode={true}>
    <App />
  </SSRProvider>
</StrictMode>

Apart from that, I came up with an idea that allow users to let react-aria use React.useId internally:

import React from 'react'

<SSRProvider useId={React.useId}>
  <App />
</SSRProvider>

This will not only solve the problem with strict mode, but also ensure that our useId will work correctly with concurrent React.

Since both features are optional, there is less risk of breaking things. Those who are using React 18 may enable the latter feature, and others may choose the former. I would like to hear from maintainers before implementing them.

@brandonpittman
Copy link
Contributor

Remix shipped their defer helper to allow streaming Promises and this is starting to become an issue now that the streaming stuff is being used.

@ciffelia ciffelia deleted the fix-ssr-on-react-18-strictmode branch February 10, 2023 08:31
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.

5 participants