From 59266f5c2c83c9756841333604b088dc0f2c6523 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Thu, 7 Apr 2022 20:04:50 +0100 Subject: [PATCH] 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 aad83fa42c8c2..d1bbdcdde156d 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 187a7776c8ada..bff8b2781310b 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 c03c8a14cb004..163aec8e7689f 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 926818d38b320..fe16c38c230d1 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 dae042d6c6ed3..bacf9da7029a7 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(); + }); }); });