diff --git a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationHooks-test.js b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationHooks-test.js index a27954785f7ea..d5d8861e5fd4d 100644 --- a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationHooks-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationHooks-test.js @@ -1975,5 +1975,55 @@ 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() { + Scheduler.unstable_yieldValue('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(); + expect(Scheduler).toHaveYielded(['App']); + + await act(async () => { + ReactDOM.hydrateRoot(container, ); + }); + expect(Scheduler).toHaveYielded(['App']); + + // Show additional content that wasn't part of the initial server- + // rendered repsonse. + await act(async () => { + setShowMore(true); + }); + expect(Scheduler).toHaveYielded([ + // First render. This one doesn't finish because the outerId is + // referenced in a client-rendered tree. We upgrade the id and retry. + 'App', + + // Second render. This time innerId needs to be upgraded, so we + // retry again. + 'App', + + // Finally everything has upgraded. + 'App', + ]); + 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 1734d9a69122c..2cc334e9c9e8e 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); } @@ -1313,7 +1342,8 @@ function prepareFreshStack(root: FiberRoot, lanes: Lanes) { workInProgressRootExitStatus = RootIncomplete; workInProgressRootFatalError = null; workInProgressRootSkippedLanes = NoLanes; - workInProgressRootUpdatedLanes = NoLanes; + workInProgressRootInterleavedUpdatedLanes = NoLanes; + workInProgressRootRenderPhaseUpdatedLanes = NoLanes; workInProgressRootPingedLanes = NoLanes; enqueueInterleavedUpdates(); @@ -1456,7 +1486,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 6ad0298b8849b..843eb854b8429 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); } @@ -1313,7 +1342,8 @@ function prepareFreshStack(root: FiberRoot, lanes: Lanes) { workInProgressRootExitStatus = RootIncomplete; workInProgressRootFatalError = null; workInProgressRootSkippedLanes = NoLanes; - workInProgressRootUpdatedLanes = NoLanes; + workInProgressRootInterleavedUpdatedLanes = NoLanes; + workInProgressRootRenderPhaseUpdatedLanes = NoLanes; workInProgressRootPingedLanes = NoLanes; enqueueInterleavedUpdates(); @@ -1456,7 +1486,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 716771a3778dd..cc842bd16d22f 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();