diff --git a/packages/react-dom/src/__tests__/ReactUpdates-test.js b/packages/react-dom/src/__tests__/ReactUpdates-test.js index a49b62062d32e..42647abf53b4a 100644 --- a/packages/react-dom/src/__tests__/ReactUpdates-test.js +++ b/packages/react-dom/src/__tests__/ReactUpdates-test.js @@ -1594,6 +1594,7 @@ describe('ReactUpdates', () => { }); }); + // TODO: Replace this branch with @gate pragmas if (__DEV__) { it('warns about a deferred infinite update loop with useEffect', () => { function NonTerminating() { @@ -1684,4 +1685,35 @@ describe('ReactUpdates', () => { expect(container.textContent).toBe('1000'); }); } + + it('prevents infinite update loop triggered by synchronous updates in useEffect', () => { + // Ignore flushSync warning + spyOnDev(console, 'error'); + + function NonTerminating() { + const [step, setStep] = React.useState(0); + React.useEffect(() => { + // Other examples of synchronous updates in useEffect are imperative + // event dispatches like `el.focus`, or `useSyncExternalStore`, which + // may schedule a synchronous update upon subscribing if it detects + // that the store has been mutated since the initial render. + // + // (Originally I wrote this test using `el.focus` but those errors + // get dispatched in a JSDOM event and I don't know how to "catch" those + // so that they don't fail the test.) + ReactDOM.flushSync(() => { + setStep(step + 1); + }); + }, [step]); + return step; + } + + const container = document.createElement('div'); + const root = ReactDOM.createRoot(container); + expect(() => { + ReactDOM.flushSync(() => { + root.render(); + }); + }).toThrow('Maximum update depth exceeded'); + }); }); diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js index 01b5f4e8190a3..dc79e39e9d6a1 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js @@ -1897,6 +1897,15 @@ function commitRootImpl(root, renderPriorityLevel) { remainingLanes = root.pendingLanes; // Check if there's remaining work on this root + // TODO: This is part of the `componentDidCatch` implementation. Its purpose + // is to detect whether something might have called setState inside + // `componentDidCatch`. The mechanism is known to be flawed because `setState` + // inside `componentDidCatch` is itself flawed — that's why we recommend + // `getDerivedStateFromError` instead. However, it could be improved by + // checking if remainingLanes includes Sync work, instead of whether there's + // any work remaining at all (which would also include stuff like Suspense + // retries or transitions). It's been like this for a while, though, so fixing + // it probably isn't that urgent. if (remainingLanes === NoLanes) { // If there's no remaining work, we can clear the set of already failed // error boundaries. @@ -1909,23 +1918,6 @@ function commitRootImpl(root, renderPriorityLevel) { } } - if (includesSomeLane(remainingLanes, (SyncLane: Lane))) { - if (enableProfilerTimer && enableProfilerNestedUpdatePhase) { - markNestedUpdateScheduled(); - } - - // Count the number of times the root synchronously re-renders without - // finishing. If there are too many, it indicates an infinite update loop. - if (root === rootWithNestedUpdates) { - nestedUpdateCount++; - } else { - nestedUpdateCount = 0; - rootWithNestedUpdates = root; - } - } else { - nestedUpdateCount = 0; - } - onCommitRootDevTools(finishedWork.stateNode, renderPriorityLevel); if (enableUpdaterTracking) { @@ -1964,6 +1956,25 @@ function commitRootImpl(root, renderPriorityLevel) { flushPassiveEffects(); } + // Read this again, since a passive effect might have updated it + remainingLanes = root.pendingLanes; + if (includesSomeLane(remainingLanes, (SyncLane: Lane))) { + if (enableProfilerTimer && enableProfilerNestedUpdatePhase) { + markNestedUpdateScheduled(); + } + + // Count the number of times the root synchronously re-renders without + // finishing. If there are too many, it indicates an infinite update loop. + if (root === rootWithNestedUpdates) { + nestedUpdateCount++; + } else { + nestedUpdateCount = 0; + rootWithNestedUpdates = root; + } + } else { + nestedUpdateCount = 0; + } + // If layout work was scheduled, flush it now. flushSyncCallbacks(); diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js index e5ee9f4bd7b4f..e0cb491e8214d 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js @@ -1897,6 +1897,15 @@ function commitRootImpl(root, renderPriorityLevel) { remainingLanes = root.pendingLanes; // Check if there's remaining work on this root + // TODO: This is part of the `componentDidCatch` implementation. Its purpose + // is to detect whether something might have called setState inside + // `componentDidCatch`. The mechanism is known to be flawed because `setState` + // inside `componentDidCatch` is itself flawed — that's why we recommend + // `getDerivedStateFromError` instead. However, it could be improved by + // checking if remainingLanes includes Sync work, instead of whether there's + // any work remaining at all (which would also include stuff like Suspense + // retries or transitions). It's been like this for a while, though, so fixing + // it probably isn't that urgent. if (remainingLanes === NoLanes) { // If there's no remaining work, we can clear the set of already failed // error boundaries. @@ -1909,23 +1918,6 @@ function commitRootImpl(root, renderPriorityLevel) { } } - if (includesSomeLane(remainingLanes, (SyncLane: Lane))) { - if (enableProfilerTimer && enableProfilerNestedUpdatePhase) { - markNestedUpdateScheduled(); - } - - // Count the number of times the root synchronously re-renders without - // finishing. If there are too many, it indicates an infinite update loop. - if (root === rootWithNestedUpdates) { - nestedUpdateCount++; - } else { - nestedUpdateCount = 0; - rootWithNestedUpdates = root; - } - } else { - nestedUpdateCount = 0; - } - onCommitRootDevTools(finishedWork.stateNode, renderPriorityLevel); if (enableUpdaterTracking) { @@ -1964,6 +1956,25 @@ function commitRootImpl(root, renderPriorityLevel) { flushPassiveEffects(); } + // Read this again, since a passive effect might have updated it + remainingLanes = root.pendingLanes; + if (includesSomeLane(remainingLanes, (SyncLane: Lane))) { + if (enableProfilerTimer && enableProfilerNestedUpdatePhase) { + markNestedUpdateScheduled(); + } + + // Count the number of times the root synchronously re-renders without + // finishing. If there are too many, it indicates an infinite update loop. + if (root === rootWithNestedUpdates) { + nestedUpdateCount++; + } else { + nestedUpdateCount = 0; + rootWithNestedUpdates = root; + } + } else { + nestedUpdateCount = 0; + } + // If layout work was scheduled, flush it now. flushSyncCallbacks();