diff --git a/packages/react-dom/src/events/__tests__/DOMPluginEventSystem-test.internal.js b/packages/react-dom/src/events/__tests__/DOMPluginEventSystem-test.internal.js
index 794f48ce676f..8858997cb97d 100644
--- a/packages/react-dom/src/events/__tests__/DOMPluginEventSystem-test.internal.js
+++ b/packages/react-dom/src/events/__tests__/DOMPluginEventSystem-test.internal.js
@@ -1667,16 +1667,28 @@ describe('DOMPluginEventSystem', () => {
function Test() {
React.useEffect(() => {
- setClick1(buttonRef.current, targetListener1);
- setClick2(buttonRef.current, targetListener2);
- setClick3(buttonRef.current, targetListener3);
- setClick4(buttonRef.current, targetListener4);
+ const clearClick1 = setClick1(
+ buttonRef.current,
+ targetListener1,
+ );
+ const clearClick2 = setClick2(
+ buttonRef.current,
+ targetListener2,
+ );
+ const clearClick3 = setClick3(
+ buttonRef.current,
+ targetListener3,
+ );
+ const clearClick4 = setClick4(
+ buttonRef.current,
+ targetListener4,
+ );
return () => {
- setClick1();
- setClick2();
- setClick3();
- setClick4();
+ clearClick1();
+ clearClick2();
+ clearClick3();
+ clearClick4();
};
});
@@ -1701,16 +1713,28 @@ describe('DOMPluginEventSystem', () => {
function Test2() {
React.useEffect(() => {
- setClick1(buttonRef.current, targetListener1);
- setClick2(buttonRef.current, targetListener2);
- setClick3(buttonRef.current, targetListener3);
- setClick4(buttonRef.current, targetListener4);
+ const clearClick1 = setClick1(
+ buttonRef.current,
+ targetListener1,
+ );
+ const clearClick2 = setClick2(
+ buttonRef.current,
+ targetListener2,
+ );
+ const clearClick3 = setClick3(
+ buttonRef.current,
+ targetListener3,
+ );
+ const clearClick4 = setClick4(
+ buttonRef.current,
+ targetListener4,
+ );
return () => {
- setClick1();
- setClick2();
- setClick3();
- setClick4();
+ clearClick1();
+ clearClick2();
+ clearClick3();
+ clearClick4();
};
});
diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.new.js b/packages/react-reconciler/src/ReactFiberCommitWork.new.js
index 79002ff6ae62..f086e47cf93b 100644
--- a/packages/react-reconciler/src/ReactFiberCommitWork.new.js
+++ b/packages/react-reconciler/src/ReactFiberCommitWork.new.js
@@ -173,13 +173,13 @@ function safelyCallComponentWillUnmount(current, instance) {
);
if (hasCaughtError()) {
const unmountError = clearCaughtError();
- captureCommitPhaseError(current, unmountError);
+ captureCommitPhaseError(current, current.return, unmountError);
}
} else {
try {
callComponentWillUnmountWithTimer(current, instance);
} catch (unmountError) {
- captureCommitPhaseError(current, unmountError);
+ captureCommitPhaseError(current, current.return, unmountError);
}
}
}
@@ -192,13 +192,13 @@ function safelyDetachRef(current: Fiber) {
invokeGuardedCallback(null, ref, null, null);
if (hasCaughtError()) {
const refError = clearCaughtError();
- captureCommitPhaseError(current, refError);
+ captureCommitPhaseError(current, current.return, refError);
}
} else {
try {
ref(null);
} catch (refError) {
- captureCommitPhaseError(current, refError);
+ captureCommitPhaseError(current, current.return, refError);
}
}
} else {
@@ -207,18 +207,22 @@ function safelyDetachRef(current: Fiber) {
}
}
-export function safelyCallDestroy(current: Fiber, destroy: () => void) {
+export function safelyCallDestroy(
+ current: Fiber,
+ nearestMountedAncestor: Fiber | null,
+ destroy: () => void,
+) {
if (__DEV__) {
invokeGuardedCallback(null, destroy, null);
if (hasCaughtError()) {
const error = clearCaughtError();
- captureCommitPhaseError(current, error);
+ captureCommitPhaseError(current, nearestMountedAncestor, error);
}
} else {
try {
destroy();
} catch (error) {
- captureCommitPhaseError(current, error);
+ captureCommitPhaseError(current, nearestMountedAncestor, error);
}
}
}
@@ -337,10 +341,10 @@ function commitHookEffectListUnmount(tag: HookEffectTag, finishedWork: Fiber) {
// TODO: Remove this duplication.
function commitHookEffectListUnmount2(
- // Tags to check for when deciding whether to unmount. e.g. to skip over
- // layout effects
+ // Tags to check for when deciding whether to unmount. e.g. to skip over layout effects
hookEffectTag: HookEffectTag,
fiber: Fiber,
+ nearestMountedAncestor: Fiber | null,
): void {
const updateQueue: FunctionComponentUpdateQueue | null = (fiber.updateQueue: any);
const lastEffect = updateQueue !== null ? updateQueue.lastEffect : null;
@@ -359,10 +363,10 @@ function commitHookEffectListUnmount2(
fiber.mode & ProfileMode
) {
startPassiveEffectTimer();
- safelyCallDestroy(fiber, destroy);
+ safelyCallDestroy(fiber, nearestMountedAncestor, destroy);
recordPassiveEffectDuration(fiber);
} else {
- safelyCallDestroy(fiber, destroy);
+ safelyCallDestroy(fiber, nearestMountedAncestor, destroy);
}
}
}
@@ -465,7 +469,7 @@ function commitHookEffectListMount2(fiber: Fiber): void {
if (hasCaughtError()) {
invariant(fiber !== null, 'Should be working on an effect.');
const error = clearCaughtError();
- captureCommitPhaseError(fiber, error);
+ captureCommitPhaseError(fiber, fiber.return, error);
}
} else {
try {
@@ -488,7 +492,7 @@ function commitHookEffectListMount2(fiber: Fiber): void {
// The warning refers to useEffect but only applies to useLayoutEffect.
} catch (error) {
invariant(fiber !== null, 'Should be working on an effect.');
- captureCommitPhaseError(fiber, error);
+ captureCommitPhaseError(fiber, fiber.return, error);
}
}
}
@@ -997,10 +1001,10 @@ function commitUnmount(
current.mode & ProfileMode
) {
startLayoutEffectTimer();
- safelyCallDestroy(current, destroy);
+ safelyCallDestroy(current, current.return, destroy);
recordLayoutEffectDuration(current);
} else {
- safelyCallDestroy(current, destroy);
+ safelyCallDestroy(current, current.return, destroy);
}
}
}
@@ -1842,18 +1846,29 @@ function commitPassiveWork(finishedWork: Fiber): void {
case ForwardRef:
case SimpleMemoComponent:
case Block: {
- commitHookEffectListUnmount2(HookPassive | HookHasEffect, finishedWork);
+ commitHookEffectListUnmount2(
+ HookPassive | HookHasEffect,
+ finishedWork,
+ finishedWork.return,
+ );
}
}
}
-function commitPassiveUnmount(current: Fiber): void {
+function commitPassiveUnmount(
+ current: Fiber,
+ nearestMountedAncestor: Fiber | null,
+): void {
switch (current.tag) {
case FunctionComponent:
case ForwardRef:
case SimpleMemoComponent:
case Block:
- commitHookEffectListUnmount2(HookPassive, current);
+ commitHookEffectListUnmount2(
+ HookPassive,
+ current,
+ nearestMountedAncestor,
+ );
}
}
diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js
index 63f20c123a39..87da91d58c5f 100644
--- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js
+++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js
@@ -2393,14 +2393,14 @@ function commitBeforeMutationEffects(firstChild: Fiber) {
invokeGuardedCallback(null, commitBeforeMutationEffectsImpl, null, fiber);
if (hasCaughtError()) {
const error = clearCaughtError();
- captureCommitPhaseError(fiber, error);
+ captureCommitPhaseError(fiber, fiber.return, error);
}
resetCurrentDebugFiberInDEV();
} else {
try {
commitBeforeMutationEffectsImpl(fiber);
} catch (error) {
- captureCommitPhaseError(fiber, error);
+ captureCommitPhaseError(fiber, fiber.return, error);
}
}
fiber = fiber.sibling;
@@ -2490,14 +2490,14 @@ function commitMutationEffects(
);
if (hasCaughtError()) {
const error = clearCaughtError();
- captureCommitPhaseError(fiber, error);
+ captureCommitPhaseError(fiber, fiber.return, error);
}
resetCurrentDebugFiberInDEV();
} else {
try {
commitMutationEffectsImpl(fiber, root, renderPriorityLevel);
} catch (error) {
- captureCommitPhaseError(fiber, error);
+ captureCommitPhaseError(fiber, fiber.return, error);
}
}
fiber = fiber.sibling;
@@ -2593,13 +2593,13 @@ function commitMutationEffectsDeletions(
);
if (hasCaughtError()) {
const error = clearCaughtError();
- captureCommitPhaseError(childToDelete, error);
+ captureCommitPhaseError(childToDelete, childToDelete.return, error);
}
} else {
try {
commitDeletion(root, childToDelete, renderPriorityLevel);
} catch (error) {
- captureCommitPhaseError(childToDelete, error);
+ captureCommitPhaseError(childToDelete, childToDelete.return, error);
}
}
}
@@ -2641,14 +2641,14 @@ function commitLayoutEffects(
);
if (hasCaughtError()) {
const error = clearCaughtError();
- captureCommitPhaseError(fiber, error);
+ captureCommitPhaseError(fiber, fiber.return, error);
}
resetCurrentDebugFiberInDEV();
} else {
try {
commitLayoutEffectsImpl(fiber, root, committedLanes);
} catch (error) {
- captureCommitPhaseError(fiber, error);
+ captureCommitPhaseError(fiber, fiber.return, error);
}
}
fiber = fiber.sibling;
@@ -2748,7 +2748,7 @@ function flushPassiveUnmountEffects(firstChild: Fiber): void {
if (deletions !== null) {
for (let i = 0; i < deletions.length; i++) {
const fiberToDelete = deletions[i];
- flushPassiveUnmountEffectsInsideOfDeletedTree(fiberToDelete);
+ flushPassiveUnmountEffectsInsideOfDeletedTree(fiberToDelete, fiber);
// Now that passive effects have been processed, it's safe to detach lingering pointers.
detachFiberAfterEffects(fiberToDelete);
@@ -2780,6 +2780,7 @@ function flushPassiveUnmountEffects(firstChild: Fiber): void {
function flushPassiveUnmountEffectsInsideOfDeletedTree(
fiberToDelete: Fiber,
+ nearestMountedAncestor: Fiber,
): void {
if ((fiberToDelete.subtreeTag & PassiveStaticSubtreeTag) !== NoSubtreeTag) {
// If any children have passive effects then traverse the subtree.
@@ -2788,14 +2789,17 @@ function flushPassiveUnmountEffectsInsideOfDeletedTree(
// since that would not cover passive effects in siblings.
let child = fiberToDelete.child;
while (child !== null) {
- flushPassiveUnmountEffectsInsideOfDeletedTree(child);
+ flushPassiveUnmountEffectsInsideOfDeletedTree(
+ child,
+ nearestMountedAncestor,
+ );
child = child.sibling;
}
}
if ((fiberToDelete.effectTag & PassiveStatic) !== NoEffect) {
setCurrentDebugFiberInDEV(fiberToDelete);
- commitPassiveUnmount(fiberToDelete);
+ commitPassiveUnmount(fiberToDelete, nearestMountedAncestor);
resetCurrentDebugFiberInDEV();
}
}
@@ -2922,7 +2926,11 @@ function captureCommitPhaseErrorOnRoot(
}
}
-export function captureCommitPhaseError(sourceFiber: Fiber, error: mixed) {
+export function captureCommitPhaseError(
+ sourceFiber: Fiber,
+ nearestMountedAncestor: Fiber | null,
+ error: mixed,
+) {
if (sourceFiber.tag === HostRoot) {
// Error was thrown at the root. There is no parent, so the root
// itself should capture it.
@@ -2930,7 +2938,7 @@ export function captureCommitPhaseError(sourceFiber: Fiber, error: mixed) {
return;
}
- let fiber = sourceFiber.return;
+ let fiber = nearestMountedAncestor;
while (fiber !== null) {
if (fiber.tag === HostRoot) {
captureCommitPhaseErrorOnRoot(fiber, sourceFiber, error);
diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js
index eab3949bde82..84188fea3742 100644
--- a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js
+++ b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js
@@ -2850,6 +2850,24 @@ export function captureCommitPhaseError(sourceFiber: Fiber, error: mixed) {
markRootUpdated(root, SyncLane, eventTime);
ensureRootIsScheduled(root, eventTime);
schedulePendingInteractions(root, SyncLane);
+ } else {
+ // This component has already been unmounted.
+ // We can't schedule any follow up work for the root because the fiber is already unmounted,
+ // but we can still call the log-only boundary so the error isn't swallowed.
+ //
+ // TODO This is only a temporary bandaid for the old reconciler fork.
+ // We can delete this special case once the new fork is merged.
+ if (
+ typeof instance.componentDidCatch === 'function' &&
+ !isAlreadyFailedLegacyErrorBoundary(instance)
+ ) {
+ try {
+ instance.componentDidCatch(error, errorInfo);
+ } catch (errorToIgnore) {
+ // TODO Ignore this error? Rethrow it?
+ // This is kind of an edge case.
+ }
+ }
}
return;
}
diff --git a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js
index eaa222456fc6..7ff61126f648 100644
--- a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js
+++ b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js
@@ -2320,6 +2320,7 @@ describe('ReactHooksWithNoopRenderer', () => {
describe('errors thrown in passive destroy function within unmounted trees', () => {
let BrokenUseEffectCleanup;
let ErrorBoundary;
+ let DerivedStateOnlyErrorBoundary;
let LogOnlyErrorBoundary;
beforeEach(() => {
@@ -2351,10 +2352,32 @@ describe('ReactHooksWithNoopRenderer', () => {
render() {
if (this.state.error) {
Scheduler.unstable_yieldValue('ErrorBoundary render error');
- return 'ErrorBoundary fallback';
+ return ;
}
Scheduler.unstable_yieldValue('ErrorBoundary render success');
- return this.props.children;
+ return this.props.children || null;
+ }
+ };
+
+ DerivedStateOnlyErrorBoundary = class extends React.Component {
+ state = {error: null};
+ static getDerivedStateFromError(error) {
+ Scheduler.unstable_yieldValue(
+ `DerivedStateOnlyErrorBoundary static getDerivedStateFromError`,
+ );
+ return {error};
+ }
+ render() {
+ if (this.state.error) {
+ Scheduler.unstable_yieldValue(
+ 'DerivedStateOnlyErrorBoundary render error',
+ );
+ return ;
+ }
+ Scheduler.unstable_yieldValue(
+ 'DerivedStateOnlyErrorBoundary render success',
+ );
+ return this.props.children || null;
}
};
@@ -2366,12 +2389,13 @@ describe('ReactHooksWithNoopRenderer', () => {
}
render() {
Scheduler.unstable_yieldValue(`LogOnlyErrorBoundary render`);
- return this.props.children;
+ return this.props.children || null;
}
};
});
- it('should not error if the nearest unmounted boundary is log-only', () => {
+ // @gate old
+ it('should call componentDidCatch() for the nearest unmounted log-only boundary', () => {
function Conditional({showChildren}) {
if (showChildren) {
return (
@@ -2411,10 +2435,268 @@ describe('ReactHooksWithNoopRenderer', () => {
expect(Scheduler).toHaveYielded([
'BrokenUseEffectCleanup useEffect destroy',
- // This should call componentDidCatch too, but we'll address that in a follow up.
- // 'LogOnlyErrorBoundary componentDidCatch',
+ 'LogOnlyErrorBoundary componentDidCatch',
]);
});
+
+ // @gate old
+ it('should call componentDidCatch() for the nearest unmounted logging-capable boundary', () => {
+ function Conditional({showChildren}) {
+ if (showChildren) {
+ return (
+
+
+
+ );
+ } else {
+ return null;
+ }
+ }
+
+ act(() => {
+ ReactNoop.render(
+
+
+ ,
+ );
+ });
+
+ expect(Scheduler).toHaveYielded([
+ 'ErrorBoundary render success',
+ 'ErrorBoundary render success',
+ 'BrokenUseEffectCleanup useEffect',
+ ]);
+
+ act(() => {
+ ReactNoop.render(
+
+
+ ,
+ );
+ expect(Scheduler).toFlushAndYieldThrough([
+ 'ErrorBoundary render success',
+ ]);
+ });
+
+ expect(Scheduler).toHaveYielded([
+ 'BrokenUseEffectCleanup useEffect destroy',
+ 'ErrorBoundary componentDidCatch',
+ ]);
+ });
+
+ // @gate old
+ it('should not call getDerivedStateFromError for unmounted error boundaries', () => {
+ function Conditional({showChildren}) {
+ if (showChildren) {
+ return (
+
+
+
+ );
+ } else {
+ return null;
+ }
+ }
+
+ act(() => {
+ ReactNoop.render();
+ });
+
+ expect(Scheduler).toHaveYielded([
+ 'ErrorBoundary render success',
+ 'BrokenUseEffectCleanup useEffect',
+ ]);
+
+ act(() => {
+ ReactNoop.render();
+ });
+
+ expect(Scheduler).toHaveYielded([
+ 'BrokenUseEffectCleanup useEffect destroy',
+ 'ErrorBoundary componentDidCatch',
+ ]);
+ });
+
+ // @gate old
+ it('should not throw if there are no unmounted logging-capable boundaries to call', () => {
+ function Conditional({showChildren}) {
+ if (showChildren) {
+ return (
+
+
+
+ );
+ } else {
+ return null;
+ }
+ }
+
+ act(() => {
+ ReactNoop.render();
+ });
+
+ expect(Scheduler).toHaveYielded([
+ 'DerivedStateOnlyErrorBoundary render success',
+ 'BrokenUseEffectCleanup useEffect',
+ ]);
+
+ act(() => {
+ ReactNoop.render();
+ });
+
+ expect(Scheduler).toHaveYielded([
+ 'BrokenUseEffectCleanup useEffect destroy',
+ ]);
+ });
+
+ // @gate new
+ it('should use the nearest still-mounted boundary if there are no unmounted boundaries', () => {
+ act(() => {
+ ReactNoop.render(
+
+
+ ,
+ );
+ });
+
+ expect(Scheduler).toHaveYielded([
+ 'LogOnlyErrorBoundary render',
+ 'BrokenUseEffectCleanup useEffect',
+ ]);
+
+ act(() => {
+ ReactNoop.render();
+ });
+
+ expect(Scheduler).toHaveYielded([
+ 'LogOnlyErrorBoundary render',
+ 'BrokenUseEffectCleanup useEffect destroy',
+ 'LogOnlyErrorBoundary componentDidCatch',
+ ]);
+ });
+
+ // @gate new
+ it('should skip unmounted boundaries and use the nearest still-mounted boundary', () => {
+ function Conditional({showChildren}) {
+ if (showChildren) {
+ return (
+
+
+
+ );
+ } else {
+ return null;
+ }
+ }
+
+ act(() => {
+ ReactNoop.render(
+
+
+ ,
+ );
+ });
+
+ expect(Scheduler).toHaveYielded([
+ 'LogOnlyErrorBoundary render',
+ 'ErrorBoundary render success',
+ 'BrokenUseEffectCleanup useEffect',
+ ]);
+
+ act(() => {
+ ReactNoop.render(
+
+
+ ,
+ );
+ });
+
+ expect(Scheduler).toHaveYielded([
+ 'LogOnlyErrorBoundary render',
+ 'BrokenUseEffectCleanup useEffect destroy',
+ 'LogOnlyErrorBoundary componentDidCatch',
+ ]);
+ });
+
+ // @gate new
+ it('should call getDerivedStateFromError in the nearest still-mounted boundary', () => {
+ function Conditional({showChildren}) {
+ if (showChildren) {
+ return ;
+ } else {
+ return null;
+ }
+ }
+
+ act(() => {
+ ReactNoop.render(
+
+
+ ,
+ );
+ });
+
+ expect(Scheduler).toHaveYielded([
+ 'ErrorBoundary render success',
+ 'BrokenUseEffectCleanup useEffect',
+ ]);
+
+ act(() => {
+ ReactNoop.render(
+
+
+ ,
+ );
+ });
+
+ expect(Scheduler).toHaveYielded([
+ 'ErrorBoundary render success',
+ 'BrokenUseEffectCleanup useEffect destroy',
+ 'ErrorBoundary static getDerivedStateFromError',
+ 'ErrorBoundary render error',
+ 'ErrorBoundary componentDidCatch',
+ ]);
+
+ expect(ReactNoop.getChildren()).toEqual([
+ span('ErrorBoundary fallback'),
+ ]);
+ });
+
+ // @gate new
+ it('should rethrow error if there are no still-mounted boundaries', () => {
+ function Conditional({showChildren}) {
+ if (showChildren) {
+ return (
+
+
+
+ );
+ } else {
+ return null;
+ }
+ }
+
+ act(() => {
+ ReactNoop.render();
+ });
+
+ expect(Scheduler).toHaveYielded([
+ 'ErrorBoundary render success',
+ 'BrokenUseEffectCleanup useEffect',
+ ]);
+
+ expect(() => {
+ act(() => {
+ ReactNoop.render();
+ });
+ }).toThrow('Expected error');
+
+ expect(Scheduler).toHaveYielded([
+ 'BrokenUseEffectCleanup useEffect destroy',
+ ]);
+
+ expect(ReactNoop.getChildren()).toEqual([]);
+ });
});
});