From a148cfc7bd498553ab7ae86edeb8117919c27175 Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Mon, 25 Feb 2019 14:28:24 -0800 Subject: [PATCH 1/5] Split props changing from permanent fallback state These will need different logic. In this commit, no logic has changed, only moved. --- .../src/ReactFiberBeginWork.js | 91 +++++++++++-------- 1 file changed, 52 insertions(+), 39 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.js b/packages/react-reconciler/src/ReactFiberBeginWork.js index e615c98c9f16f..02e20f9558767 100644 --- a/packages/react-reconciler/src/ReactFiberBeginWork.js +++ b/packages/react-reconciler/src/ReactFiberBeginWork.js @@ -1631,6 +1631,42 @@ function updateSuspenseComponent( return next; } +function retrySuspenseComponentWithoutHydrating( + current: Fiber, + workInProgress: Fiber, + renderExpirationTime: ExpirationTime, +) { + // Detach from the current dehydrated boundary. + current.alternate = null; + workInProgress.alternate = null; + + // Insert a deletion in the effect list. + let returnFiber = workInProgress.return; + invariant( + returnFiber !== null, + 'Suspense boundaries are never on the root. ' + + 'This is probably a bug in React.', + ); + const last = returnFiber.lastEffect; + if (last !== null) { + last.nextEffect = current; + returnFiber.lastEffect = current; + } else { + returnFiber.firstEffect = returnFiber.lastEffect = current; + } + current.nextEffect = null; + current.effectTag = Deletion; + + // Upgrade this work in progress to a real Suspense component. + workInProgress.tag = SuspenseComponent; + workInProgress.stateNode = null; + workInProgress.memoizedState = null; + // This is now an insertion. + workInProgress.effectTag |= Placement; + // Retry as a real Suspense component. + return updateSuspenseComponent(null, workInProgress, renderExpirationTime); +} + function updateDehydratedSuspenseComponent( current: Fiber | null, workInProgress: Fiber, @@ -1648,55 +1684,32 @@ function updateDehydratedSuspenseComponent( workInProgress.child = null; return null; } + const suspenseInstance = (current.stateNode: SuspenseInstance); + if (isSuspenseInstanceFallback(suspenseInstance)) { + // This boundary is in a permanent fallback state. In this case, we'll never + // get an update and we'll never be able to hydrate the final content. Let's just try the + // client side render instead. + return retrySuspenseComponentWithoutHydrating( + current, + workInProgress, + renderExpirationTime, + ); + } // We use childExpirationTime to indicate that a child might depend on context, so if // any context has changed, we need to treat is as if the input might have changed. const hasContextChanged = current.childExpirationTime >= renderExpirationTime; - const suspenseInstance = (current.stateNode: SuspenseInstance); - if ( - didReceiveUpdate || - hasContextChanged || - isSuspenseInstanceFallback(suspenseInstance) - ) { + if (didReceiveUpdate || hasContextChanged) { // This boundary has changed since the first render. This means that we are now unable to // hydrate it. We might still be able to hydrate it using an earlier expiration time but // during this render we can't. Instead, we're going to delete the whole subtree and // instead inject a new real Suspense boundary to take its place, which may render content // or fallback. The real Suspense boundary will suspend for a while so we have some time // to ensure it can produce real content, but all state and pending events will be lost. - - // Alternatively, this boundary is in a permanent fallback state. In this case, we'll never - // get an update and we'll never be able to hydrate the final content. Let's just try the - // client side render instead. - - // Detach from the current dehydrated boundary. - current.alternate = null; - workInProgress.alternate = null; - - // Insert a deletion in the effect list. - let returnFiber = workInProgress.return; - invariant( - returnFiber !== null, - 'Suspense boundaries are never on the root. ' + - 'This is probably a bug in React.', + return retrySuspenseComponentWithoutHydrating( + current, + workInProgress, + renderExpirationTime, ); - const last = returnFiber.lastEffect; - if (last !== null) { - last.nextEffect = current; - returnFiber.lastEffect = current; - } else { - returnFiber.firstEffect = returnFiber.lastEffect = current; - } - current.nextEffect = null; - current.effectTag = Deletion; - - // Upgrade this work in progress to a real Suspense component. - workInProgress.tag = SuspenseComponent; - workInProgress.stateNode = null; - workInProgress.memoizedState = null; - // This is now an insertion. - workInProgress.effectTag |= Placement; - // Retry as a real Suspense component. - return updateSuspenseComponent(null, workInProgress, renderExpirationTime); } else if (isSuspenseInstancePending(suspenseInstance)) { // This component is still pending more data from the server, so we can't hydrate its // content. We treat it as if this component suspended itself. It might seem as if From ad67ba8928c23f5d9ba22d7e5c202bf27d0e49d3 Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Thu, 7 Mar 2019 20:11:20 -0800 Subject: [PATCH 2/5] Delete terminal fallback content in first pass If the dehydrated suspense boundary's fallback content is terminal there is nothing to show. We need to get actual content on the screen soon. If we deprioritize that work to offscreen, then the timeout heuristics will be wrong. Therefore, if we have no current and we're already at terminal fallback state we'll immediately schedule a deletion and upgrade to real suspense. --- ...DOMServerPartialHydration-test.internal.js | 59 +++++++++++++++++++ .../src/ReactFiberBeginWork.js | 54 ++++++++++------- 2 files changed, 93 insertions(+), 20 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMServerPartialHydration-test.internal.js b/packages/react-dom/src/__tests__/ReactDOMServerPartialHydration-test.internal.js index 22e5f30f00091..256a3966cc561 100644 --- a/packages/react-dom/src/__tests__/ReactDOMServerPartialHydration-test.internal.js +++ b/packages/react-dom/src/__tests__/ReactDOMServerPartialHydration-test.internal.js @@ -703,6 +703,65 @@ describe('ReactDOMServerPartialHydration', () => { expect(ref.current).toBe(span); }); + it('replaces the fallback within the maxDuration if there is a nested suspense', async () => { + let suspend = false; + let promise = new Promise(resolvePromise => {}); + let ref = React.createRef(); + + function Child() { + if (suspend) { + throw promise; + } else { + return 'Hello'; + } + } + + function InnerChild() { + // Always suspends indefinitely + throw promise; + } + + function App() { + return ( +
+ + + + + + + + +
+ ); + } + + // First we render the final HTML. With the streaming renderer + // this may have suspense points on the server but here we want + // to test the completed HTML. Don't suspend on the server. + suspend = true; + let finalHTML = ReactDOMServer.renderToString(); + let container = document.createElement('div'); + container.innerHTML = finalHTML; + + expect(container.getElementsByTagName('span').length).toBe(0); + + // On the client we have the data available quickly for some reason. + suspend = false; + let root = ReactDOM.unstable_createRoot(container, {hydrate: true}); + root.render(); + Scheduler.flushAll(); + // This will have exceeded the maxDuration so we should timeout. + jest.advanceTimersByTime(500); + // The boundary should longer be suspended for the middle content + // even though the inner boundary is still suspended. + + expect(container.textContent).toBe('Hello'); + + let span = container.getElementsByTagName('span')[0]; + expect(ref.current).toBe(span); + }); + it('waits for pending content to come in from the server and then hydrates it', async () => { let suspend = false; let promise = new Promise(resolvePromise => {}); diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.js b/packages/react-reconciler/src/ReactFiberBeginWork.js index 02e20f9558767..d3a24306dac25 100644 --- a/packages/react-reconciler/src/ReactFiberBeginWork.js +++ b/packages/react-reconciler/src/ReactFiberBeginWork.js @@ -1632,12 +1632,28 @@ function updateSuspenseComponent( } function retrySuspenseComponentWithoutHydrating( - current: Fiber, + current: Fiber | null, workInProgress: Fiber, renderExpirationTime: ExpirationTime, ) { + let fiberToDelete = current; + if (fiberToDelete === null) { + // We're going to delete the dehydrated boundary but we don't have a + // current. To be able to insert something in the deletion list we + // to create one. That's because our workInProgress is going to be + // upgraded so we can't use that one. + fiberToDelete = createWorkInProgress( + workInProgress, + workInProgress.pendingProps, + renderExpirationTime, + ); + fiberToDelete.return = workInProgress.return; + } else { + // This is now an insertion. + workInProgress.effectTag |= Placement; + } // Detach from the current dehydrated boundary. - current.alternate = null; + fiberToDelete.alternate = null; workInProgress.alternate = null; // Insert a deletion in the effect list. @@ -1649,20 +1665,18 @@ function retrySuspenseComponentWithoutHydrating( ); const last = returnFiber.lastEffect; if (last !== null) { - last.nextEffect = current; - returnFiber.lastEffect = current; + last.nextEffect = fiberToDelete; + returnFiber.lastEffect = fiberToDelete; } else { - returnFiber.firstEffect = returnFiber.lastEffect = current; + returnFiber.firstEffect = returnFiber.lastEffect = fiberToDelete; } - current.nextEffect = null; - current.effectTag = Deletion; + fiberToDelete.nextEffect = null; + fiberToDelete.effectTag = Deletion; // Upgrade this work in progress to a real Suspense component. workInProgress.tag = SuspenseComponent; workInProgress.stateNode = null; workInProgress.memoizedState = null; - // This is now an insertion. - workInProgress.effectTag |= Placement; // Retry as a real Suspense component. return updateSuspenseComponent(null, workInProgress, renderExpirationTime); } @@ -1672,6 +1686,17 @@ function updateDehydratedSuspenseComponent( workInProgress: Fiber, renderExpirationTime: ExpirationTime, ) { + const suspenseInstance = (workInProgress.stateNode: SuspenseInstance); + if (isSuspenseInstanceFallback(suspenseInstance)) { + // This boundary is in a permanent fallback state. In this case, we'll never + // get an update and we'll never be able to hydrate the final content. Let's just try the + // client side render instead. + return retrySuspenseComponentWithoutHydrating( + current, + workInProgress, + renderExpirationTime, + ); + } if (current === null) { // During the first pass, we'll bail out and not drill into the children. // Instead, we'll leave the content in place and try to hydrate it later. @@ -1684,17 +1709,6 @@ function updateDehydratedSuspenseComponent( workInProgress.child = null; return null; } - const suspenseInstance = (current.stateNode: SuspenseInstance); - if (isSuspenseInstanceFallback(suspenseInstance)) { - // This boundary is in a permanent fallback state. In this case, we'll never - // get an update and we'll never be able to hydrate the final content. Let's just try the - // client side render instead. - return retrySuspenseComponentWithoutHydrating( - current, - workInProgress, - renderExpirationTime, - ); - } // We use childExpirationTime to indicate that a child might depend on context, so if // any context has changed, we need to treat is as if the input might have changed. const hasContextChanged = current.childExpirationTime >= renderExpirationTime; From e33a93f587adb7457c90a7bfadcf7d0bb8e6bf86 Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Fri, 8 Mar 2019 16:42:21 -0800 Subject: [PATCH 3/5] Show failing case when there is another wrapper boundary --- ...DOMServerPartialHydration-test.internal.js | 61 +++++++++++++++++++ 1 file changed, 61 insertions(+) diff --git a/packages/react-dom/src/__tests__/ReactDOMServerPartialHydration-test.internal.js b/packages/react-dom/src/__tests__/ReactDOMServerPartialHydration-test.internal.js index 256a3966cc561..b0e62e2fc789c 100644 --- a/packages/react-dom/src/__tests__/ReactDOMServerPartialHydration-test.internal.js +++ b/packages/react-dom/src/__tests__/ReactDOMServerPartialHydration-test.internal.js @@ -762,6 +762,67 @@ describe('ReactDOMServerPartialHydration', () => { expect(ref.current).toBe(span); }); + it('replaces the fallback within the maxDuration if there is a nested suspense in a nested suspense', async () => { + let suspend = false; + let promise = new Promise(resolvePromise => {}); + let ref = React.createRef(); + + function Child() { + if (suspend) { + throw promise; + } else { + return 'Hello'; + } + } + + function InnerChild() { + // Always suspends indefinitely + throw promise; + } + + function App() { + return ( +
+ + + + + + + + + + +
+ ); + } + + // First we render the final HTML. With the streaming renderer + // this may have suspense points on the server but here we want + // to test the completed HTML. Don't suspend on the server. + suspend = true; + let finalHTML = ReactDOMServer.renderToString(); + let container = document.createElement('div'); + container.innerHTML = finalHTML; + + expect(container.getElementsByTagName('span').length).toBe(0); + + // On the client we have the data available quickly for some reason. + suspend = false; + let root = ReactDOM.unstable_createRoot(container, {hydrate: true}); + root.render(); + Scheduler.flushAll(); + // This will have exceeded the maxDuration so we should timeout. + jest.advanceTimersByTime(500); + // The boundary should longer be suspended for the middle content + // even though the inner boundary is still suspended. + + expect(container.textContent).toBe('Hello'); + + let span = container.getElementsByTagName('span')[0]; + expect(ref.current).toBe(span); + }); + it('waits for pending content to come in from the server and then hydrates it', async () => { let suspend = false; let promise = new Promise(resolvePromise => {}); From 0aed005c5988d564aae41eaf75d8b2868643f47c Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Fri, 8 Mar 2019 16:43:53 -0800 Subject: [PATCH 4/5] Revert "Delete terminal fallback content in first pass" This reverts commit ad67ba8928c23f5d9ba22d7e5c202bf27d0e49d3. --- .../src/ReactFiberBeginWork.js | 54 +++++++------------ 1 file changed, 20 insertions(+), 34 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.js b/packages/react-reconciler/src/ReactFiberBeginWork.js index d3a24306dac25..02e20f9558767 100644 --- a/packages/react-reconciler/src/ReactFiberBeginWork.js +++ b/packages/react-reconciler/src/ReactFiberBeginWork.js @@ -1632,28 +1632,12 @@ function updateSuspenseComponent( } function retrySuspenseComponentWithoutHydrating( - current: Fiber | null, + current: Fiber, workInProgress: Fiber, renderExpirationTime: ExpirationTime, ) { - let fiberToDelete = current; - if (fiberToDelete === null) { - // We're going to delete the dehydrated boundary but we don't have a - // current. To be able to insert something in the deletion list we - // to create one. That's because our workInProgress is going to be - // upgraded so we can't use that one. - fiberToDelete = createWorkInProgress( - workInProgress, - workInProgress.pendingProps, - renderExpirationTime, - ); - fiberToDelete.return = workInProgress.return; - } else { - // This is now an insertion. - workInProgress.effectTag |= Placement; - } // Detach from the current dehydrated boundary. - fiberToDelete.alternate = null; + current.alternate = null; workInProgress.alternate = null; // Insert a deletion in the effect list. @@ -1665,18 +1649,20 @@ function retrySuspenseComponentWithoutHydrating( ); const last = returnFiber.lastEffect; if (last !== null) { - last.nextEffect = fiberToDelete; - returnFiber.lastEffect = fiberToDelete; + last.nextEffect = current; + returnFiber.lastEffect = current; } else { - returnFiber.firstEffect = returnFiber.lastEffect = fiberToDelete; + returnFiber.firstEffect = returnFiber.lastEffect = current; } - fiberToDelete.nextEffect = null; - fiberToDelete.effectTag = Deletion; + current.nextEffect = null; + current.effectTag = Deletion; // Upgrade this work in progress to a real Suspense component. workInProgress.tag = SuspenseComponent; workInProgress.stateNode = null; workInProgress.memoizedState = null; + // This is now an insertion. + workInProgress.effectTag |= Placement; // Retry as a real Suspense component. return updateSuspenseComponent(null, workInProgress, renderExpirationTime); } @@ -1686,17 +1672,6 @@ function updateDehydratedSuspenseComponent( workInProgress: Fiber, renderExpirationTime: ExpirationTime, ) { - const suspenseInstance = (workInProgress.stateNode: SuspenseInstance); - if (isSuspenseInstanceFallback(suspenseInstance)) { - // This boundary is in a permanent fallback state. In this case, we'll never - // get an update and we'll never be able to hydrate the final content. Let's just try the - // client side render instead. - return retrySuspenseComponentWithoutHydrating( - current, - workInProgress, - renderExpirationTime, - ); - } if (current === null) { // During the first pass, we'll bail out and not drill into the children. // Instead, we'll leave the content in place and try to hydrate it later. @@ -1709,6 +1684,17 @@ function updateDehydratedSuspenseComponent( workInProgress.child = null; return null; } + const suspenseInstance = (current.stateNode: SuspenseInstance); + if (isSuspenseInstanceFallback(suspenseInstance)) { + // This boundary is in a permanent fallback state. In this case, we'll never + // get an update and we'll never be able to hydrate the final content. Let's just try the + // client side render instead. + return retrySuspenseComponentWithoutHydrating( + current, + workInProgress, + renderExpirationTime, + ); + } // We use childExpirationTime to indicate that a child might depend on context, so if // any context has changed, we need to treat is as if the input might have changed. const hasContextChanged = current.childExpirationTime >= renderExpirationTime; From bc068d67e9f212b08920016e272000863188b2f7 Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Fri, 8 Mar 2019 17:12:33 -0800 Subject: [PATCH 5/5] Use the new approach of leaving work at normal pri to replace fallback --- .../src/ReactFiberBeginWork.js | 31 ++++++++++++++++--- 1 file changed, 27 insertions(+), 4 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.js b/packages/react-reconciler/src/ReactFiberBeginWork.js index 02e20f9558767..f1b65633324c6 100644 --- a/packages/react-reconciler/src/ReactFiberBeginWork.js +++ b/packages/react-reconciler/src/ReactFiberBeginWork.js @@ -74,7 +74,11 @@ import { cloneChildFibers, } from './ReactChildFiber'; import {processUpdateQueue} from './ReactUpdateQueue'; -import {NoWork, Never} from './ReactFiberExpirationTime'; +import { + NoWork, + Never, + computeAsyncExpiration, +} from './ReactFiberExpirationTime'; import { ConcurrentMode, NoContext, @@ -133,7 +137,7 @@ import { createWorkInProgress, isSimpleFunctionComponent, } from './ReactFiber'; -import {retryTimedOutBoundary} from './ReactFiberScheduler'; +import {requestCurrentTime, retryTimedOutBoundary} from './ReactFiberScheduler'; const ReactCurrentOwner = ReactSharedInternals.ReactCurrentOwner; @@ -1672,10 +1676,30 @@ function updateDehydratedSuspenseComponent( workInProgress: Fiber, renderExpirationTime: ExpirationTime, ) { + const suspenseInstance = (workInProgress.stateNode: SuspenseInstance); if (current === null) { // During the first pass, we'll bail out and not drill into the children. // Instead, we'll leave the content in place and try to hydrate it later. - workInProgress.expirationTime = Never; + if (isSuspenseInstanceFallback(suspenseInstance)) { + // This is a client-only boundary. Since we won't get any content from the server + // for this, we need to schedule that at a higher priority based on when it would + // have timed out. In theory we could render it in this pass but it would have the + // wrong priority associated with it and will prevent hydration of parent path. + // Instead, we'll leave work left on it to render it in a separate commit. + + // TODO This time should be the time at which the server rendered response that is + // a parent to this boundary was displayed. However, since we currently don't have + // a protocol to transfer that time, we'll just estimate it by using the current + // time. This will mean that Suspense timeouts are slightly shifted to later than + // they should be. + let serverDisplayTime = requestCurrentTime(); + // Schedule a normal pri update to render this content. + workInProgress.expirationTime = computeAsyncExpiration(serverDisplayTime); + } else { + // We'll continue hydrating the rest at offscreen priority since we'll already + // be showing the right content coming from the server, it is no rush. + workInProgress.expirationTime = Never; + } return null; } if ((workInProgress.effectTag & DidCapture) !== NoEffect) { @@ -1684,7 +1708,6 @@ function updateDehydratedSuspenseComponent( workInProgress.child = null; return null; } - const suspenseInstance = (current.stateNode: SuspenseInstance); if (isSuspenseInstanceFallback(suspenseInstance)) { // This boundary is in a permanent fallback state. In this case, we'll never // get an update and we'll never be able to hydrate the final content. Let's just try the