From c71d3abd6027c27a6fd744a3810c5d163ef639cb Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Wed, 13 Oct 2021 14:12:56 -0400 Subject: [PATCH 1/2] Handle render phase updates explicitly We fire a warning in development if a component is updated during the render phase (with the exception of local hook updates, which have their own defined behavior). Because it's not a supported React pattern, we don't have that many tests that trigger this path. But it is meant to have reasonable semantics when it does happen, so that if it accidentally ships to production, the app doesn't crash unnecessarily. The behavior is not super well-defined, though. There are also some _internal_ React implementation details that intentionally to rely on this behavior. Most prominently, selective hydration and useOpaqueIdentifier. I need to tweak the behavior of render phase updates slightly as part of a fix for useOpaqueIdentifier. This shouldn't cause a user-facing change in behavior outside of useOpaqueIdentifier, but it does require that we explicitly model render phase updates. --- .../src/ReactFiberWorkLoop.new.js | 136 ++++++++++-------- .../src/ReactFiberWorkLoop.old.js | 136 ++++++++++-------- 2 files changed, 148 insertions(+), 124 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js index aaeac5fdb22..1734d9a6912 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js @@ -454,86 +454,99 @@ export function scheduleUpdateOnFiber( eventTime: number, ): FiberRoot | null { checkForNestedUpdates(); - warnAboutRenderPhaseUpdatesInDEV(fiber); const root = markUpdateLaneFromFiberToRoot(fiber, lane); if (root === null) { return null; } - if (enableUpdaterTracking) { - if (isDevToolsPresent) { - addFiberToLanesMap(root, fiber, lane); - } - } - // Mark that the root has a pending update. markRootUpdated(root, lane, eventTime); - if (enableProfilerTimer && enableProfilerNestedUpdateScheduledHook) { - if ( - (executionContext & CommitContext) !== NoContext && - root === rootCommittingMutationOrLayoutEffects - ) { - if (fiber.mode & ProfileMode) { - let current = fiber; - while (current !== null) { - if (current.tag === Profiler) { - const {id, onNestedUpdateScheduled} = current.memoizedProps; - if (typeof onNestedUpdateScheduled === 'function') { - onNestedUpdateScheduled(id); + if ( + (executionContext & RenderContext) !== NoLanes && + root === workInProgressRoot + ) { + // This update was dispatched during the render phase. This is a mistake + // if the update originates from user space (with the exception of local + // hook updates, which are handled differently and don't reach this + // function), but there are some internal React features that use this as + // an implementation detail, like selective hydration + // and useOpaqueIdentifier. + warnAboutRenderPhaseUpdatesInDEV(fiber); + } else { + // This is a normal update, scheduled from outside the render phase. For + // example, during an input event. + if (enableUpdaterTracking) { + if (isDevToolsPresent) { + addFiberToLanesMap(root, fiber, lane); + } + } + + if (enableProfilerTimer && enableProfilerNestedUpdateScheduledHook) { + if ( + (executionContext & CommitContext) !== NoContext && + root === rootCommittingMutationOrLayoutEffects + ) { + if (fiber.mode & ProfileMode) { + let current = fiber; + while (current !== null) { + if (current.tag === Profiler) { + const {id, onNestedUpdateScheduled} = current.memoizedProps; + if (typeof onNestedUpdateScheduled === 'function') { + onNestedUpdateScheduled(id); + } } + current = current.return; } - current = current.return; } } } - } - // TODO: Consolidate with `isInterleavedUpdate` check - if (root === workInProgressRoot) { - // Received an update to a tree that's in the middle of rendering. Mark - // that there was an interleaved update work on this root. Unless the - // `deferRenderPhaseUpdateToNextBatch` flag is off and this is a render - // phase update. In that case, we don't treat render phase updates as if - // they were interleaved, for backwards compat reasons. + // TODO: Consolidate with `isInterleavedUpdate` check + if (root === workInProgressRoot) { + // Received an update to a tree that's in the middle of rendering. Mark + // that there was an interleaved update work on this root. Unless the + // `deferRenderPhaseUpdateToNextBatch` flag is off and this is a render + // phase update. In that case, we don't treat render phase updates as if + // they were interleaved, for backwards compat reasons. + if ( + deferRenderPhaseUpdateToNextBatch || + (executionContext & RenderContext) === NoContext + ) { + workInProgressRootUpdatedLanes = mergeLanes( + workInProgressRootUpdatedLanes, + lane, + ); + } + if (workInProgressRootExitStatus === RootSuspendedWithDelay) { + // The root already suspended with a delay, which means this render + // definitely won't finish. Since we have a new update, let's mark it as + // suspended now, right before marking the incoming update. This has the + // effect of interrupting the current render and switching to the update. + // TODO: Make sure this doesn't override pings that happen while we've + // already started rendering. + markRootSuspended(root, workInProgressRootRenderLanes); + } + } + + ensureRootIsScheduled(root, eventTime); if ( - deferRenderPhaseUpdateToNextBatch || - (executionContext & RenderContext) === NoContext + lane === SyncLane && + executionContext === NoContext && + (fiber.mode & ConcurrentMode) === NoMode && + // Treat `act` as if it's inside `batchedUpdates`, even in legacy mode. + !(__DEV__ && ReactCurrentActQueue.isBatchingLegacy) ) { - workInProgressRootUpdatedLanes = mergeLanes( - workInProgressRootUpdatedLanes, - lane, - ); - } - if (workInProgressRootExitStatus === RootSuspendedWithDelay) { - // The root already suspended with a delay, which means this render - // definitely won't finish. Since we have a new update, let's mark it as - // suspended now, right before marking the incoming update. This has the - // effect of interrupting the current render and switching to the update. - // TODO: Make sure this doesn't override pings that happen while we've - // already started rendering. - markRootSuspended(root, workInProgressRootRenderLanes); + // Flush the synchronous work now, unless we're already working or inside + // a batch. This is intentionally inside scheduleUpdateOnFiber instead of + // scheduleCallbackForFiber to preserve the ability to schedule a callback + // without immediately flushing it. We only do this for user-initiated + // updates, to preserve historical behavior of legacy mode. + resetRenderTimer(); + flushSyncCallbacksOnlyInLegacyMode(); } } - - ensureRootIsScheduled(root, eventTime); - if ( - lane === SyncLane && - executionContext === NoContext && - (fiber.mode & ConcurrentMode) === NoMode && - // Treat `act` as if it's inside `batchedUpdates`, even in legacy mode. - !(__DEV__ && ReactCurrentActQueue.isBatchingLegacy) - ) { - // Flush the synchronous work now, unless we're already working or inside - // a batch. This is intentionally inside scheduleUpdateOnFiber instead of - // scheduleCallbackForFiber to preserve the ability to schedule a callback - // without immediately flushing it. We only do this for user-initiated - // updates, to preserve historical behavior of legacy mode. - resetRenderTimer(); - flushSyncCallbacksOnlyInLegacyMode(); - } - return root; } @@ -2697,7 +2710,6 @@ function warnAboutRenderPhaseUpdatesInDEV(fiber) { if (__DEV__) { if ( ReactCurrentDebugFiberIsRenderingInDEV && - (executionContext & RenderContext) !== NoContext && !getIsUpdatingOpaqueValueInRenderPhaseInDEV() ) { switch (fiber.tag) { diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js index 67984298a5d..6ad0298b884 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js @@ -454,86 +454,99 @@ export function scheduleUpdateOnFiber( eventTime: number, ): FiberRoot | null { checkForNestedUpdates(); - warnAboutRenderPhaseUpdatesInDEV(fiber); const root = markUpdateLaneFromFiberToRoot(fiber, lane); if (root === null) { return null; } - if (enableUpdaterTracking) { - if (isDevToolsPresent) { - addFiberToLanesMap(root, fiber, lane); - } - } - // Mark that the root has a pending update. markRootUpdated(root, lane, eventTime); - if (enableProfilerTimer && enableProfilerNestedUpdateScheduledHook) { - if ( - (executionContext & CommitContext) !== NoContext && - root === rootCommittingMutationOrLayoutEffects - ) { - if (fiber.mode & ProfileMode) { - let current = fiber; - while (current !== null) { - if (current.tag === Profiler) { - const {id, onNestedUpdateScheduled} = current.memoizedProps; - if (typeof onNestedUpdateScheduled === 'function') { - onNestedUpdateScheduled(id); + if ( + (executionContext & RenderContext) !== NoLanes && + root === workInProgressRoot + ) { + // This update was dispatched during the render phase. This is a mistake + // if the update originates from user space (with the exception of local + // hook updates, which are handled differently and don't reach this + // function), but there are some internal React features that use this as + // an implementation detail, like selective hydration + // and useOpaqueIdentifier. + warnAboutRenderPhaseUpdatesInDEV(fiber); + } else { + // This is a normal update, scheduled from outside the render phase. For + // example, during an input event. + if (enableUpdaterTracking) { + if (isDevToolsPresent) { + addFiberToLanesMap(root, fiber, lane); + } + } + + if (enableProfilerTimer && enableProfilerNestedUpdateScheduledHook) { + if ( + (executionContext & CommitContext) !== NoContext && + root === rootCommittingMutationOrLayoutEffects + ) { + if (fiber.mode & ProfileMode) { + let current = fiber; + while (current !== null) { + if (current.tag === Profiler) { + const {id, onNestedUpdateScheduled} = current.memoizedProps; + if (typeof onNestedUpdateScheduled === 'function') { + onNestedUpdateScheduled(id); + } } + current = current.return; } - current = current.return; } } } - } - // TODO: Consolidate with `isInterleavedUpdate` check - if (root === workInProgressRoot) { - // Received an update to a tree that's in the middle of rendering. Mark - // that there was an interleaved update work on this root. Unless the - // `deferRenderPhaseUpdateToNextBatch` flag is off and this is a render - // phase update. In that case, we don't treat render phase updates as if - // they were interleaved, for backwards compat reasons. + // TODO: Consolidate with `isInterleavedUpdate` check + if (root === workInProgressRoot) { + // Received an update to a tree that's in the middle of rendering. Mark + // that there was an interleaved update work on this root. Unless the + // `deferRenderPhaseUpdateToNextBatch` flag is off and this is a render + // phase update. In that case, we don't treat render phase updates as if + // they were interleaved, for backwards compat reasons. + if ( + deferRenderPhaseUpdateToNextBatch || + (executionContext & RenderContext) === NoContext + ) { + workInProgressRootUpdatedLanes = mergeLanes( + workInProgressRootUpdatedLanes, + lane, + ); + } + if (workInProgressRootExitStatus === RootSuspendedWithDelay) { + // The root already suspended with a delay, which means this render + // definitely won't finish. Since we have a new update, let's mark it as + // suspended now, right before marking the incoming update. This has the + // effect of interrupting the current render and switching to the update. + // TODO: Make sure this doesn't override pings that happen while we've + // already started rendering. + markRootSuspended(root, workInProgressRootRenderLanes); + } + } + + ensureRootIsScheduled(root, eventTime); if ( - deferRenderPhaseUpdateToNextBatch || - (executionContext & RenderContext) === NoContext + lane === SyncLane && + executionContext === NoContext && + (fiber.mode & ConcurrentMode) === NoMode && + // Treat `act` as if it's inside `batchedUpdates`, even in legacy mode. + !(__DEV__ && ReactCurrentActQueue.isBatchingLegacy) ) { - workInProgressRootUpdatedLanes = mergeLanes( - workInProgressRootUpdatedLanes, - lane, - ); - } - if (workInProgressRootExitStatus === RootSuspendedWithDelay) { - // The root already suspended with a delay, which means this render - // definitely won't finish. Since we have a new update, let's mark it as - // suspended now, right before marking the incoming update. This has the - // effect of interrupting the current render and switching to the update. - // TODO: Make sure this doesn't override pings that happen while we've - // already started rendering. - markRootSuspended(root, workInProgressRootRenderLanes); + // Flush the synchronous work now, unless we're already working or inside + // a batch. This is intentionally inside scheduleUpdateOnFiber instead of + // scheduleCallbackForFiber to preserve the ability to schedule a callback + // without immediately flushing it. We only do this for user-initiated + // updates, to preserve historical behavior of legacy mode. + resetRenderTimer(); + flushSyncCallbacksOnlyInLegacyMode(); } } - - ensureRootIsScheduled(root, eventTime); - if ( - lane === SyncLane && - executionContext === NoContext && - (fiber.mode & ConcurrentMode) === NoMode && - // Treat `act` as if it's inside `batchedUpdates`, even in legacy mode. - !(__DEV__ && ReactCurrentActQueue.isBatchingLegacy) - ) { - // Flush the synchronous work now, unless we're already working or inside - // a batch. This is intentionally inside scheduleUpdateOnFiber instead of - // scheduleCallbackForFiber to preserve the ability to schedule a callback - // without immediately flushing it. We only do this for user-initiated - // updates, to preserve historical behavior of legacy mode. - resetRenderTimer(); - flushSyncCallbacksOnlyInLegacyMode(); - } - return root; } @@ -2697,7 +2710,6 @@ function warnAboutRenderPhaseUpdatesInDEV(fiber) { if (__DEV__) { if ( ReactCurrentDebugFiberIsRenderingInDEV && - (executionContext & RenderContext) !== NoContext && !getIsUpdatingOpaqueValueInRenderPhaseInDEV() ) { switch (fiber.tag) { From aca927845f36f32f5b56668ba5cdc2bb3956588b Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Wed, 13 Oct 2021 17:45:21 -0400 Subject: [PATCH 2/2] Bugfix: Nested useOpaqueIdentifier calls MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes an issue where multiple useOpaqueIdentifier hooks are upgraded to client ids within the same render. The way the upgrade works is that useOpaqueIdentifier schedules a render phase update then throws an error to trigger React's error recovery mechanism. The normal error recovery mechanism is designed for errors that occur as a result of interleaved mutations, so we usually only retry a single time, synchronously, before giving up. useOpaqueIdentifier is different because the error its throws when upgrading is not caused by an interleaved mutation. Rather, it happens when an ID is referenced for the first time inside a client-rendered tree (i.e. sommething that wasn't part of the initial server render). The fact that it relies on the error recovery mechanism is an implementation detail. And a single recovery attempt may be insufficient. For example, if a parent and a child component may reference different ids, and both are mounted as a result of the same client update, that will trigger two separate error recovery attempts. Because render phase updates are not allowed when triggered from userspace — we log a warning in developement to prevent them — we can assume that if something does update during the render phase, it is one of our "legit" implementation details like useOpaqueIdentifier. So we can keep retrying until we succeed — up to a limit, to protect against inifite loops. I chose 50 since that's the limit we use for commit phase updates. --- .../ReactDOMServerIntegrationHooks-test.js | 35 +++++++++++ .../src/ReactFiberWorkLoop.new.js | 61 ++++++++++++------- .../src/ReactFiberWorkLoop.old.js | 61 ++++++++++++------- ...tIncrementalErrorHandling-test.internal.js | 59 ++++++++++++++++++ 4 files changed, 170 insertions(+), 46 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationHooks-test.js b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationHooks-test.js index a27954785f7..daf34533141 100644 --- a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationHooks-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationHooks-test.js @@ -1975,5 +1975,40 @@ describe('ReactDOMServerHooks', () => { container.getElementsByTagName('span')[0].getAttribute('id'), ).not.toBeNull(); }); + + it('useOpaqueIdentifier with multiple ids in nested components', async () => { + function DivWithId({id, children}) { + return
{children}
; + } + + let setShowMore; + function App() { + const outerId = useOpaqueIdentifier(); + const innerId = useOpaqueIdentifier(); + const [showMore, _setShowMore] = useState(false); + setShowMore = _setShowMore; + return showMore ? ( + + + + ) : null; + } + + const container = document.createElement('div'); + container.innerHTML = ReactDOMServer.renderToString(); + + await act(async () => { + ReactDOM.hydrateRoot(container, ); + }); + + // Show additional content that wasn't part of the initial server- + // rendered repsonse. + await act(async () => { + setShowMore(true); + }); + const [div1, div2] = container.getElementsByTagName('div'); + expect(typeof div1.getAttribute('id')).toBe('string'); + expect(typeof div2.getAttribute('id')).toBe('string'); + }); }); }); diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js index 1734d9a6912..884f8cc22aa 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js @@ -294,7 +294,9 @@ let workInProgressRootIncludedLanes: Lanes = NoLanes; // includes unprocessed updates, not work in bailed out children. let workInProgressRootSkippedLanes: Lanes = NoLanes; // Lanes that were updated (in an interleaved event) during this render. -let workInProgressRootUpdatedLanes: Lanes = NoLanes; +let workInProgressRootInterleavedUpdatedLanes: Lanes = NoLanes; +// Lanes that were updated during the render phase (*not* an interleaved event). +let workInProgressRootRenderPhaseUpdatedLanes: Lanes = NoLanes; // Lanes that were pinged (in an interleaved event) during this render. let workInProgressRootPingedLanes: Lanes = NoLanes; @@ -474,6 +476,12 @@ export function scheduleUpdateOnFiber( // an implementation detail, like selective hydration // and useOpaqueIdentifier. warnAboutRenderPhaseUpdatesInDEV(fiber); + + // Track lanes that were updated during the render phase + workInProgressRootRenderPhaseUpdatedLanes = mergeLanes( + workInProgressRootRenderPhaseUpdatedLanes, + lane, + ); } else { // This is a normal update, scheduled from outside the render phase. For // example, during an input event. @@ -514,8 +522,8 @@ export function scheduleUpdateOnFiber( deferRenderPhaseUpdateToNextBatch || (executionContext & RenderContext) === NoContext ) { - workInProgressRootUpdatedLanes = mergeLanes( - workInProgressRootUpdatedLanes, + workInProgressRootInterleavedUpdatedLanes = mergeLanes( + workInProgressRootInterleavedUpdatedLanes, lane, ); } @@ -878,7 +886,25 @@ function recoverFromConcurrentError(root, errorRetryLanes) { clearContainer(root.containerInfo); } - const exitStatus = renderRootSync(root, errorRetryLanes); + let exitStatus; + + const MAX_ERROR_RETRY_ATTEMPTS = 50; + for (let i = 0; i < MAX_ERROR_RETRY_ATTEMPTS; i++) { + exitStatus = renderRootSync(root, errorRetryLanes); + if ( + exitStatus === RootErrored && + workInProgressRootRenderPhaseUpdatedLanes !== NoLanes + ) { + // There was a render phase update during this render. This was likely a + // useOpaqueIdentifier hook upgrading itself to a client ID. Try rendering + // again. This time, the component will use a client ID and will proceed + // without throwing. If multiple IDs upgrade as a result of the same + // update, we will have to do multiple render passes. To protect against + // an inifinite loop, eventually we'll give up. + continue; + } + break; + } executionContext = prevExecutionContext; @@ -1055,7 +1081,10 @@ function markRootSuspended(root, suspendedLanes) { // TODO: Lol maybe there's a better way to factor this besides this // obnoxiously named function :) suspendedLanes = removeLanes(suspendedLanes, workInProgressRootPingedLanes); - suspendedLanes = removeLanes(suspendedLanes, workInProgressRootUpdatedLanes); + suspendedLanes = removeLanes( + suspendedLanes, + workInProgressRootInterleavedUpdatedLanes, + ); markRootSuspended_dontCallThisOneDirectly(root, suspendedLanes); } @@ -1081,19 +1110,6 @@ function performSyncWorkOnRoot(root) { let exitStatus = renderRootSync(root, lanes); if (root.tag !== LegacyRoot && exitStatus === RootErrored) { - const prevExecutionContext = executionContext; - executionContext |= RetryAfterError; - - // If an error occurred during hydration, - // discard server response and fall back to client side render. - if (root.isDehydrated) { - root.isDehydrated = false; - if (__DEV__) { - errorHydratingContainer(root.containerInfo); - } - clearContainer(root.containerInfo); - } - // If something threw an error, try rendering one more time. We'll render // synchronously to block concurrent data mutations, and we'll includes // all pending updates are included. If it still fails after the second @@ -1101,10 +1117,8 @@ function performSyncWorkOnRoot(root) { const errorRetryLanes = getLanesToRetrySynchronouslyOnError(root); if (errorRetryLanes !== NoLanes) { lanes = errorRetryLanes; - exitStatus = renderRootSync(root, lanes); + exitStatus = recoverFromConcurrentError(root, errorRetryLanes); } - - executionContext = prevExecutionContext; } if (exitStatus === RootFatalErrored) { @@ -1313,7 +1327,8 @@ function prepareFreshStack(root: FiberRoot, lanes: Lanes) { workInProgressRootExitStatus = RootIncomplete; workInProgressRootFatalError = null; workInProgressRootSkippedLanes = NoLanes; - workInProgressRootUpdatedLanes = NoLanes; + workInProgressRootInterleavedUpdatedLanes = NoLanes; + workInProgressRootRenderPhaseUpdatedLanes = NoLanes; workInProgressRootPingedLanes = NoLanes; enqueueInterleavedUpdates(); @@ -1456,7 +1471,7 @@ export function renderDidSuspendDelayIfPossible(): void { if ( workInProgressRoot !== null && (includesNonIdleWork(workInProgressRootSkippedLanes) || - includesNonIdleWork(workInProgressRootUpdatedLanes)) + includesNonIdleWork(workInProgressRootInterleavedUpdatedLanes)) ) { // Mark the current render as suspended so that we switch to working on // the updates that were skipped. Usually we only suspend at the end of diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js index 6ad0298b884..db7a3de837b 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js @@ -294,7 +294,9 @@ let workInProgressRootIncludedLanes: Lanes = NoLanes; // includes unprocessed updates, not work in bailed out children. let workInProgressRootSkippedLanes: Lanes = NoLanes; // Lanes that were updated (in an interleaved event) during this render. -let workInProgressRootUpdatedLanes: Lanes = NoLanes; +let workInProgressRootInterleavedUpdatedLanes: Lanes = NoLanes; +// Lanes that were updated during the render phase (*not* an interleaved event). +let workInProgressRootRenderPhaseUpdatedLanes: Lanes = NoLanes; // Lanes that were pinged (in an interleaved event) during this render. let workInProgressRootPingedLanes: Lanes = NoLanes; @@ -474,6 +476,12 @@ export function scheduleUpdateOnFiber( // an implementation detail, like selective hydration // and useOpaqueIdentifier. warnAboutRenderPhaseUpdatesInDEV(fiber); + + // Track lanes that were updated during the render phase + workInProgressRootRenderPhaseUpdatedLanes = mergeLanes( + workInProgressRootRenderPhaseUpdatedLanes, + lane, + ); } else { // This is a normal update, scheduled from outside the render phase. For // example, during an input event. @@ -514,8 +522,8 @@ export function scheduleUpdateOnFiber( deferRenderPhaseUpdateToNextBatch || (executionContext & RenderContext) === NoContext ) { - workInProgressRootUpdatedLanes = mergeLanes( - workInProgressRootUpdatedLanes, + workInProgressRootInterleavedUpdatedLanes = mergeLanes( + workInProgressRootInterleavedUpdatedLanes, lane, ); } @@ -878,7 +886,25 @@ function recoverFromConcurrentError(root, errorRetryLanes) { clearContainer(root.containerInfo); } - const exitStatus = renderRootSync(root, errorRetryLanes); + let exitStatus; + + const MAX_ERROR_RETRY_ATTEMPTS = 50; + for (let i = 0; i < MAX_ERROR_RETRY_ATTEMPTS; i++) { + exitStatus = renderRootSync(root, errorRetryLanes); + if ( + exitStatus === RootErrored && + workInProgressRootRenderPhaseUpdatedLanes !== NoLanes + ) { + // There was a render phase update during this render. This was likely a + // useOpaqueIdentifier hook upgrading itself to a client ID. Try rendering + // again. This time, the component will use a client ID and will proceed + // without throwing. If multiple IDs upgrade as a result of the same + // update, we will have to do multiple render passes. To protect against + // an inifinite loop, eventually we'll give up. + continue; + } + break; + } executionContext = prevExecutionContext; @@ -1055,7 +1081,10 @@ function markRootSuspended(root, suspendedLanes) { // TODO: Lol maybe there's a better way to factor this besides this // obnoxiously named function :) suspendedLanes = removeLanes(suspendedLanes, workInProgressRootPingedLanes); - suspendedLanes = removeLanes(suspendedLanes, workInProgressRootUpdatedLanes); + suspendedLanes = removeLanes( + suspendedLanes, + workInProgressRootInterleavedUpdatedLanes, + ); markRootSuspended_dontCallThisOneDirectly(root, suspendedLanes); } @@ -1081,19 +1110,6 @@ function performSyncWorkOnRoot(root) { let exitStatus = renderRootSync(root, lanes); if (root.tag !== LegacyRoot && exitStatus === RootErrored) { - const prevExecutionContext = executionContext; - executionContext |= RetryAfterError; - - // If an error occurred during hydration, - // discard server response and fall back to client side render. - if (root.isDehydrated) { - root.isDehydrated = false; - if (__DEV__) { - errorHydratingContainer(root.containerInfo); - } - clearContainer(root.containerInfo); - } - // If something threw an error, try rendering one more time. We'll render // synchronously to block concurrent data mutations, and we'll includes // all pending updates are included. If it still fails after the second @@ -1101,10 +1117,8 @@ function performSyncWorkOnRoot(root) { const errorRetryLanes = getLanesToRetrySynchronouslyOnError(root); if (errorRetryLanes !== NoLanes) { lanes = errorRetryLanes; - exitStatus = renderRootSync(root, lanes); + exitStatus = recoverFromConcurrentError(root, errorRetryLanes); } - - executionContext = prevExecutionContext; } if (exitStatus === RootFatalErrored) { @@ -1313,7 +1327,8 @@ function prepareFreshStack(root: FiberRoot, lanes: Lanes) { workInProgressRootExitStatus = RootIncomplete; workInProgressRootFatalError = null; workInProgressRootSkippedLanes = NoLanes; - workInProgressRootUpdatedLanes = NoLanes; + workInProgressRootInterleavedUpdatedLanes = NoLanes; + workInProgressRootRenderPhaseUpdatedLanes = NoLanes; workInProgressRootPingedLanes = NoLanes; enqueueInterleavedUpdates(); @@ -1456,7 +1471,7 @@ export function renderDidSuspendDelayIfPossible(): void { if ( workInProgressRoot !== null && (includesNonIdleWork(workInProgressRootSkippedLanes) || - includesNonIdleWork(workInProgressRootUpdatedLanes)) + includesNonIdleWork(workInProgressRootInterleavedUpdatedLanes)) ) { // Mark the current render as suspended so that we switch to working on // the updates that were skipped. Usually we only suspend at the end of diff --git a/packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js b/packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js index 716771a3778..cc842bd16d2 100644 --- a/packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js @@ -1915,6 +1915,65 @@ describe('ReactIncrementalErrorHandling', () => { expect(root).toMatchRenderedOutput('Everything is fine.'); }); + it("does not infinite loop if there's a render phase update in the same render as an error", async () => { + // useOpaqueIdentifier uses an render phase update as an implementation + // detail. When an error is accompanied by a render phase update, we assume + // that it comes from useOpaqueIdentifier, because render phase updates + // triggered from userspace are not allowed (we log a warning). So we keep + // attempting to recover until no more opaque identifiers need to be + // upgraded. However, we should give up after some point to prevent an + // infinite loop in the case where there is (by accident) a render phase + // triggered from userspace. + + spyOnDev(console, 'error'); + + let numberOfThrows = 0; + + let setStateInRenderPhase; + function Child() { + const [, setState] = React.useState(0); + setStateInRenderPhase = setState; + return 'All good'; + } + + function App({shouldThrow}) { + if (shouldThrow) { + setStateInRenderPhase(); + numberOfThrows++; + throw new Error('Oops!'); + } + return ; + } + + const root = ReactNoop.createRoot(); + await act(async () => { + root.render(); + }); + expect(root).toMatchRenderedOutput('All good'); + + let error; + try { + await act(async () => { + root.render(); + }); + } catch (e) { + error = e; + } + + expect(error.message).toBe('Oops!'); + expect(numberOfThrows < 100).toBe(true); + + if (__DEV__) { + expect(console.error).toHaveBeenCalledTimes(2); + expect(console.error.calls.argsFor(0)[0]).toContain( + 'Cannot update a component (`%s`) while rendering a different component', + ); + expect(console.error.calls.argsFor(1)[0]).toContain( + 'The above error occurred in the component', + ); + } + }); + if (global.__PERSISTENT__) { it('regression test: should fatal if error is thrown at the root', () => { const root = ReactNoop.createRoot();