Use existing DEV reset mechanism

facebook · Apr 7, 2022 · 59266f5 · 59266f5
1 parent b58e231
commit 59266f5
Show file tree

Hide file tree

Showing 5 changed files with 85 additions and 31 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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(<App />);
         }),
       ).toErrorDev(['Warning: useInsertionEffect must not schedule updates.']);
+
+      expect(() => {
+        act(() => {
+          root.render(<App throw={true} />);
+        });
+      }).toThrow('No');
+
+      // Should not warn for regular effects after throw.
+      function NotInsertion() {
+        const [, setX] = useState(0);
+        useEffect(() => {
+          setX(1);
+        }, []);
+        return null;
+      }
+      act(() => {
+        root.render(<NotInsertion />);
+      });
     });
 
     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(<App foo="goodbye" />);
         });
       }).toErrorDev(['Warning: useInsertionEffect must not schedule updates.']);
+
+      expect(() => {
+        act(() => {
+          root.render(<App throw={true} />);
+        });
+      }).toThrow('No');
+
+      // Should not warn for regular effects after throw.
+      function NotInsertion() {
+        const [, setX] = useState(0);
+        useEffect(() => {
+          setX(1);
+        }, []);
+        return null;
+      }
+      act(() => {
+        root.render(<NotInsertion />);
+      });
     });
   });