Don't delete trailing mismatches during hydration at the root (facebo…

…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
zhengjitf · Mar 23, 2021 · 2facf5e · 2facf5e
1 parent e2a1825
commit 2facf5e
Show file tree

Hide file tree

Showing 15 changed files with 159 additions and 63 deletions.
diff --git a/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js b/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js
@@ -19,6 +19,7 @@ let Suspense;
 let textCache;
 let document;
 let writable;
+let container;
 let buffer = '';
 let hasErrored = false;
 let fatalError = undefined;
@@ -38,10 +39,14 @@ describe('ReactDOMFizzServer', () => {
     textCache = new Map();
 
     // Test Environment
-    const jsdom = new JSDOM('<!DOCTYPE html><html><head></head><body>', {
-      runScripts: 'dangerously',
-    });
+    const jsdom = new JSDOM(
+      '<!DOCTYPE html><html><head></head><body><div id="container">',
+      {
+        runScripts: 'dangerously',
+      },
+    );
     document = jsdom.window.document;
+    container = document.getElementById('container');
 
     buffer = '';
     hasErrored = false;
@@ -80,9 +85,9 @@ describe('ReactDOMFizzServer', () => {
         const script = document.createElement('script');
         script.textContent = node.textContent;
         fakeBody.removeChild(node);
-        document.body.appendChild(script);
+        container.appendChild(script);
       } else {
-        document.body.appendChild(node);
+        container.appendChild(node);
       }
     }
   }
@@ -200,11 +205,11 @@ describe('ReactDOMFizzServer', () => {
         writable,
       );
     });
-    expect(getVisibleChildren(document.body)).toEqual(<div>Loading...</div>);
+    expect(getVisibleChildren(container)).toEqual(<div>Loading...</div>);
     await act(async () => {
       resolveText('Hello World');
     });
-    expect(getVisibleChildren(document.body)).toEqual(<div>Hello World</div>);
+    expect(getVisibleChildren(container)).toEqual(<div>Hello World</div>);
   });
 
   // @gate experimental
@@ -224,20 +229,12 @@ describe('ReactDOMFizzServer', () => {
     }
 
     await act(async () => {
-      ReactDOMFizzServer.pipeToNodeWritable(
-        // We currently have to wrap the server node in a container because
-        // otherwise the Fizz nodes get deleted during hydration.
-        <div id="container">
-          <App />
-        </div>,
-        writable,
-      );
+      ReactDOMFizzServer.pipeToNodeWritable(<App />, writable);
     });
 
     // We're still showing a fallback.
 
     // Attempt to hydrate the content.
-    const container = document.body.firstChild;
     const root = ReactDOM.unstable_createRoot(container, {hydrate: true});
     root.render(<App />);
     Scheduler.unstable_flushAll();

diff --git a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationFragment-test.js b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationFragment-test.js
@@ -103,7 +103,15 @@ describe('ReactDOMServerIntegration', () => {
     });
 
     itRenders('an empty fragment', async render => {
-      expect(await render(<React.Fragment />)).toBe(null);
+      expect(
+        (
+          await render(
+            <div>
+              <React.Fragment />
+            </div>,
+          )
+        ).firstChild,
+      ).toBe(null);
     });
   });
 });
diff --git a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationHooks-test.js b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationHooks-test.js
@@ -1654,9 +1654,13 @@ describe('ReactDOMServerHooks', () => {
       // This is the wrong HTML string
       container.innerHTML = '<span></span>';
       ReactDOM.unstable_createRoot(container, {hydrate: true}).render(<App />);
-      expect(() => Scheduler.unstable_flushAll()).toErrorDev([
-        'Warning: Expected server HTML to contain a matching <div> in <div>.',
-      ]);
+      expect(() => Scheduler.unstable_flushAll()).toErrorDev(
+        [
+          'Warning: An error occurred during hydration. The server HTML was replaced with client content in <div>.',
+          'Warning: Expected server HTML to contain a matching <div> in <div>.',
+        ],
+        {withoutStack: 1},
+      );
     });
 
     // @gate experimental
