diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.js b/packages/react-reconciler/src/ReactFiberWorkLoop.js index dd25a2cee256c..ae4db267b638a 100644 --- 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 index 6f933c5880c3e..cc3eee28d8ef6 100644 --- 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]', + ]); }); // 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([]); });