From 06dc802223113b5e9af7c674db503aa4ec9b5d59 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Thu, 7 Apr 2022 17:05:19 +0100 Subject: [PATCH 1/3] Fix warning about setState in useEffect --- .../src/ReactFiberWorkLoop.new.js | 29 ++++++++++++++++--- .../src/ReactFiberWorkLoop.old.js | 29 ++++++++++++++++--- .../useSyncExternalStoreShared-test.js | 5 ++-- 3 files changed, 53 insertions(+), 10 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js index 4da17c7bb6e80..78dd996ecfd3f 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js @@ -396,6 +396,8 @@ let pendingPassiveEffectsRemainingLanes: Lanes = NoLanes; const NESTED_UPDATE_LIMIT = 50; let nestedUpdateCount: number = 0; let rootWithNestedUpdates: FiberRoot | null = null; +let isFlushingPassiveEffects = false; +let didScheduleUpdateDuringPassiveEffects = false; const NESTED_PASSIVE_UPDATE_LIMIT = 50; let nestedPassiveUpdateCount: number = 0; @@ -522,6 +524,12 @@ export function scheduleUpdateOnFiber( return null; } + if (__DEV__) { + if (isFlushingPassiveEffects) { + didScheduleUpdateDuringPassiveEffects = true; + } + } + // Mark that the root has a pending update. markRootUpdated(root, lane, eventTime); @@ -2204,6 +2212,9 @@ function commitRootImpl( // There were no passive effects, so we can immediately release the cache // pool for this render. releaseRootPooledCache(root, remainingLanes); + if (__DEV__) { + nestedPassiveUpdateCount = 0; + } } // Read this again, since an effect might have updated it @@ -2420,6 +2431,9 @@ function flushPassiveEffectsImpl() { } if (__DEV__) { + isFlushingPassiveEffects = true; + didScheduleUpdateDuringPassiveEffects = false; + if (enableDebugTracing) { logPassiveEffectsStarted(lanes); } @@ -2463,10 +2477,17 @@ function flushPassiveEffectsImpl() { flushSyncCallbacks(); - // If additional passive effects were scheduled, increment a counter. If this - // exceeds the limit, we'll fire a warning. - nestedPassiveUpdateCount = - rootWithPendingPassiveEffects === null ? 0 : nestedPassiveUpdateCount + 1; + if (__DEV__) { + // If additional passive effects were scheduled, increment a counter. If this + // exceeds the limit, we'll fire a warning. + if (didScheduleUpdateDuringPassiveEffects) { + nestedPassiveUpdateCount++; + } else { + nestedPassiveUpdateCount = 0; + } + isFlushingPassiveEffects = false; + didScheduleUpdateDuringPassiveEffects = false; + } // TODO: Move to commitPassiveMountEffects onPostCommitRootDevTools(root); diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js index 2c940a71001db..fd9ab04904532 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js @@ -396,6 +396,8 @@ let pendingPassiveEffectsRemainingLanes: Lanes = NoLanes; const NESTED_UPDATE_LIMIT = 50; let nestedUpdateCount: number = 0; let rootWithNestedUpdates: FiberRoot | null = null; +let isFlushingPassiveEffects = false; +let didScheduleUpdateDuringPassiveEffects = false; const NESTED_PASSIVE_UPDATE_LIMIT = 50; let nestedPassiveUpdateCount: number = 0; @@ -522,6 +524,12 @@ export function scheduleUpdateOnFiber( return null; } + if (__DEV__) { + if (isFlushingPassiveEffects) { + didScheduleUpdateDuringPassiveEffects = true; + } + } + // Mark that the root has a pending update. markRootUpdated(root, lane, eventTime); @@ -2204,6 +2212,9 @@ function commitRootImpl( // There were no passive effects, so we can immediately release the cache // pool for this render. releaseRootPooledCache(root, remainingLanes); + if (__DEV__) { + nestedPassiveUpdateCount = 0; + } } // Read this again, since an effect might have updated it @@ -2420,6 +2431,9 @@ function flushPassiveEffectsImpl() { } if (__DEV__) { + isFlushingPassiveEffects = true; + didScheduleUpdateDuringPassiveEffects = false; + if (enableDebugTracing) { logPassiveEffectsStarted(lanes); } @@ -2463,10 +2477,17 @@ function flushPassiveEffectsImpl() { flushSyncCallbacks(); - // If additional passive effects were scheduled, increment a counter. If this - // exceeds the limit, we'll fire a warning. - nestedPassiveUpdateCount = - rootWithPendingPassiveEffects === null ? 0 : nestedPassiveUpdateCount + 1; + if (__DEV__) { + // If additional passive effects were scheduled, increment a counter. If this + // exceeds the limit, we'll fire a warning. + if (didScheduleUpdateDuringPassiveEffects) { + nestedPassiveUpdateCount++; + } else { + nestedPassiveUpdateCount = 0; + } + isFlushingPassiveEffects = false; + didScheduleUpdateDuringPassiveEffects = false; + } // TODO: Move to commitPassiveMountEffects onPostCommitRootDevTools(root); diff --git a/packages/use-sync-external-store/src/__tests__/useSyncExternalStoreShared-test.js b/packages/use-sync-external-store/src/__tests__/useSyncExternalStoreShared-test.js index 5b0ce4f1a4afe..b1f70514eba3f 100644 --- a/packages/use-sync-external-store/src/__tests__/useSyncExternalStoreShared-test.js +++ b/packages/use-sync-external-store/src/__tests__/useSyncExternalStoreShared-test.js @@ -584,9 +584,10 @@ describe('Shared useSyncExternalStore behavior (shim and built-in)', () => { 'calls setState inside componentWillUpdate or componentDidUpdate. React limits ' + 'the number of nested updates to prevent infinite loops.', ); - }).toErrorDev( + }).toErrorDev([ + 'Maximum update depth exceeded.', 'The result of getSnapshot should be cached to avoid an infinite loop', - ); + ]); }); test('getSnapshot can return NaN without infinite loop warning', async () => { From 44bd8624c56e447f1999a54d6b08f782fb4d7bd5 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Thu, 7 Apr 2022 17:23:17 +0100 Subject: [PATCH 2/3] Fix test --- .../__tests__/useSyncExternalStoreShared-test.js | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/packages/use-sync-external-store/src/__tests__/useSyncExternalStoreShared-test.js b/packages/use-sync-external-store/src/__tests__/useSyncExternalStoreShared-test.js index b1f70514eba3f..97dc89ac4a805 100644 --- a/packages/use-sync-external-store/src/__tests__/useSyncExternalStoreShared-test.js +++ b/packages/use-sync-external-store/src/__tests__/useSyncExternalStoreShared-test.js @@ -584,10 +584,16 @@ describe('Shared useSyncExternalStore behavior (shim and built-in)', () => { 'calls setState inside componentWillUpdate or componentDidUpdate. React limits ' + 'the number of nested updates to prevent infinite loops.', ); - }).toErrorDev([ - 'Maximum update depth exceeded.', - 'The result of getSnapshot should be cached to avoid an infinite loop', - ]); + }).toErrorDev( + gate(flags => flags.enableUseSyncExternalStoreShim) + ? [ + 'The result of getSnapshot should be cached to avoid an infinite loop', + ] + : [ + 'Maximum update depth exceeded.', + 'The result of getSnapshot should be cached to avoid an infinite loop', + ], + ); }); test('getSnapshot can return NaN without infinite loop warning', async () => { From a300efc8f4832e385bd38b4a823d87f72c30983f Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Thu, 7 Apr 2022 17:51:51 +0100 Subject: [PATCH 3/3] Fix multiple roots --- .../react-reconciler/src/ReactFiberWorkLoop.new.js | 11 ++++++++++- .../react-reconciler/src/ReactFiberWorkLoop.old.js | 11 ++++++++++- .../src/__tests__/useSyncExternalStoreShared-test.js | 9 +-------- 3 files changed, 21 insertions(+), 10 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js index 78dd996ecfd3f..3f4071164df57 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js @@ -401,6 +401,7 @@ let didScheduleUpdateDuringPassiveEffects = false; const NESTED_PASSIVE_UPDATE_LIMIT = 50; let nestedPassiveUpdateCount: number = 0; +let rootWithPassiveNestedUpdates: FiberRoot | null = null; // If two updates are scheduled within the same event, we should treat their // event times as simultaneous, even if the actual clock time has advanced @@ -2214,6 +2215,7 @@ function commitRootImpl( releaseRootPooledCache(root, remainingLanes); if (__DEV__) { nestedPassiveUpdateCount = 0; + rootWithPassiveNestedUpdates = null; } } @@ -2481,7 +2483,12 @@ function flushPassiveEffectsImpl() { // If additional passive effects were scheduled, increment a counter. If this // exceeds the limit, we'll fire a warning. if (didScheduleUpdateDuringPassiveEffects) { - nestedPassiveUpdateCount++; + if (root === rootWithPassiveNestedUpdates) { + nestedPassiveUpdateCount++; + } else { + nestedPassiveUpdateCount = 0; + rootWithPassiveNestedUpdates = root; + } } else { nestedPassiveUpdateCount = 0; } @@ -2760,6 +2767,8 @@ function checkForNestedUpdates() { if (__DEV__) { if (nestedPassiveUpdateCount > NESTED_PASSIVE_UPDATE_LIMIT) { nestedPassiveUpdateCount = 0; + rootWithPassiveNestedUpdates = null; + console.error( 'Maximum update depth exceeded. This can happen when a component ' + "calls setState inside useEffect, but useEffect either doesn't " + diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js index fd9ab04904532..604a585406ae6 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js @@ -401,6 +401,7 @@ let didScheduleUpdateDuringPassiveEffects = false; const NESTED_PASSIVE_UPDATE_LIMIT = 50; let nestedPassiveUpdateCount: number = 0; +let rootWithPassiveNestedUpdates: FiberRoot | null = null; // If two updates are scheduled within the same event, we should treat their // event times as simultaneous, even if the actual clock time has advanced @@ -2214,6 +2215,7 @@ function commitRootImpl( releaseRootPooledCache(root, remainingLanes); if (__DEV__) { nestedPassiveUpdateCount = 0; + rootWithPassiveNestedUpdates = null; } } @@ -2481,7 +2483,12 @@ function flushPassiveEffectsImpl() { // If additional passive effects were scheduled, increment a counter. If this // exceeds the limit, we'll fire a warning. if (didScheduleUpdateDuringPassiveEffects) { - nestedPassiveUpdateCount++; + if (root === rootWithPassiveNestedUpdates) { + nestedPassiveUpdateCount++; + } else { + nestedPassiveUpdateCount = 0; + rootWithPassiveNestedUpdates = root; + } } else { nestedPassiveUpdateCount = 0; } @@ -2760,6 +2767,8 @@ function checkForNestedUpdates() { if (__DEV__) { if (nestedPassiveUpdateCount > NESTED_PASSIVE_UPDATE_LIMIT) { nestedPassiveUpdateCount = 0; + rootWithPassiveNestedUpdates = null; + console.error( 'Maximum update depth exceeded. This can happen when a component ' + "calls setState inside useEffect, but useEffect either doesn't " + diff --git a/packages/use-sync-external-store/src/__tests__/useSyncExternalStoreShared-test.js b/packages/use-sync-external-store/src/__tests__/useSyncExternalStoreShared-test.js index 97dc89ac4a805..5b0ce4f1a4afe 100644 --- a/packages/use-sync-external-store/src/__tests__/useSyncExternalStoreShared-test.js +++ b/packages/use-sync-external-store/src/__tests__/useSyncExternalStoreShared-test.js @@ -585,14 +585,7 @@ describe('Shared useSyncExternalStore behavior (shim and built-in)', () => { 'the number of nested updates to prevent infinite loops.', ); }).toErrorDev( - gate(flags => flags.enableUseSyncExternalStoreShim) - ? [ - 'The result of getSnapshot should be cached to avoid an infinite loop', - ] - : [ - 'Maximum update depth exceeded.', - 'The result of getSnapshot should be cached to avoid an infinite loop', - ], + 'The result of getSnapshot should be cached to avoid an infinite loop', ); });