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: Incorrect Hydration Mismatch Detection during Suspense - "Hydration failed because the initial UI does not match what was rendered on the server." #24384

Closed
xiel opened this issue Apr 15, 2022 · 13 comments · Fixed by #24404
Labels
React 18 Bug reports, questions, and general feedback about React 18 Type: Bug

Comments

@xiel
Copy link

xiel commented Apr 15, 2022

React version: 18.0.0

Steps To Reproduce

  1. Add a Suspense Boundary
  2. Add a component that will suspend to load some data (faked).
  3. Render at least one sibling component after the suspending component.
  4. Render server-side using renderToPipeableStream()
  5. Render client-side using hydrateRoot()

Reproductions in CodeSandbox:

<>
  <h4>This headline hydrates fine. </h4>
  <SomethingA />{/* Will suspend on client and server */}
  <h3>💥 This element after the suspending Component triggers an error (only in development).</h3>
</>

The current behavior

  • The sibling component following the suspending component is incorrectly seen as a hydration mismatch.
  • Console will show errors (and Next.js will show big error overlay):
Warning: Expected server HTML to contain a matching <h3> in <div>.
    at h3
    at IndexPage
    at Suspense
    at App (webpack-internal:///./pages/_app.js:42:27)
    at ErrorBoundary (webpack-internal:///./node_modules/next/dist/compiled/@next/react-dev-overlay/client.js:8:20638)
    at ReactDevOverlay (webpack-internal:///./node_modules/next/dist/compiled/@next/react-dev-overlay/client.js:8:23179)
    at Container (webpack-internal:///./node_modules/next/dist/client/index.js:323:9)
    at AppContainer (webpack-internal:///./node_modules/next/dist/client/index.js:825:26)
    at Root (webpack-internal:///./node_modules/next/dist/client/index.js:949:27)
Uncaught Error: Hydration failed because the initial UI does not match what was rendered on the server.
    at throwOnHydrationMismatch (react-dom.development.js?ac89:14344:1)
    at tryToClaimNextHydratableInstance (react-dom.development.js?ac89:14372:1)
    at updateHostComponent$1 (react-dom.development.js?ac89:20636:1)
    at beginWork (react-dom.development.js?ac89:22373:1)
    at HTMLUnknownElement.callCallback (react-dom.development.js?ac89:4157:1)
    at Object.invokeGuardedCallbackDev (react-dom.development.js?ac89:4206:1)
    at invokeGuardedCallback (react-dom.development.js?ac89:4270:1)
  • Depending on the location of the suspending component and their siblings, there are errors or no errors at all. When the suspending component is the last component, there are no errors logged.
  • Error can be suppressed by wrapping the suspending component in a Suspense boundary directly

The expected behavior

  • I expect to be able to use the same fetching mechanism using suspense on server and client in React v18.
    OR
  • If this is unsupported usage, the errors should be more clear and consistent.
@xiel xiel added the Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug label Apr 15, 2022
@gaearon
Copy link
Collaborator

gaearon commented Apr 15, 2022

Small repro: https://codesandbox.io/s/sleepy-dan-ghlh1u?file=/pages/_app.js

@gaearon gaearon added Type: Bug React 18 Bug reports, questions, and general feedback about React 18 and removed Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug labels Apr 15, 2022
@gaearon
Copy link
Collaborator

gaearon commented Apr 15, 2022

Minimal repro without Next.js: https://codesandbox.io/s/compassionate-night-uzyxv1?file=/src/App.js

@gaearon
Copy link
Collaborator

gaearon commented Apr 15, 2022

Seems like a bug. Weird that I can't repro this in tests.

@gurkerl83
Copy link

gurkerl83 commented Apr 19, 2022

Hi, I have seen this error lots of times in #23381. The issue was closed because Dom-Patching executed by React is not an alternative in React 18 anymore.

The problem with a sibling component following a suspending component seems quite related to what follows the suspending component can be seen as the mismatch itself. In my opinion, when a suspender alike component is involved, it is similar if you take direct action patching the DOM now on the client-side, on user code because react doesn`t do this not anymore.

The following findings ask if the suspected bug is related to the following interpretation.

...
Or patch up the HTML tree to prune the invisible branches before starting hydration.

It seems it is impossible to prune on the client side before starting hydration. The only way possible is involving an effect and a state. Even with a suspense boundary in place it is not possible.

Observation
Removing the throw statement in tryToClaimNextHydratableInstance works! An effect and state mutation is no longer required when handled inside a Suspense boundary.

The throw statement in popHydrationState ensures that an error is thrown if there are still actual differences between the server and client that have not been previously resolved through client intervention in a Suspense boundary.

I ask you to revisit the comments about the repetitive throw events in the samples.

The function tryToClaimNextHydratableInstance gets invoked inside the update functions of

The base question is if the throw statements in the function tryToClaimNextHydratableInstance are too early and unnecessary; the second sample extracted from popHydrationState throws them again and mentions an interesting comment.

Note:
Before the function tryToClaimNextHydratableInstance did not have any logic regarding the older DOM-Patching or the new approach followed to throw an error and catch them calling clear container.

Question???
Does an incomplete component (Suspense) don't need the time and opportunity to evolve and resolve the embodied promises (unknown what the shape of the return component is) before it gets canceled? If so this does not seem to be recognised because invoking tryToClaimNextHydratableInstance seems immediate in the update functions from above (on the mount).

function tryToClaimNextHydratableInstance(fiber: Fiber): void {
  if (!isHydrating) {
    return;
  }
  let nextInstance = nextHydratableInstance;
  if (!nextInstance) {
    if (shouldClientRenderOnMismatch(fiber)) {
      warnNonhydratedInstance((hydrationParentFiber: any), fiber);
      throwOnHydrationMismatch(fiber);            <== !!!!!!!!! This is too strict; client-side 
                                                  render decisions are not respected
                                                  before hydration starts. A typical 
                                                  use case is to avoid the initial render 
                                                  when the client has more info such as
                                                  screen size; currently passing an effect 
                                                  executing a state mutation is required; 
                                                  client-side render decisions need an 
                                                  additional render on the client. 

                                                  Unfortunately this is required even when
                                                  render-decisions and components are
                                                  embedded inside a Suspense boundary. :'( 
                                                  !!!!!!!!!
    }
    // Nothing to hydrate. Make it an insertion.
    insertNonHydratedInstance((hydrationParentFiber: any), fiber);
    isHydrating = false;
    hydrationParentFiber = fiber;
    return;
  }
  const firstAttemptedInstance = nextInstance;
  if (!tryHydrate(fiber, nextInstance)) {
    if (shouldClientRenderOnMismatch(fiber)) {
      warnNonhydratedInstance((hydrationParentFiber: any), fiber);
      throwOnHydrationMismatch(fiber);       <== !!!!!!!!! Also too strict; 
                                             same as previous comment 
                                             !!!!!!!!!
    }
    // If we can't hydrate this instance let's try the next one.
    // We use this as a heuristic. It's based on intuition and not data so it
    // might be flawed or unnecessary.
    nextInstance = getNextHydratableSibling(firstAttemptedInstance);
    const prevHydrationParentFiber: Fiber = (hydrationParentFiber: any);
    if (!nextInstance || !tryHydrate(fiber, nextInstance)) {
      // Nothing to hydrate. Make it an insertion.
      insertNonHydratedInstance((hydrationParentFiber: any), fiber);
      isHydrating = false;
      hydrationParentFiber = fiber;
      return;
    }
    // We matched the next one, we'll now assume that the first one was
    // superfluous and we'll delete it. Since we can't eagerly delete it
    // we'll have to schedule a deletion. To do that, this node needs a dummy
    // fiber associated with it.
    deleteHydratableInstance(prevHydrationParentFiber, firstAttemptedInstance);
  }
}

The function popHydrationState gets called after tryToClaimNextHydratableInstance in the respective update functions from above.

I copied the relevant section of the popHydrationState function. The comment in the code makes me wonder if this is the only position where the throw of a hydration mismatch makes sense. As mentioned above, the popHydrationState gets executed after the update functions for Host, Suspense, and Text.

function popHydrationState(fiber: Fiber): boolean {
  ...
  // If we have any remaining hydratable nodes, we need to delete them now.
  // We only do this deeper than head and body since they tend to have random
  // other nodes in them. We also ignore components with pure text content in
  // side of them. We also don't delete anything inside the root container.
  if (
    fiber.tag !== HostRoot &&
    (fiber.tag !== HostComponent ||
      (shouldDeleteUnhydratedTailInstances(fiber.type) &&
        !shouldSetTextContent(fiber.type, fiber.memoizedProps)))
  ) {
    let nextInstance = nextHydratableInstance;
    if (nextInstance) {
      if (shouldClientRenderOnMismatch(fiber)) {
        warnIfUnhydratedTailNodes(fiber);
        throwOnHydrationMismatch(fiber);        <== !!!!!!!!! Reasonable, not to patch the DOM, 
                                                but this is handled by the application directly;
                                                see comments from the previous sample 
                                                !!!!!!!!!
      } else {
        while (nextInstance) {
          deleteHydratableInstance(fiber, nextInstance);
          nextInstance = getNextHydratableSibling(nextInstance);
        }
      }
    }
  }
...
}

Thx!

gnoff added a commit to gnoff/react that referenced this issue Apr 19, 2022
If a components suspends during hydration we expect there to be mismatches with server rendered HTML but we were not always supressing warning messages related to these expected mismatches
gnoff added a commit to gnoff/react that referenced this issue Apr 19, 2022
If a components suspends during hydration we expect there to be mismatches with server rendered HTML but we were not always supressing warning messages related to these expected mismatches
gnoff added a commit to gnoff/react that referenced this issue Apr 20, 2022
If a components suspends during hydration we expect there to be mismatches with server rendered HTML but we were not always supressing warning messages related to these expected mismatches
gnoff added a commit that referenced this issue Apr 20, 2022
* Add failing test case for #24384

If a components suspends during hydration we expect there to be mismatches with server rendered HTML but we were not always supressing warning messages related to these expected mismatches

* Mark hydration as suspending on every thrownException

previously hydration would only be marked as supsending when a genuine error was thrown. This created an opportunity for a hydration mismatch that would warn after which later hydration mismatches would not lead to warnings. By moving the marker check earlier in the thrownException function we get the hydration context to enter the didSuspend state on both error and thrown promise cases which eliminates this gap.

* Fix failing test related to client render fallbacks

This test was actually subject to the project identified in the issue fixed in this branch. After fixing the underlying issue the assertion logic needed to change to pick the right warning which now emits after hydration successfully completes on promise resolution. I changed the container type to 'section' to make the error message slightly easier to read/understand (for me)

* Only mark didSuspend on suspense path

For unknown reasons the didSuspend was being set only on the error path and nto the suspense path. The original change hoisted this to happen on both paths. This change moves the didSuspend call to the suspense path only. This appears to be a noop because if the error path marked didSuspend it would suppress later warnings but if it does not the warning functions themsevles do that suppression (after the first one which necessarily already happened)

* gate test on hydration fallback flags

* refactor didSuspend to didSuspendOrError

the orignial behavior applied the hydration warning bailout to error paths only. originally I moved it to Suspense paths only but this commit restores it to both paths and renames the marker function as didThrow rather than didSuspend

The logic here is that for either case if we get a mismatch in hydration we want to warm up components but effectively consider the hydration for this boundary halted

* factor tests to assert different behavior between prod and dev

* add DEV suffix to didSuspendOrError to better indicate this feature should only affect dev behavior

* move tests back to ReactDOMFizzServer-test

* fix comment casing

* drop extra flag gates in tests

* add test for user error case

* remove unnecessary gate

* Make test better

it now has an intentional client mismatch that would error if there wasn't suppression brought about by the earlier error. when it client renders it has the updated value not found in the server response but we do not see a hydration warning because it was superseded by the thrown error in that render
@xiel
Copy link
Author

xiel commented Apr 21, 2022

Please see my comment in the PR: #24404 (comment)

@gaearon gaearon reopened this Apr 22, 2022
@gurkerl83
Copy link

gurkerl83 commented Apr 22, 2022

@gnoff @gaearon

In my opinion, when a suspender alike component is involved, it is similar if you take direct action patching the DOM on the client-side, within your own code, because React does not do this anymore.

<Suspense>
<CompA><div>A</div>
<CompB - [Suspender]><div>B</div>
<CompC><div>C</div>
</Suspense>
  • 1st render on the server
<div>A</div>
<div>B - when the suspenders promise can get resolved on the server otherwise initiate a fallback and resolve at client-side, the final tree is unknown</div>
<div>C - if the promise within the suspender does resolve on the server; unknown otherwise, the output depends on the result of the promise</div> 
  • 1st render on the client
    If the suspense boundary does not resolve on the server completely producing the final tree, client-side render kicks in; the client re-runs the entire tree within the suspense block again and I think this is happening here.

BUT
The client will get aborted immediately because the erroring mechanism gets enforced before the actual thing can produce anything. See the comments made in the erroring sections in

  • tryToClaimNextHydratableInstance (the erroring in here seems to be what is producing this whole cascade of problems, no erroring was enforced before, this was brought in a couple of month back) and
  • popHydrationState (erroring in here seems now reasonable, before Reacts DOM-Patching was handled here)

functions extracted from Reacts source code #24384 (comment).

What do you think?

@gnoff
Copy link
Collaborator

gnoff commented May 3, 2022

@xiel can you confirm the fix in #24480 addresses your concern. Here is a slightly modified version of the original repro on codesandbox using the build from that PR and I believe it is doing what you expect: https://codesandbox.io/s/trusting-buck-2j21to?file=/package.json

@xiel
Copy link
Author

xiel commented May 3, 2022

@gnoff Looks good, thanks so much for working on this! 🤟

@xiel xiel closed this as completed May 3, 2022
@antonybudianto
Copy link

antonybudianto commented May 7, 2022

Hi, will the fix be on 18.1.1 / 18.2.0? I see the issue still persist on 18.1.0
however I tried https://react-builds.vercel.app/api/prs/24480/packages/react and it solved on my repo.

note:

    "react": "18.2.0-next-e531a4a62-20220505",
    "react-dom": "18.2.0-next-e531a4a62-20220505",

also solved the issue...

@gaearon
Copy link
Collaborator

gaearon commented May 7, 2022

The plan is to release it in 18.2.

@opensrc0

This comment was marked as resolved.

@gaearon
Copy link
Collaborator

gaearon commented Jun 9, 2022

It's expected that there is a development-only remount if your code is running in Strict Mode.

https://reactjs.org/blog/2022/03/08/react-18-upgrade-guide.html#updates-to-strict-mode

@insteptech
Copy link

When the Login token is undefined that time "Hydration failed because the initial UI does not match what was rendered on the server " is coming just remove the token from local storage

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
React 18 Bug reports, questions, and general feedback about React 18 Type: Bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants