Don't delete trailing mismatches during hydration at the root #21021

sebmarkbage · 2021-03-17T01:42:06Z

The problem I'm trying to solve is that with Fizz, we add extra nodes to the end (segments, and scripts). If you pipe Fizz into a container, then those end up at the end of the container.

<div id="container">
  <div>My Content</div>
  <div hidden id="Segment:123">...</div>
  <script>...</script>
</div>

Typically those extra nodes gets automatically deleted but segments that are part of an incomplete suspense boundary hang around for a while.

If you try to hydrate during this time, those segments end up getting deleted and you get a hydration error.

We have precedence for a similar case. If you hydrate a whole document, then it's likely that the <head> and <body> tag get extra nodes insert into it. So we already special cased those to ignore any mismatches at the end. However, we didn't actually special case if you hydrated with <body> as the container instead of the document.

This PR changes it so that we don't delete the tail if there's additional nodes remaining after hydration if it's directly below the container. We used to warn in these cases and you're really expected to always match. So it's an error case regardless.

It's a bit unfortunate that we can't warn since if your initial set up is flawed, then the root might be where that shows up initially. This commit shows the unfortunate consequences of this approach. It's really mostly an issue if your root is text, a fragment or not always the same tag.

I suspect that mostly you'll get errors deeper in the tree regardless in a real app. It just shows up a bit more in our tests since using the root is the easiest way to write the tests.

As a bonus, I added a specific warning if an error is thrown during hydration. In case it's fixed like useOpaqueIdentifier and useMutableSource. I also moved the head/body special case to the DOM host config.

sizebot · 2021-03-17T01:52:29Z

Comparing: 1d1e49c...618232e

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name	+/-	Base	Current	+/- gzip	Base gzip	Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js	+0.03%	122.85 kB	122.89 kB	+0.04%	39.53 kB	39.55 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js	+0.03%	129.35 kB	129.38 kB	+0.03%	41.60 kB	41.61 kB
facebook-www/ReactDOM-prod.classic.js	+0.07%	408.73 kB	409.01 kB	+0.06%	75.80 kB	75.85 kB
facebook-www/ReactDOM-prod.modern.js	+0.07%	396.99 kB	397.27 kB	+0.06%	73.86 kB	73.91 kB
facebook-www/ReactDOMForked-prod.classic.js	+0.07%	408.74 kB	409.02 kB	+0.06%	75.80 kB	75.85 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against 618232e

sebmarkbage · 2021-03-17T01:54:30Z

packages/react-reconciler/src/ReactFiberHydrationContext.new.js

-    (type !== 'head' &&
-      type !== 'body' &&
-      !shouldSetTextContent(type, fiber.memoizedProps))
+    fiber.tag !== HostRoot &&


This is the effective change.

packages/react-dom/src/__tests__/ReactDOMServerIntegrationFragment-test.js

packages/react-dom/src/__tests__/ReactDOMServerIntegrationModes-test.js

packages/react-dom/src/__tests__/ReactRenderDocument-test.js

bvaughn · 2021-03-17T14:33:16Z

packages/react-dom/src/__tests__/ReactRenderDocument-test.js

      const container = document.createElement('div');
      container.textContent = 'potato';
      expect(() => ReactDOM.hydrate(<div>parsnip</div>, container)).toErrorDev(
        'Expected server HTML to contain a matching <div> in <div>.',
      );
+      // This creates an unfortunate double text case.
+      expect(container.textContent).toBe('potatoparsnip');
+    });


nit: Looks like this test verifies broken behavior.

I wonder if there's a more explicit way we could write it (e.g. // @gate false and then expect the desired output instead of the unfortunate current output)?

Yea I'm not sure what would be best way to encode the subtleties here. This is not a "bug" as in it's not an issue with the implementation details that we could fix. It's an intentional behavior that we want to keep whatever else was in there and changing that behavior could be a breaking change.

bvaughn · 2021-03-17T14:38:43Z

packages/react-dom/src/__tests__/ReactServerRenderingHydration-test.js

