From b58e231cc9122e559c225125896ab4ae1769a79a Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Thu, 7 Apr 2022 18:54:44 +0100 Subject: [PATCH 1/2] Warn on setState() in useInsertionEffect() --- .../src/ReactFiberCommitWork.new.js | 23 +++++++++++- .../src/ReactFiberCommitWork.old.js | 23 +++++++++++- .../src/ReactFiberWorkLoop.new.js | 14 +++++++ .../src/ReactFiberWorkLoop.old.js | 14 +++++++ .../ReactHooksWithNoopRenderer-test.js | 37 +++++++++++++++++++ 5 files changed, 107 insertions(+), 4 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.new.js b/packages/react-reconciler/src/ReactFiberCommitWork.new.js index fb6040109518..aad83fa42c8c 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.new.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.new.js @@ -136,6 +136,7 @@ import { restorePendingUpdaters, addTransitionStartCallbackToPendingTransition, addTransitionCompleteCallbackToPendingTransition, + setIsRunningInsertionEffect, } from './ReactFiberWorkLoop.new'; import { NoFlags as NoHookEffect, @@ -536,7 +537,16 @@ function commitHookEffectListUnmount( } } - safelyCallDestroy(finishedWork, nearestMountedAncestor, destroy); + if (__DEV__ && (flags & HookInsertion) !== NoHookEffect) { + setIsRunningInsertionEffect(true); + try { + safelyCallDestroy(finishedWork, nearestMountedAncestor, destroy); + } finally { + setIsRunningInsertionEffect(false); + } + } else { + safelyCallDestroy(finishedWork, nearestMountedAncestor, destroy); + } if (enableSchedulingProfiler) { if ((flags & HookPassive) !== NoHookEffect) { @@ -570,7 +580,16 @@ function commitHookEffectListMount(flags: HookFlags, finishedWork: Fiber) { // Mount const create = effect.create; - effect.destroy = create(); + if (__DEV__ && (flags & HookInsertion) !== NoHookEffect) { + setIsRunningInsertionEffect(true); + try { + effect.destroy = create(); + } finally { + setIsRunningInsertionEffect(false); + } + } else { + effect.destroy = create(); + } if (enableSchedulingProfiler) { if ((flags & HookPassive) !== NoHookEffect) { diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.old.js b/packages/react-reconciler/src/ReactFiberCommitWork.old.js index e962039d9ca9..187a7776c8ad 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.old.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.old.js @@ -136,6 +136,7 @@ import { restorePendingUpdaters, addTransitionStartCallbackToPendingTransition, addTransitionCompleteCallbackToPendingTransition, + setIsRunningInsertionEffect, } from './ReactFiberWorkLoop.old'; import { NoFlags as NoHookEffect, @@ -536,7 +537,16 @@ function commitHookEffectListUnmount( } } - safelyCallDestroy(finishedWork, nearestMountedAncestor, destroy); + if (__DEV__ && (flags & HookInsertion) !== NoHookEffect) { + setIsRunningInsertionEffect(true); + try { + safelyCallDestroy(finishedWork, nearestMountedAncestor, destroy); + } finally { + setIsRunningInsertionEffect(false); + } + } else { + safelyCallDestroy(finishedWork, nearestMountedAncestor, destroy); + } if (enableSchedulingProfiler) { if ((flags & HookPassive) !== NoHookEffect) { @@ -570,7 +580,16 @@ function commitHookEffectListMount(flags: HookFlags, finishedWork: Fiber) { // Mount const create = effect.create; - effect.destroy = create(); + if (__DEV__ && (flags & HookInsertion) !== NoHookEffect) { + setIsRunningInsertionEffect(true); + try { + effect.destroy = create(); + } finally { + setIsRunningInsertionEffect(false); + } + } else { + effect.destroy = create(); + } if (enableSchedulingProfiler) { if ((flags & HookPassive) !== NoHookEffect) { diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js index 4da17c7bb6e8..c03c8a14cb00 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js @@ -406,6 +406,8 @@ let nestedPassiveUpdateCount: number = 0; let currentEventTime: number = NoTimestamp; let currentEventTransitionLane: Lanes = NoLanes; +let isRunningInsertionEffect = false; + export function getWorkInProgressRoot(): FiberRoot | null { return workInProgressRoot; } @@ -517,6 +519,12 @@ export function scheduleUpdateOnFiber( ): FiberRoot | null { checkForNestedUpdates(); + if (__DEV__) { + if (isRunningInsertionEffect) { + console.error('useInsertionEffect must not schedule updates.'); + } + } + const root = markUpdateLaneFromFiberToRoot(fiber, lane); if (root === null) { return null; @@ -3132,3 +3140,9 @@ function warnIfSuspenseResolutionNotWrappedWithActDEV(root: FiberRoot): void { } } } + +export function setIsRunningInsertionEffect(isRunning: boolean): void { + if (__DEV__) { + isRunningInsertionEffect = isRunning; + } +} diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js index 2c940a71001d..926818d38b32 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js @@ -406,6 +406,8 @@ let nestedPassiveUpdateCount: number = 0; let currentEventTime: number = NoTimestamp; let currentEventTransitionLane: Lanes = NoLanes; +let isRunningInsertionEffect = false; + export function getWorkInProgressRoot(): FiberRoot | null { return workInProgressRoot; } @@ -517,6 +519,12 @@ export function scheduleUpdateOnFiber( ): FiberRoot | null { checkForNestedUpdates(); + if (__DEV__) { + if (isRunningInsertionEffect) { + console.error('useInsertionEffect must not schedule updates.'); + } + } + const root = markUpdateLaneFromFiberToRoot(fiber, lane); if (root === null) { return null; @@ -3132,3 +3140,9 @@ function warnIfSuspenseResolutionNotWrappedWithActDEV(root: FiberRoot): void { } } } + +export function setIsRunningInsertionEffect(isRunning: boolean): void { + if (__DEV__) { + isRunningInsertionEffect = isRunning; + } +} diff --git a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js index 811be0963915..dae042d6c6ed 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js +++ b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js @@ -3090,6 +3090,43 @@ describe('ReactHooksWithNoopRenderer', () => { }), ).toThrow('is not a function'); }); + + it('warns when setState is called from insertion effect setup', () => { + function App(props) { + const [, setX] = useState(0); + useInsertionEffect(() => { + setX(1); + }, []); + return null; + } + + const root = ReactNoop.createRoot(); + expect(() => + act(() => { + root.render(); + }), + ).toErrorDev(['Warning: useInsertionEffect must not schedule updates.']); + }); + + it('warns when setState is called from insertion effect cleanup', () => { + function App(props) { + const [, setX] = useState(0); + useInsertionEffect(() => { + return () => setX(1); + }, [props.foo]); + return null; + } + + const root = ReactNoop.createRoot(); + act(() => { + root.render(); + }); + expect(() => { + act(() => { + root.render(); + }); + }).toErrorDev(['Warning: useInsertionEffect must not schedule updates.']); + }); }); describe('useLayoutEffect', () => { From 59266f5c2c83c9756841333604b088dc0f2c6523 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Thu, 7 Apr 2022 20:04:50 +0100 Subject: [PATCH 2/2] Use existing DEV reset mechanism --- .../src/ReactFiberCommitWork.new.js | 30 +++++------ .../src/ReactFiberCommitWork.old.js | 30 +++++------ .../src/ReactFiberWorkLoop.new.js | 3 ++ .../src/ReactFiberWorkLoop.old.js | 3 ++ .../ReactHooksWithNoopRenderer-test.js | 50 +++++++++++++++++-- 5 files changed, 85 insertions(+), 31 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.new.js b/packages/react-reconciler/src/ReactFiberCommitWork.new.js index aad83fa42c8c..d1bbdcdde156 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.new.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.new.js @@ -537,15 +537,16 @@ function commitHookEffectListUnmount( } } - if (__DEV__ && (flags & HookInsertion) !== NoHookEffect) { - setIsRunningInsertionEffect(true); - try { - safelyCallDestroy(finishedWork, nearestMountedAncestor, destroy); - } finally { + if (__DEV__) { + if ((flags & HookInsertion) !== NoHookEffect) { + setIsRunningInsertionEffect(true); + } + } + safelyCallDestroy(finishedWork, nearestMountedAncestor, destroy); + if (__DEV__) { + if ((flags & HookInsertion) !== NoHookEffect) { setIsRunningInsertionEffect(false); } - } else { - safelyCallDestroy(finishedWork, nearestMountedAncestor, destroy); } if (enableSchedulingProfiler) { @@ -580,15 +581,16 @@ function commitHookEffectListMount(flags: HookFlags, finishedWork: Fiber) { // Mount const create = effect.create; - if (__DEV__ && (flags & HookInsertion) !== NoHookEffect) { - setIsRunningInsertionEffect(true); - try { - effect.destroy = create(); - } finally { + if (__DEV__) { + if ((flags & HookInsertion) !== NoHookEffect) { + setIsRunningInsertionEffect(true); + } + } + effect.destroy = create(); + if (__DEV__) { + if ((flags & HookInsertion) !== NoHookEffect) { setIsRunningInsertionEffect(false); } - } else { - effect.destroy = create(); } if (enableSchedulingProfiler) { diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.old.js b/packages/react-reconciler/src/ReactFiberCommitWork.old.js index 187a7776c8ad..bff8b2781310 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.old.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.old.js @@ -537,15 +537,16 @@ function commitHookEffectListUnmount( } } - if (__DEV__ && (flags & HookInsertion) !== NoHookEffect) { - setIsRunningInsertionEffect(true); - try { - safelyCallDestroy(finishedWork, nearestMountedAncestor, destroy); - } finally { + if (__DEV__) { + if ((flags & HookInsertion) !== NoHookEffect) { + setIsRunningInsertionEffect(true); + } + } + safelyCallDestroy(finishedWork, nearestMountedAncestor, destroy); + if (__DEV__) { + if ((flags & HookInsertion) !== NoHookEffect) { setIsRunningInsertionEffect(false); } - } else { - safelyCallDestroy(finishedWork, nearestMountedAncestor, destroy); } if (enableSchedulingProfiler) { @@ -580,15 +581,16 @@ function commitHookEffectListMount(flags: HookFlags, finishedWork: Fiber) { // Mount const create = effect.create; - if (__DEV__ && (flags & HookInsertion) !== NoHookEffect) { - setIsRunningInsertionEffect(true); - try { - effect.destroy = create(); - } finally { + if (__DEV__) { + if ((flags & HookInsertion) !== NoHookEffect) { + setIsRunningInsertionEffect(true); + } + } + effect.destroy = create(); + if (__DEV__) { + if ((flags & HookInsertion) !== NoHookEffect) { setIsRunningInsertionEffect(false); } - } else { - effect.destroy = create(); } if (enableSchedulingProfiler) { diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js index c03c8a14cb00..163aec8e7689 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js @@ -2531,6 +2531,9 @@ export function captureCommitPhaseError( nearestMountedAncestor: Fiber | null, error: mixed, ) { + if (__DEV__) { + setIsRunningInsertionEffect(false); + } if (sourceFiber.tag === HostRoot) { // Error was thrown at the root. There is no parent, so the root // itself should capture it. diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js index 926818d38b32..fe16c38c230d 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js @@ -2531,6 +2531,9 @@ export function captureCommitPhaseError( nearestMountedAncestor: Fiber | null, error: mixed, ) { + if (__DEV__) { + setIsRunningInsertionEffect(false); + } if (sourceFiber.tag === HostRoot) { // Error was thrown at the root. There is no parent, so the root // itself should capture it. diff --git a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js index dae042d6c6ed..bacf9da7029a 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js +++ b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js @@ -3096,7 +3096,10 @@ describe('ReactHooksWithNoopRenderer', () => { const [, setX] = useState(0); useInsertionEffect(() => { setX(1); - }, []); + if (props.throw) { + throw Error('No'); + } + }, [props.throw]); return null; } @@ -3106,14 +3109,37 @@ describe('ReactHooksWithNoopRenderer', () => { root.render(); }), ).toErrorDev(['Warning: useInsertionEffect must not schedule updates.']); + + expect(() => { + act(() => { + root.render(); + }); + }).toThrow('No'); + + // Should not warn for regular effects after throw. + function NotInsertion() { + const [, setX] = useState(0); + useEffect(() => { + setX(1); + }, []); + return null; + } + act(() => { + root.render(); + }); }); it('warns when setState is called from insertion effect cleanup', () => { function App(props) { const [, setX] = useState(0); useInsertionEffect(() => { - return () => setX(1); - }, [props.foo]); + if (props.throw) { + throw Error('No'); + } + return () => { + setX(1); + }; + }, [props.throw, props.foo]); return null; } @@ -3126,6 +3152,24 @@ describe('ReactHooksWithNoopRenderer', () => { root.render(); }); }).toErrorDev(['Warning: useInsertionEffect must not schedule updates.']); + + expect(() => { + act(() => { + root.render(); + }); + }).toThrow('No'); + + // Should not warn for regular effects after throw. + function NotInsertion() { + const [, setX] = useState(0); + useEffect(() => { + setX(1); + }, []); + return null; + } + act(() => { + root.render(); + }); }); });