Removed passive unmount error tracking check

facebook · Feb 3, 2020 · 44e2b96 · 44e2b96
1 parent 0fb7bba
commit 44e2b96
Show file tree

Hide file tree

Showing 2 changed files with 33 additions and 31 deletions.
diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.js b/packages/react-reconciler/src/ReactFiberWorkLoop.js
@@ -2238,7 +2238,6 @@ function flushPassiveEffectsImpl() {
     // because the root is not part of its own effect list.
     // This could change in the future.
     let effect = root.current.firstEffect;
-    let effectWithErrorDuringUnmount = null;
     while (effect !== null) {
       if (__DEV__) {
         setCurrentDebugFiberInDEV(effect);
@@ -2249,7 +2248,6 @@ function flushPassiveEffectsImpl() {
           effect,
         );
         if (hasCaughtError()) {
-          effectWithErrorDuringUnmount = effect;
           invariant(effect !== null, 'Should be working on an effect.');
           const error = clearCaughtError();
           captureCommitPhaseError(effect, error);
@@ -2259,7 +2257,6 @@ function flushPassiveEffectsImpl() {
         try {
           commitPassiveHookUnmountEffects(effect);
         } catch (error) {
-          effectWithErrorDuringUnmount = effect;
           invariant(effect !== null, 'Should be working on an effect.');
           captureCommitPhaseError(effect, error);
         }
@@ -2274,31 +2271,26 @@ function flushPassiveEffectsImpl() {
     // This could change in the future.
     effect = root.current.firstEffect;
     while (effect !== null) {
-      // Don't run create effects for a Fiber that errored during destroy.
-      // This check is in place to match previous behavior.
-      // TODO: Rethink whether we want to carry this behavior forward.
-      if (effectWithErrorDuringUnmount !== effect) {
-        if (__DEV__) {
-          setCurrentDebugFiberInDEV(effect);
-          invokeGuardedCallback(
-            null,
-            commitPassiveHookMountEffects,
-            null,
-            effect,
-          );
-          if (hasCaughtError()) {
-            invariant(effect !== null, 'Should be working on an effect.');
-            const error = clearCaughtError();
-            captureCommitPhaseError(effect, error);
-          }
-          resetCurrentDebugFiberInDEV();
-        } else {
-          try {
-            commitPassiveHookMountEffects(effect);
-          } catch (error) {
-            invariant(effect !== null, 'Should be working on an effect.');
-            captureCommitPhaseError(effect, error);
-          }
+      if (__DEV__) {
+        setCurrentDebugFiberInDEV(effect);
+        invokeGuardedCallback(
+          null,
+          commitPassiveHookMountEffects,
+          null,
+          effect,
+        );
+        if (hasCaughtError()) {
+          invariant(effect !== null, 'Should be working on an effect.');
+          const error = clearCaughtError();
+          captureCommitPhaseError(effect, error);
+        }
+        resetCurrentDebugFiberInDEV();
+      } else {
+        try {
+          commitPassiveHookMountEffects(effect);
+        } catch (error) {
+          invariant(effect !== null, 'Should be working on an effect.');
+          captureCommitPhaseError(effect, error);
         }
       }
       const nextNextEffect = effect.nextEffect;

diff --git a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js
@@ -1746,12 +1746,22 @@ describe('ReactHooksWithNoopRenderer', () => {
         expect(Scheduler).toFlushAndYieldThrough(['Count: 1', 'Sync effect']);
         expect(ReactNoop.getChildren()).toEqual([span('Count: 1')]);
         expect(() => ReactNoop.flushPassiveEffects()).toThrow('Oops');
-        expect(Scheduler).toHaveYielded(['Oops!']);
+
+        // This tests enables a feature flag that flushes all passive destroys in a
+        // separate pass before flushing any passive creates.
+        // A result of this two-pass flush is that an error thrown from unmount does
+        // not block the subsequent create functions from being run.
+        expect(Scheduler).toHaveYielded([
+          'Oops!',
+          'Mount A [1]',
+          'Mount B [1]',
+        ]);
       });
 
       // <Counter> gets unmounted because an error is thrown above.
-      // The remaining destroy function gets runs later on unmount, since it's passive
-      expect(Scheduler).toHaveYielded(['Unmount B [0]']);
+      // The remaining destroy functions are run later on unmount, since they're passive.
+      // In this case, one of them throws again (because of how the test is written).
+      expect(Scheduler).toHaveYielded(['Oops!', 'Unmount B [1]']);
       expect(ReactNoop.getChildren()).toEqual([]);
     });