+    expect(ref.current).not.toBe(div);
+    // Unfortunately, since we don't delete the tail at the root, a duplicate will remain.
+    expect(element.innerHTML).toBe(
+      '<div>Hello World</div><div>Hello World</div>',


I'm not sure I understand the difference in observable behavior between these two tests:

Suspense + hydration in legacy mode
Suspense + hydration in legacy mode (at root)

Seems like the significant difference here is that you have a <div> wrapper around the topmost Suspense boundary in your hydrated content. I think that causes us to add an extra node/segment and maybe the result of that is that we also don't clean up the previous <div>Hello World</div> content? Because of the changed fiber.tag !== HostRoot conditional?

This test is a bit weird. There's no suspense boundary comment in the HTML so it doesn't find any matching suspense boundary. If that mismatch is at the root, we just assume that we have to insert the DOM node. We assume that we want to keep what was already there - that didn't match. So we end up with two.

If it's not at the root, we still mismatch and insert a new node but we also remove what was already there.

packages/react-dom/src/client/ReactDOMHostConfig.js

bvaughn · 2021-03-17T14:41:50Z

packages/react-reconciler/src/ReactFiberHydrationContext.new.js

@@ -438,18 +439,15 @@ function popHydrationState(fiber: Fiber): boolean {
    return false;
  }

-  const type = fiber.type;
-
  // 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


nit This comment is pretty DOM renderer specific

Co-authored-by: Brian Vaughn <brian.david.vaughn@gmail.com>

…ok#21021) * Don't delete any trailing nodes in the container during hydration error * Warn when an error during hydration causes us to clear the container * Encode unfortunate case in test * Wrap the root for tests that are applicable to nested cases * Now we can pipe Fizz into a container * Grammatical fix

Summary: This sync includes the following changes: - **[6d3ecb70d](facebook/react@6d3ecb70d )**: Remove unstable_changedBits ([#20953](facebook/react#20953)) //<Andrew Clark>// - **[754e30728](facebook/react@754e30728 )**: Delete immediateQueueCallbackNode ([#20980](facebook/react#20980)) //<Andrew Clark>// - **[be5a2e231](facebook/react@be5a2e231 )**: Land enableSyncMicrotasks ([#20979](facebook/react#20979)) //<Andrew Clark>// - **[f6bc9c824](facebook/react@f6bc9c824 )**: [Fizz] Expose maxBoundarySize as an option ([#21029](facebook/react#21029)) //<Sebastian Markbåge>// - **[154b85213](facebook/react@154b85213 )**: [Fizz] Expose a method to explicitly start writing to a Node stream ([#21028](facebook/react#21028)) //<Sebastian Markbåge>// - **[cf485e6f6](facebook/react@cf485e6f6 )**: [Fizz] Expose a method to abort a pending request ([#21027](facebook/react#21027)) //<Sebastian Markbåge>// - **[3fb11eed9](facebook/react@3fb11eed9 )**: React Native: Touch Instrumentation Interface ([#21024](facebook/react#21024)) //<Timothy Yung>// - **[825c3021f](facebook/react@825c3021f )**: Don't delete trailing mismatches during hydration at the root ([#21021](facebook/react#21021)) //<Sebastian Markbåge>// - **[1d1e49cfa](facebook/react@1d1e49cfa )**: [Fizz] Assign an ID to the first DOM element in a fallback or insert a dummy (and testing infra) ([#21020](facebook/react#21020)) //<Sebastian Markbåge>// - **[466b26c92](facebook/react@466b26c92 )**: Store commit durations on HostRoot for DevTools access ([#20983](facebook/react#20983)) //<Brian Vaughn>// - **[89acfa639](facebook/react@89acfa639 )**: Fix native event batching in concurrent mode ([#21010](facebook/react#21010)) //<Ricky>// - **[0203b6567](facebook/react@0203b6567 )**: chore: update react-reconciler README ([#21016](facebook/react#21016)) //<susiwen8>// Changelog: [General][Changed] - React Native sync for revisions c9f6d0a...6d3ecb7 jest_e2e[run_all_tests] Reviewed By: JoshuaGross, kacieb Differential Revision: D27231625 fbshipit-source-id: 89c0c0662e69044ae8890486a693013bee6005dd

…ok#21021) * Don't delete any trailing nodes in the container during hydration error * Warn when an error during hydration causes us to clear the container * Encode unfortunate case in test * Wrap the root for tests that are applicable to nested cases * Now we can pipe Fizz into a container * Grammatical fix

I originally added this in #21021 but I didn't mention why and I don't quite remember why. Maybe because there were no other message? However at the time the recoverable errors mechanism didn't exist. Today I believe all cases where this happens will trigger another recoverable error. Namely these two: https://github.com/facebook/react/blob/9f33f699e4f832971dc0f2047129f832655a3b6d/packages/react-reconciler/src/ReactFiberBeginWork.js#L1442-L1446 https://github.com/facebook/react/blob/9f33f699e4f832971dc0f2047129f832655a3b6d/packages/react-reconciler/src/ReactFiberBeginWork.js#L2962-L2965 Therefore this is just an extra unnecessary log.

I originally added this in #21021 but I didn't mention why and I don't quite remember why. Maybe because there were no other message? However at the time the recoverable errors mechanism didn't exist. Today I believe all cases where this happens will trigger another recoverable error. Namely these two: https://github.com/facebook/react/blob/9f33f699e4f832971dc0f2047129f832655a3b6d/packages/react-reconciler/src/ReactFiberBeginWork.js#L1442-L1446 https://github.com/facebook/react/blob/9f33f699e4f832971dc0f2047129f832655a3b6d/packages/react-reconciler/src/ReactFiberBeginWork.js#L2962-L2965 Therefore this is just an extra unnecessary log. DiffTrain build for [323b6e9](323b6e9)

I originally added this in facebook#21021 but I didn't mention why and I don't quite remember why. Maybe because there were no other message? However at the time the recoverable errors mechanism didn't exist. Today I believe all cases where this happens will trigger another recoverable error. Namely these two: https://github.com/facebook/react/blob/9f33f699e4f832971dc0f2047129f832655a3b6d/packages/react-reconciler/src/ReactFiberBeginWork.js#L1442-L1446 https://github.com/facebook/react/blob/9f33f699e4f832971dc0f2047129f832655a3b6d/packages/react-reconciler/src/ReactFiberBeginWork.js#L2962-L2965 Therefore this is just an extra unnecessary log.

Don't delete any trailing nodes in the container during hydration error

0451e99

sebmarkbage requested review from bvaughn and gaearon March 17, 2021 01:42

facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Mar 17, 2021

sebmarkbage added 4 commits March 16, 2021 21:48

Warn when an error during hydration causes us to clear the container

64b4cf1

Encode unfortunate case in test

2ea0f18

Wrap the root for tests that are applicable to nested cases

7234ff4

Now we can pipe Fizz into a container

c035eec

sebmarkbage force-pushed the dontdeletefizz branch from 795541b to c035eec Compare March 17, 2021 01:48

sebmarkbage commented Mar 17, 2021

View reviewed changes

bvaughn self-assigned this Mar 17, 2021

bvaughn approved these changes Mar 17, 2021

View reviewed changes

Grammatical fix

618232e

Co-authored-by: Brian Vaughn <brian.david.vaughn@gmail.com>

sebmarkbage merged commit 825c302 into facebook:master Mar 17, 2021

sebmarkbage mentioned this pull request Mar 28, 2024

Remove errorHydratingContainer #28664

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't delete trailing mismatches during hydration at the root #21021

Don't delete trailing mismatches during hydration at the root #21021

sebmarkbage commented Mar 17, 2021 •

edited

Loading

sizebot commented Mar 17, 2021 •

edited

Loading

sebmarkbage Mar 17, 2021

bvaughn Mar 17, 2021

sebmarkbage Mar 17, 2021

bvaughn Mar 17, 2021

sebmarkbage Mar 17, 2021

bvaughn Mar 17, 2021

Don't delete trailing mismatches during hydration at the root #21021

Don't delete trailing mismatches during hydration at the root #21021

Conversation

sebmarkbage commented Mar 17, 2021 • edited Loading

sizebot commented Mar 17, 2021 • edited Loading

Critical size changes

Significant size changes

sebmarkbage Mar 17, 2021

Choose a reason for hiding this comment

bvaughn Mar 17, 2021

Choose a reason for hiding this comment

sebmarkbage Mar 17, 2021

Choose a reason for hiding this comment

bvaughn Mar 17, 2021

Choose a reason for hiding this comment

sebmarkbage Mar 17, 2021

Choose a reason for hiding this comment

bvaughn Mar 17, 2021

Choose a reason for hiding this comment

sebmarkbage commented Mar 17, 2021 •

edited

Loading

sizebot commented Mar 17, 2021 •

edited

Loading