From 6ff23f2a5da0e60fa008cef97529469945618d06 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Markb=C3=A5ge?= Date: Tue, 15 Oct 2019 19:53:20 -0700 Subject: [PATCH] Change retry priority to "Never" for dehydrated boundaries (#17105) 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 | 30 +++---- 4 files changed, 102 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 5c238f88b6592..968779d23ff92 100644 --- a/packages/react-dom/src/__tests__/ReactDOMServerPartialHydration-test.internal.js +++ b/packages/react-dom/src/__tests__/ReactDOMServerPartialHydration-test.internal.js @@ -77,6 +77,7 @@ describe('ReactDOMServerPartialHydration', () => { ReactFeatureFlags = require('shared/ReactFeatureFlags'); ReactFeatureFlags.enableSuspenseCallback = true; ReactFeatureFlags.enableFlareAPI = true; + ReactFeatureFlags.debugRenderPhaseSideEffectsForStrictMode = false; React = require('react'); ReactDOM = require('react-dom'); @@ -2475,4 +2476,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 675d444f3ac05..a6855d9b90bbe 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..63c7eefca591d 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,7 +2397,7 @@ 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; } @@ -2403,7 +2405,7 @@ export function retryDehydratedSuspenseBoundary(boundaryFiber: Fiber) { } 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) {