From 3c4c46c429ec454fad5ec786ec668de38288a34c Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Mon, 14 Oct 2019 09:12:48 -0700 Subject: [PATCH] Change retry priority to "Never" for dehydrated boundaries This changes the "default" retryTime to NoWork which schedules at Normal pri. Dehydrated bouundaries normally hydrate at Never priority except when they retry where we accidentally increased them to Normal because Never was used as the default value. This changes it so NoWork is the default. Dehydrated boundaries however get initialized to Never as the default. Therefore they now hydrate as Never pri unless their priority gets increased by a forced rerender or selective hydration. This revealed that erroring at this Never priority can cause an infinite rerender. So I fixed that too. --- ...DOMServerPartialHydration-test.internal.js | 82 +++++++++++++++++++ .../src/ReactFiberBeginWork.js | 2 +- .../src/ReactFiberSuspenseComponent.js | 4 +- .../src/ReactFiberWorkLoop.js | 31 +++---- 4 files changed, 103 insertions(+), 16 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMServerPartialHydration-test.internal.js b/packages/react-dom/src/__tests__/ReactDOMServerPartialHydration-test.internal.js index dffe284c69b25..4cc36c0b044d8 100644 --- a/packages/react-dom/src/__tests__/ReactDOMServerPartialHydration-test.internal.js +++ b/packages/react-dom/src/__tests__/ReactDOMServerPartialHydration-test.internal.js @@ -78,6 +78,7 @@ describe('ReactDOMServerPartialHydration', () => { ReactFeatureFlags.enableSuspenseServerRenderer = true; ReactFeatureFlags.enableSuspenseCallback = true; ReactFeatureFlags.enableFlareAPI = true; + ReactFeatureFlags.debugRenderPhaseSideEffectsForStrictMode = false; React = require('react'); ReactDOM = require('react-dom'); @@ -2471,4 +2472,85 @@ describe('ReactDOMServerPartialHydration', () => { document.body.removeChild(container); }); + + it('finishes normal pri work before continuing to hydrate a retry', async () => { + let suspend = false; + let resolve; + let promise = new Promise(resolvePromise => (resolve = resolvePromise)); + let ref = React.createRef(); + + function Child() { + if (suspend) { + throw promise; + } else { + Scheduler.unstable_yieldValue('Child'); + return 'Hello'; + } + } + + function Sibling() { + Scheduler.unstable_yieldValue('Sibling'); + React.useLayoutEffect(() => { + Scheduler.unstable_yieldValue('Commit Sibling'); + }); + return 'World'; + } + + // Avoid rerendering the tree by hoisting it. + const tree = ( + + + + + + ); + + function App({showSibling}) { + return ( +
+ {tree} + {showSibling ? : null} +
+ ); + } + + suspend = false; + let finalHTML = ReactDOMServer.renderToString(); + expect(Scheduler).toHaveYielded(['Child']); + + let container = document.createElement('div'); + container.innerHTML = finalHTML; + + suspend = true; + let root = ReactDOM.unstable_createRoot(container, {hydrate: true}); + root.render(); + expect(Scheduler).toFlushAndYield([]); + + expect(ref.current).toBe(null); + expect(container.textContent).toBe('Hello'); + + // Resolving the promise should continue hydration + suspend = false; + resolve(); + await promise; + + Scheduler.unstable_advanceTime(100); + + // Before we have a chance to flush it, we'll also render an update. + root.render(); + + // When we flush we expect the Normal pri render to take priority + // over hydration. + expect(Scheduler).toFlushAndYieldThrough(['Sibling', 'Commit Sibling']); + + // We shouldn't have hydrated the child yet. + expect(ref.current).toBe(null); + // But we did have a chance to update the content. + expect(container.textContent).toBe('HelloWorld'); + + expect(Scheduler).toFlushAndYield(['Child']); + + // Now we're hydrated. + expect(ref.current).not.toBe(null); + }); }); diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.js b/packages/react-reconciler/src/ReactFiberBeginWork.js index 1e613daac5b04..93a52707bf777 100644 --- a/packages/react-reconciler/src/ReactFiberBeginWork.js +++ b/packages/react-reconciler/src/ReactFiberBeginWork.js @@ -1482,7 +1482,7 @@ function validateFunctionComponentInDev(workInProgress: Fiber, Component: any) { const SUSPENDED_MARKER: SuspenseState = { dehydrated: null, - retryTime: Never, + retryTime: NoWork, }; function shouldRemainOnFallback( diff --git a/packages/react-reconciler/src/ReactFiberSuspenseComponent.js b/packages/react-reconciler/src/ReactFiberSuspenseComponent.js index dfb6964d64e28..4d29159d270cb 100644 --- a/packages/react-reconciler/src/ReactFiberSuspenseComponent.js +++ b/packages/react-reconciler/src/ReactFiberSuspenseComponent.js @@ -35,7 +35,9 @@ export type SuspenseState = {| // to check things like isSuspenseInstancePending. dehydrated: null | SuspenseInstance, // Represents the earliest expiration time we should attempt to hydrate - // a dehydrated boundary at. Never is the default. + // a dehydrated boundary at. + // Never is the default for dehydrated boundaries. + // NoWork is the default for normal boundaries, which turns into "normal" pri. retryTime: ExpirationTime, |}; diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.js b/packages/react-reconciler/src/ReactFiberWorkLoop.js index 0ff8ebe047699..27daab3503139 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.js @@ -741,17 +741,19 @@ function finishConcurrentRender( // statement, but eslint doesn't know about invariant, so it complains // if I do. eslint-disable-next-line no-fallthrough case RootErrored: { - if (expirationTime !== Idle) { - // If this was an async render, the error may have happened due to - // a mutation in a concurrent event. Try rendering one more time, - // synchronously, to see if the error goes away. If there are - // lower priority updates, let's include those, too, in case they - // fix the inconsistency. Render at Idle to include all updates. - markRootExpiredAtTime(root, Idle); - break; - } - // Commit the root in its errored state. - commitRoot(root); + // If this was an async render, the error may have happened due to + // a mutation in a concurrent event. Try rendering one more time, + // synchronously, to see if the error goes away. If there are + // lower priority updates, let's include those, too, in case they + // fix the inconsistency. Render at Idle to include all updates. + // If it was Idle or Never or some not-yet-invented time, render + // at that time. + markRootExpiredAtTime( + root, + expirationTime > Idle ? Idle : expirationTime, + ); + // We assume that this second render pass will be synchronous + // and therefore not hit this path again. break; } case RootSuspended: { @@ -2376,7 +2378,7 @@ function retryTimedOutBoundary( // previously was rendered in its fallback state. One of the promises that // suspended it has resolved, which means at least part of the tree was // likely unblocked. Try rendering again, at a new expiration time. - if (retryTime === Never) { + if (retryTime === NoWork) { const suspenseConfig = null; // Retries don't carry over the already committed update. const currentTime = requestCurrentTime(); retryTime = computeExpirationForFiber( @@ -2395,15 +2397,16 @@ function retryTimedOutBoundary( export function retryDehydratedSuspenseBoundary(boundaryFiber: Fiber) { const suspenseState: null | SuspenseState = boundaryFiber.memoizedState; - let retryTime = Never; + let retryTime = NoWork; if (suspenseState !== null) { retryTime = suspenseState.retryTime; } + console.log('retry at', retryTime); retryTimedOutBoundary(boundaryFiber, retryTime); } export function resolveRetryThenable(boundaryFiber: Fiber, thenable: Thenable) { - let retryTime = Never; // Default + let retryTime = NoWork; // Default let retryCache: WeakSet | Set | null; if (enableSuspenseServerRenderer) { switch (boundaryFiber.tag) {