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

Bug: <Suspense> fallback is rendered inconsistently when there’s a hydration mismatch + the boundary suspends during hydration #28285

Closed
iamakulov opened this issue Feb 9, 2024 · 4 comments
Labels
Resolution: Stale Automatically closed due to inactivity Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug

Comments

@iamakulov
Copy link

iamakulov commented Feb 9, 2024

React version: 18.2.0

Steps To Reproduce

  1. Create a tiny app that uses <Suspense> + suspends during hydration + has a hydration mismatch, for example:

    hydrateRoot(
      document.getElementById('app'),
      <StrictMode>
        <Suspense fallback={<SuspenseFallback />}>
          <p>
            Hello from StackBlitz! <span>Hydration mismatch here</span>
          </p>
          <LinkThatSuspends />
        </Suspense>
      </StrictMode>
    );

    (Full code in a StackBlitz)

  2. Load the app and observe that the suspense fallback is never mounted

  3. Now, wrap the <LinkThatSuspends /> element with a <div>:

    - <LinkThatSuspends />
    + <div><LinkThatSuspends /></div>

    (StackBlitz)

  4. Load the app and observe that the suspense fallback is now mounted

Code example: fallback not mounted · fallback mounted

The current behavior

The <Suspense> fallback is rendered inconsistently – depending on whether the element that suspends is wrapped with a <div>?!

The expected behavior

The <Suspense> fallback is rendered either never, or always, no matter if the element that suspends is wrapped with anything.

@iamakulov iamakulov added the Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug label Feb 9, 2024
@iamakulov iamakulov changed the title Bug: <Suspense> fallback is rendered inconsistently when there’s a hydration mismatch + Bug: <Suspense> fallback is rendered inconsistently when there’s a hydration mismatch + the boundary suspends during hydration Feb 9, 2024
@iamakulov
Copy link
Author

iamakulov commented Feb 9, 2024

Spent a bunch of time debugging this, and I think I understand what’s causing this.

Bug

Let’s say we’re trying to hydrate the following tree:

<Suspense>
  <div>
    <span />
    <span />
    [text content] ← this text content causes a hydration mismatch
    <span />
    <span />
    <ComponentThatSuspends />
  </div>
</Suspense>

Here’s why the bug happens:

  1. When hydrating the tree, React enters the <div> and starts walking its children. At some point, it walks over the [text content] node – and encounters a hydration mismatch. This causes React to find the nearest suspense boundary and set the ForceClientRender flag on it.

    (Later, another part of the code checks this flag and switches the <Suspense> boundary to client-side rendering, as documented.)

  2. However, after setting the ForceClientRender flag, React does not switch the <Suspense> to client rendering immediately. Instead, it walks the remaining siblings (in this case, two <span />s and <ComponentThatSuspends />) and then exits the subtree.

  3. This is where the inconsistency lies.

    In the normal case (without <ComponentThatSuspends />), React finishes walking all siblings, walks back up to <Suspense>, realizes that <Suspense> has ForceClientRender set, and client-renders <Suspense> again from scratch.

    However, if one of the siblings React walks over (<ComponentThatSuspends />) suspends, React removes the ForceClientRender flag. This effectively disables mismatch handling (until later) – instead, <Suspense> suspends, as if there was no hydration mismatch. And because it’s still hydrating, Suspense keeps the server HTML instead of rendering the fallback.

    This behavior disappears if <ComponentThatSuspends /> lives in another part of the tree (not in direct siblings):

    <Suspense>
      <div>
        <span />
        <span />
        [text content] ← this text content causes a hydration mismatch
        <span />
        <span />
        <div>
          <ComponentThatSuspends />
        </div>
      </div>
    </Suspense>

    With this tree, React won’t walk over <ComponentThatSuspends /> (because it’s inside a sibling, and React won’t enter siblings after it encountered a hydration error). This means React will follow the normal code path → re-render the Suspense boundary from scratch instead of hydrating it → suspend that boundary when it finally encounters <ComponentThatSuspends /> → and render the Suspense fallback.

  4. As a result, if <ComponentThatSuspends /> is a direct sibling to the hydration mismatch, React will keep the server HTML until the component unsuspends. If it’s not, React will render the Suspense fallback.

    This is (imo) the bug: React’s behavior shouldn’t depend on where inside the <Suspense> boundary the hydration mismatch happens.

What should React do?

Just from the code perspective, it feels easier/cheaper to keep the “normal” case – always resetting the <Suspense> boundary if there’s a hydration mismatch. This also matches the documented and intuitive model that says that <Suspense> is rerendered from scratch on a mismatch – this is good because every edge case is an overhead that devs need to learn.

But from the UX perspective, I’d maybe prefer React to keep the server HTML until the boundary unsuspends?

  • If we keep the server HTML, the UI will go through two states: Server HTML → Client HTML. Both of these should be similar, despite a minor hydration mismatch.
  • If we reset the <Suspense> boundary, the UI will jump through Server HTML → “Loading...” fallback → Client HTML, which feels like a worse UX.

IDK, feels like a hard choice to make (good thing it’s not mine I guess ^_^)

rickhanlonii added a commit that referenced this issue Mar 26, 2024
…28299)

While investigating #28285 I
found a possible bug in handling Suspense and mismatches. As the tests
show, if the first sibling in a boundary suspends, and the second has a
mismatch, we will NOT show a fallback. If the first sibling is a
mismatch, and the second sibling suspends, we WILL show a fallback.

[Here's a stackbliz showing the behavior on
Canary](https://stackblitz.com/edit/stackblitz-starters-bh3snf?file=src%2Fstyle.css,public%2Findex.html,src%2Findex.tsx).

This breakage was introduced by:
#26380. Before this PR, we would
not show a fallback in either case. That PR makes it so that we don't
pre-render siblings of suspended trees, so presumably, whatever
detection we had to avoid fallbacks on mismatches, requires knowing
there's a mismatch in the tree when we suspend.
@iamakulov
Copy link
Author

@rickhanlonii Sorry for the direct ping – I see you referenced this bug in some other PR; any chance this bug can be set to “Confirmed”? (It still reproduces even in canary)

EdisonVan pushed a commit to EdisonVan/react that referenced this issue Apr 15, 2024
…acebook#28299)

While investigating facebook#28285 I
found a possible bug in handling Suspense and mismatches. As the tests
show, if the first sibling in a boundary suspends, and the second has a
mismatch, we will NOT show a fallback. If the first sibling is a
mismatch, and the second sibling suspends, we WILL show a fallback.

[Here's a stackbliz showing the behavior on
Canary](https://stackblitz.com/edit/stackblitz-starters-bh3snf?file=src%2Fstyle.css,public%2Findex.html,src%2Findex.tsx).

This breakage was introduced by:
facebook#26380. Before this PR, we would
not show a fallback in either case. That PR makes it so that we don't
pre-render siblings of suspended trees, so presumably, whatever
detection we had to avoid fallbacks on mismatches, requires knowing
there's a mismatch in the tree when we suspend.
Copy link

This issue has been automatically marked as stale. If this issue is still affecting you, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize it yet. If you have any new additional information, please include it with your comment!

@github-actions github-actions bot added the Resolution: Stale Automatically closed due to inactivity label Jul 10, 2024
Copy link

Closing this issue after a prolonged period of inactivity. If this issue is still present in the latest release, please create a new issue with up-to-date information. Thank you!

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Jul 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Resolution: Stale Automatically closed due to inactivity Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug
Projects
None yet
Development

No branches or pull requests

1 participant