@@ -1740,9 +1744,13 @@ describe('ReactDOMServerHooks', () => {
       // This is the wrong HTML string
       container.innerHTML = '<span></span>';
       ReactDOM.unstable_createRoot(container, {hydrate: true}).render(<App />);
-      expect(() => Scheduler.unstable_flushAll()).toErrorDev([
-        'Warning: Expected server HTML to contain a matching <div> in <div>.',
-      ]);
+      expect(() => Scheduler.unstable_flushAll()).toErrorDev(
+        [
+          'Warning: An error occurred during hydration. The server HTML was replaced with client content in <div>.',
+          'Warning: Expected server HTML to contain a matching <div> in <div>.',
+        ],
+        {withoutStack: 1},
+      );
     });
 
     // @gate experimental
@@ -1764,7 +1772,7 @@ describe('ReactDOMServerHooks', () => {
       expect(() => Scheduler.unstable_flushAll()).toErrorDev(
         [
           'Warning: The object passed back from useOpaqueIdentifier is meant to be passed through to attributes only. Do not read the value directly.',
-          'Warning: Did not expect server HTML to contain a <span> in <div>.',
+          'Warning: An error occurred during hydration. The server HTML was replaced with client content in <div>.',
         ],
         {withoutStack: 1},
       );
@@ -1789,7 +1797,7 @@ describe('ReactDOMServerHooks', () => {
       expect(() => Scheduler.unstable_flushAll()).toErrorDev(
         [
           'Warning: The object passed back from useOpaqueIdentifier is meant to be passed through to attributes only. Do not read the value directly.',
-          'Warning: Did not expect server HTML to contain a <span> in <div>.',
+          'Warning: An error occurred during hydration. The server HTML was replaced with client content in <div>.',
         ],
         {withoutStack: 1},
       );
@@ -1813,7 +1821,7 @@ describe('ReactDOMServerHooks', () => {
       expect(() => Scheduler.unstable_flushAll()).toErrorDev(
         [
           'Warning: The object passed back from useOpaqueIdentifier is meant to be passed through to attributes only. Do not read the value directly.',
-          'Warning: Did not expect server HTML to contain a <div> in <div>.',
+          'Warning: An error occurred during hydration. The server HTML was replaced with client content in <div>.',
         ],
         {withoutStack: 1},
       );
@@ -1834,7 +1842,7 @@ describe('ReactDOMServerHooks', () => {
       expect(() => Scheduler.unstable_flushAll()).toErrorDev(
         [
           'Warning: The object passed back from useOpaqueIdentifier is meant to be passed through to attributes only. Do not read the value directly.',
-          'Warning: Did not expect server HTML to contain a <div> in <div>.',
+          'Warning: An error occurred during hydration. The server HTML was replaced with client content in <div>.',
         ],
         {withoutStack: 1},
       );

diff --git a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationModes-test.js b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationModes-test.js
@@ -153,7 +153,15 @@ describe('ReactDOMServerIntegration', () => {
     });
 
     itRenders('an empty strict mode', async render => {
-      expect(await render(<React.StrictMode />)).toBe(null);
+      expect(
+        (
+          await render(
+            <div>
+              <React.StrictMode />
+            </div>,
+          )
+        ).firstChild,
+      ).toBe(null);
     });
   });
 });
diff --git a/packages/react-dom/src/__tests__/ReactMount-test.js b/packages/react-dom/src/__tests__/ReactMount-test.js
@@ -123,16 +123,12 @@ describe('ReactMount', () => {
     expect(instance1 === instance2).toBe(true);
   });
 
-  it('should warn if mounting into left padded rendered markup', () => {
+  it('does not warn if mounting into left padded rendered markup', () => {
     const container = document.createElement('container');
     container.innerHTML = ReactDOMServer.renderToString(<div />) + ' ';
 
-    expect(() =>
-      ReactDOM.hydrate(<div />, container),
-    ).toErrorDev(
-      'Did not expect server HTML to contain the text node " " in <container>.',
-      {withoutStack: true},
-    );
+    // This should probably ideally warn but we ignore extra markup at the root.
+    ReactDOM.hydrate(<div />, container);
   });
 
   it('should warn if mounting into right padded rendered markup', () => {

diff --git a/packages/react-dom/src/__tests__/ReactRenderDocument-test.js b/packages/react-dom/src/__tests__/ReactRenderDocument-test.js
@@ -368,12 +368,31 @@ describe('rendering React components at document', () => {
       expect(testDocument.body.innerHTML).toBe('Hello world');
     });
 
-    it('renders over an existing text child without throwing', () => {
+    it('cannot render over an existing text child at the root', () => {
       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');
+    });
+
+    it('renders over an existing nested text child without throwing', () => {
+      const container = document.createElement('div');
+      const wrapper = document.createElement('div');
+      wrapper.textContent = 'potato';
+      container.appendChild(wrapper);
+      expect(() =>
+        ReactDOM.hydrate(
+          <div>
+            <div>parsnip</div>
+          </div>,
+          container,
+        ),
+      ).toErrorDev(
+        'Expected server HTML to contain a matching <div> in <div>.',
+      );
       expect(container.textContent).toBe('parsnip');
     });
 

diff --git a/packages/react-dom/src/__tests__/ReactServerRenderingHydration-test.js b/packages/react-dom/src/__tests__/ReactServerRenderingHydration-test.js
@@ -510,26 +510,47 @@ describe('ReactDOMServerHydration', () => {
 
   it('Suspense + hydration in legacy mode', () => {
     const element = document.createElement('div');
-    element.innerHTML = '<div>Hello World</div>';
-    const div = element.firstChild;
+    element.innerHTML = '<div><div>Hello World</div></div>';
+    const div = element.firstChild.firstChild;
     const ref = React.createRef();
     expect(() =>
       ReactDOM.hydrate(
-        <React.Suspense fallback={null}>
-          <div ref={ref}>Hello World</div>
-        </React.Suspense>,
+        <div>
+          <React.Suspense fallback={null}>
+            <div ref={ref}>Hello World</div>
+          </React.Suspense>
+        </div>,
         element,
       ),
     ).toErrorDev(
       'Warning: Did not expect server HTML to contain a <div> in <div>.',
-      {withoutStack: true},
     );
 
     // The content should've been client rendered and replaced the
     // existing div.
     expect(ref.current).not.toBe(div);
     // The HTML should be the same though.
-    expect(element.innerHTML).toBe('<div>Hello World</div>');
+    expect(element.innerHTML).toBe('<div><div>Hello World</div></div>');
+  });
+
+  it('Suspense + hydration in legacy mode (at root)', () => {
+    const element = document.createElement('div');
+    element.innerHTML = '<div>Hello World</div>';
+    const div = element.firstChild;
+    const ref = React.createRef();
+    ReactDOM.hydrate(
+      <React.Suspense fallback={null}>
+        <div ref={ref}>Hello World</div>
+      </React.Suspense>,
+      element,
+    );
+
+    // The content should've been client rendered.
+    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>',
+    );
   });
 
   it('Suspense + hydration in legacy mode with no fallback', () => {

diff --git a/packages/react-dom/src/client/ReactDOMHostConfig.js b/packages/react-dom/src/client/ReactDOMHostConfig.js
@@ -894,6 +894,12 @@ export function commitHydratedSuspenseInstance(
   retryIfBlockedOn(suspenseInstance);
 }
 
+export function shouldDeleteUnhydratedTailInstances(
+  parentType: string,
+): boolean {
+  return parentType !== 'head' || parentType !== 'body';
+}
+
 export function didNotMatchHydratedContainerTextInstance(
   parentContainer: Container,
   textInstance: TextInstance,
@@ -1008,6 +1014,15 @@ export function didNotFindHydratableSuspenseInstance(
   }
 }
 
+export function errorHydratingContainer(parentContainer: Container): void {
+  if (__DEV__) {
+    console.error(
+      'An error occurred during hydration. The server HTML was replaced with client content in <%s>.',
+      parentContainer.nodeName.toLowerCase(),
+    );
+  }
+}
+
 export function getInstanceFromNode(node: HTMLElement): null | Object {
   return getClosestInstanceFromNode(node) || null;
 }

diff --git a/packages/react-reconciler/src/ReactFiberHostConfigWithNoHydration.js b/packages/react-reconciler/src/ReactFiberHostConfigWithNoHydration.js
@@ -40,6 +40,7 @@ export const commitHydratedContainer = shim;
 export const commitHydratedSuspenseInstance = shim;
 export const clearSuspenseBoundary = shim;
 export const clearSuspenseBoundaryFromContainer = shim;
+export const shouldDeleteUnhydratedTailInstances = shim;
 export const didNotMatchHydratedContainerTextInstance = shim;
 export const didNotMatchHydratedTextInstance = shim;
 export const didNotHydrateContainerInstance = shim;
@@ -50,3 +51,4 @@ export const didNotFindHydratableContainerSuspenseInstance = shim;
 export const didNotFindHydratableInstance = shim;
 export const didNotFindHydratableTextInstance = shim;
 export const didNotFindHydratableSuspenseInstance = shim;
+export const errorHydratingContainer = shim;
diff --git a/packages/react-reconciler/src/ReactFiberHydrationContext.new.js b/packages/react-reconciler/src/ReactFiberHydrationContext.new.js
@@ -43,6 +43,7 @@ import {
   hydrateTextInstance,
   hydrateSuspenseInstance,
   getNextHydratableInstanceAfterSuspenseInstance,
+  shouldDeleteUnhydratedTailInstances,
   didNotMatchHydratedContainerTextInstance,
   didNotMatchHydratedTextInstance,
   didNotHydrateContainerInstance,
@@ -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
   // other nodes in them. We also ignore components with pure text content in
-  // side of them.
-  // TODO: Better heuristic.
+  // side of them. We also don't delete anything inside the root container.
   if (
-    fiber.tag !== HostComponent ||
-    (type !== 'head' &&
-      type !== 'body' &&
-      !shouldSetTextContent(type, fiber.memoizedProps))
+    fiber.tag !== HostRoot &&
+    (fiber.tag !== HostComponent ||
+      (shouldDeleteUnhydratedTailInstances(fiber.type) &&
+        !shouldSetTextContent(fiber.type, fiber.memoizedProps)))
   ) {
     let nextInstance = nextHydratableInstance;
     while (nextInstance) {

diff --git a/packages/react-reconciler/src/ReactFiberHydrationContext.old.js b/packages/react-reconciler/src/ReactFiberHydrationContext.old.js
@@ -43,6 +43,7 @@ import {
   hydrateTextInstance,
   hydrateSuspenseInstance,
   getNextHydratableInstanceAfterSuspenseInstance,
+  shouldDeleteUnhydratedTailInstances,
   didNotMatchHydratedContainerTextInstance,
   didNotMatchHydratedTextInstance,
   didNotHydrateContainerInstance,
@@ -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
   // other nodes in them. We also ignore components with pure text content in
-  // side of them.
-  // TODO: Better heuristic.
+  // side of them. We also don't delete anything inside the root container.
   if (
-    fiber.tag !== HostComponent ||
-    (type !== 'head' &&
-      type !== 'body' &&
-      !shouldSetTextContent(type, fiber.memoizedProps))
+    fiber.tag !== HostRoot &&
+    (fiber.tag !== HostComponent ||
+      (shouldDeleteUnhydratedTailInstances(fiber.type) &&
+        !shouldSetTextContent(fiber.type, fiber.memoizedProps)))
   ) {
     let nextInstance = nextHydratableInstance;
     while (nextInstance) {