From d3b6ca6968464e8e11dad51c80c981158b74da16 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Markb=C3=A5ge?= Date: Tue, 19 Oct 2021 19:28:01 -0400 Subject: [PATCH] Clear extra nodes if there's a hydration mismatch within a suspense boundary (#22592) * Clear extra nodes if there's a mismatch within a suspense boundary This usually happens when we exit out a DOM node but a suspense boundary is a virtual DOM node and we didn't do it in that case because we took a short cut by calling resetHydrationState directly since we know we won't need to pop. * Tighten up the types of getFirstHydratableChild We currently call getFirstHydratableChild to step into the children of a suspense boundary. This can be a text node or a suspense boundary which isn't compatible with getFirstHydratableChild, and we cheat the type. This accidentally works because .firstChild always returns null on those nodes in the DOM. This just makes that explicit. --- ...DOMServerPartialHydration-test.internal.js | 58 +++++++++++++++++++ .../src/client/ReactDOMHostConfig.js | 3 + .../src/ReactFiberCompleteWork.new.js | 11 ++-- .../src/ReactFiberCompleteWork.old.js | 11 ++-- .../src/ReactFiberHydrationContext.new.js | 17 ++++-- .../src/ReactFiberHydrationContext.old.js | 17 ++++-- 6 files changed, 93 insertions(+), 24 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMServerPartialHydration-test.internal.js b/packages/react-dom/src/__tests__/ReactDOMServerPartialHydration-test.internal.js index 4b647e00032bc..505a100df3aa6 100644 --- a/packages/react-dom/src/__tests__/ReactDOMServerPartialHydration-test.internal.js +++ b/packages/react-dom/src/__tests__/ReactDOMServerPartialHydration-test.internal.js @@ -308,6 +308,64 @@ describe('ReactDOMServerPartialHydration', () => { expect(deleted.length).toBe(1); }); + it('hydrates an empty suspense boundary', async () => { + function App() { + return ( +
+ +
Sibling
+
+ ); + } + + const finalHTML = ReactDOMServer.renderToString(); + + const container = document.createElement('div'); + container.innerHTML = finalHTML; + + ReactDOM.hydrateRoot(container, ); + Scheduler.unstable_flushAll(); + jest.runAllTimers(); + + expect(container.innerHTML).toContain('
Sibling
'); + }); + + it('recovers when server rendered additional nodes', async () => { + const ref = React.createRef(); + function App({hasB}) { + return ( +
+ + A + {hasB ? B : null} + +
Sibling
+
+ ); + } + + const finalHTML = ReactDOMServer.renderToString(); + + const container = document.createElement('div'); + container.innerHTML = finalHTML; + + const span = container.getElementsByTagName('span')[0]; + + expect(container.innerHTML).toContain('A'); + expect(container.innerHTML).toContain('B'); + expect(ref.current).toBe(null); + + ReactDOM.hydrateRoot(container, ); + expect(() => { + Scheduler.unstable_flushAll(); + }).toErrorDev('Did not expect server HTML to contain a in
'); + jest.runAllTimers(); + + expect(container.innerHTML).toContain('A'); + expect(container.innerHTML).not.toContain('B'); + expect(ref.current).toBe(span); + }); + it('calls the onDeleted hydration callback if the parent gets deleted', async () => { let suspend = false; const promise = new Promise(() => {}); diff --git a/packages/react-dom/src/client/ReactDOMHostConfig.js b/packages/react-dom/src/client/ReactDOMHostConfig.js index 79b534d1e1812..8a5417ae49272 100644 --- a/packages/react-dom/src/client/ReactDOMHostConfig.js +++ b/packages/react-dom/src/client/ReactDOMHostConfig.js @@ -751,6 +751,9 @@ function getNextHydratable(node) { ) { break; } + if (nodeData === SUSPENSE_END_DATA) { + return null; + } } } } diff --git a/packages/react-reconciler/src/ReactFiberCompleteWork.new.js b/packages/react-reconciler/src/ReactFiberCompleteWork.new.js index a15702985e9a0..dadec516c3a4b 100644 --- a/packages/react-reconciler/src/ReactFiberCompleteWork.new.js +++ b/packages/react-reconciler/src/ReactFiberCompleteWork.new.js @@ -1007,16 +1007,16 @@ function completeWork( if (enableSuspenseServerRenderer) { if (nextState !== null && nextState.dehydrated !== null) { + // We might be inside a hydration state the first time we're picking up this + // Suspense boundary, and also after we've reentered it for further hydration. + const wasHydrated = popHydrationState(workInProgress); if (current === null) { - const wasHydrated = popHydrationState(workInProgress); - if (!wasHydrated) { throw new Error( 'A dehydrated suspense component was completed without a hydrated node. ' + 'This is probably a bug in React.', ); } - prepareToHydrateHostSuspenseInstance(workInProgress); bubbleProperties(workInProgress); if (enableProfilerTimer) { @@ -1034,9 +1034,8 @@ function completeWork( } return null; } else { - // We should never have been in a hydration state if we didn't have a current. - // However, in some of those paths, we might have reentered a hydration state - // and then we might be inside a hydration state. In that case, we'll need to exit out of it. + // We might have reentered this boundary to hydrate it. If so, we need to reset the hydration + // state since we're now exiting out of it. popHydrationState doesn't do that for us. resetHydrationState(); if ((workInProgress.flags & DidCapture) === NoFlags) { // This boundary did not suspend so it's now hydrated and unsuspended. diff --git a/packages/react-reconciler/src/ReactFiberCompleteWork.old.js b/packages/react-reconciler/src/ReactFiberCompleteWork.old.js index 89b8b980bc3e3..06fbf5abff50f 100644 --- a/packages/react-reconciler/src/ReactFiberCompleteWork.old.js +++ b/packages/react-reconciler/src/ReactFiberCompleteWork.old.js @@ -1007,16 +1007,16 @@ function completeWork( if (enableSuspenseServerRenderer) { if (nextState !== null && nextState.dehydrated !== null) { + // We might be inside a hydration state the first time we're picking up this + // Suspense boundary, and also after we've reentered it for further hydration. + const wasHydrated = popHydrationState(workInProgress); if (current === null) { - const wasHydrated = popHydrationState(workInProgress); - if (!wasHydrated) { throw new Error( 'A dehydrated suspense component was completed without a hydrated node. ' + 'This is probably a bug in React.', ); } - prepareToHydrateHostSuspenseInstance(workInProgress); bubbleProperties(workInProgress); if (enableProfilerTimer) { @@ -1034,9 +1034,8 @@ function completeWork( } return null; } else { - // We should never have been in a hydration state if we didn't have a current. - // However, in some of those paths, we might have reentered a hydration state - // and then we might be inside a hydration state. In that case, we'll need to exit out of it. + // We might have reentered this boundary to hydrate it. If so, we need to reset the hydration + // state since we're now exiting out of it. popHydrationState doesn't do that for us. resetHydrationState(); if ((workInProgress.flags & DidCapture) === NoFlags) { // This boundary did not suspend so it's now hydrated and unsuspended. diff --git a/packages/react-reconciler/src/ReactFiberHydrationContext.new.js b/packages/react-reconciler/src/ReactFiberHydrationContext.new.js index 6aad7f03339f5..7275f1663cad8 100644 --- a/packages/react-reconciler/src/ReactFiberHydrationContext.new.js +++ b/packages/react-reconciler/src/ReactFiberHydrationContext.new.js @@ -261,6 +261,8 @@ function tryHydrate(fiber, nextInstance) { const instance = canHydrateInstance(nextInstance, type, props); if (instance !== null) { fiber.stateNode = (instance: Instance); + hydrationParentFiber = fiber; + nextHydratableInstance = getFirstHydratableChild(instance); return true; } return false; @@ -270,6 +272,9 @@ function tryHydrate(fiber, nextInstance) { const textInstance = canHydrateTextInstance(nextInstance, text); if (textInstance !== null) { fiber.stateNode = (textInstance: TextInstance); + hydrationParentFiber = fiber; + // Text Instances don't have children so there's nothing to hydrate. + nextHydratableInstance = null; return true; } return false; @@ -294,6 +299,10 @@ function tryHydrate(fiber, nextInstance) { ); dehydratedFragment.return = fiber; fiber.child = dehydratedFragment; + hydrationParentFiber = fiber; + // While a Suspense Instance does have children, we won't step into + // it during the first pass. Instead, we'll reenter it later. + nextHydratableInstance = null; return true; } } @@ -322,6 +331,7 @@ function tryToClaimNextHydratableInstance(fiber: Fiber): void { // 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); @@ -333,13 +343,8 @@ function tryToClaimNextHydratableInstance(fiber: Fiber): void { // 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( - (hydrationParentFiber: any), - firstAttemptedInstance, - ); + deleteHydratableInstance(prevHydrationParentFiber, firstAttemptedInstance); } - hydrationParentFiber = fiber; - nextHydratableInstance = getFirstHydratableChild((nextInstance: any)); } function prepareToHydrateHostInstance( diff --git a/packages/react-reconciler/src/ReactFiberHydrationContext.old.js b/packages/react-reconciler/src/ReactFiberHydrationContext.old.js index fd0dd8e99a5b0..654de3f9a2894 100644 --- a/packages/react-reconciler/src/ReactFiberHydrationContext.old.js +++ b/packages/react-reconciler/src/ReactFiberHydrationContext.old.js @@ -261,6 +261,8 @@ function tryHydrate(fiber, nextInstance) { const instance = canHydrateInstance(nextInstance, type, props); if (instance !== null) { fiber.stateNode = (instance: Instance); + hydrationParentFiber = fiber; + nextHydratableInstance = getFirstHydratableChild(instance); return true; } return false; @@ -270,6 +272,9 @@ function tryHydrate(fiber, nextInstance) { const textInstance = canHydrateTextInstance(nextInstance, text); if (textInstance !== null) { fiber.stateNode = (textInstance: TextInstance); + hydrationParentFiber = fiber; + // Text Instances don't have children so there's nothing to hydrate. + nextHydratableInstance = null; return true; } return false; @@ -294,6 +299,10 @@ function tryHydrate(fiber, nextInstance) { ); dehydratedFragment.return = fiber; fiber.child = dehydratedFragment; + hydrationParentFiber = fiber; + // While a Suspense Instance does have children, we won't step into + // it during the first pass. Instead, we'll reenter it later. + nextHydratableInstance = null; return true; } } @@ -322,6 +331,7 @@ function tryToClaimNextHydratableInstance(fiber: Fiber): void { // 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); @@ -333,13 +343,8 @@ function tryToClaimNextHydratableInstance(fiber: Fiber): void { // 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( - (hydrationParentFiber: any), - firstAttemptedInstance, - ); + deleteHydratableInstance(prevHydrationParentFiber, firstAttemptedInstance); } - hydrationParentFiber = fiber; - nextHydratableInstance = getFirstHydratableChild((nextInstance: any)); } function prepareToHydrateHostInstance(