Skip to content

Commit ac2cff4

Browse files
authored
Warn if commit phase error thrown in detached tree (#20286)
Until `skipUnmountedBoundaries` lands again, we need some way to detect when errors are thrown inside a deleted tree. I've added a warning to `captureCommitPhaseError` that fires when we reach the root of a subtree without finding either a boundary or a HostRoot. Even after `skipUnmountedBoundaries` lands, this warning could be a useful guard against internal bugs, like a bug in the `skipUnmountedBoundaries` implementation itself. In the meantime, do not add this warning to the allowlist; this is only for our internal use. For this reason, I've also only added it to the new fork, not the old one, to prevent this from accidentally leaking into the open source build.
1 parent 0f83a64 commit ac2cff4

File tree

2 files changed

+56
-16
lines changed

2 files changed

+56
-16
lines changed

packages/react-dom/src/events/__tests__/DOMPluginEventSystem-test.internal.js

Lines changed: 40 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1669,16 +1669,28 @@ describe('DOMPluginEventSystem', () => {
16691669

16701670
function Test() {
16711671
React.useEffect(() => {
1672-
setClick1(buttonRef.current, targetListener1);
1673-
setClick2(buttonRef.current, targetListener2);
1674-
setClick3(buttonRef.current, targetListener3);
1675-
setClick4(buttonRef.current, targetListener4);
1672+
const clearClick1 = setClick1(
1673+
buttonRef.current,
1674+
targetListener1,
1675+
);
1676+
const clearClick2 = setClick2(
1677+
buttonRef.current,
1678+
targetListener2,
1679+
);
1680+
const clearClick3 = setClick3(
1681+
buttonRef.current,
1682+
targetListener3,
1683+
);
1684+
const clearClick4 = setClick4(
1685+
buttonRef.current,
1686+
targetListener4,
1687+
);
16761688

16771689
return () => {
1678-
setClick1();
1679-
setClick2();
1680-
setClick3();
1681-
setClick4();
1690+
clearClick1();
1691+
clearClick2();
1692+
clearClick3();
1693+
clearClick4();
16821694
};
16831695
});
16841696

@@ -1703,16 +1715,28 @@ describe('DOMPluginEventSystem', () => {
17031715

17041716
function Test2() {
17051717
React.useEffect(() => {
1706-
setClick1(buttonRef.current, targetListener1);
1707-
setClick2(buttonRef.current, targetListener2);
1708-
setClick3(buttonRef.current, targetListener3);
1709-
setClick4(buttonRef.current, targetListener4);
1718+
const clearClick1 = setClick1(
1719+
buttonRef.current,
1720+
targetListener1,
1721+
);
1722+
const clearClick2 = setClick2(
1723+
buttonRef.current,
1724+
targetListener2,
1725+
);
1726+
const clearClick3 = setClick3(
1727+
buttonRef.current,
1728+
targetListener3,
1729+
);
1730+
const clearClick4 = setClick4(
1731+
buttonRef.current,
1732+
targetListener4,
1733+
);
17101734

17111735
return () => {
1712-
setClick1();
1713-
setClick2();
1714-
setClick3();
1715-
setClick4();
1736+
clearClick1();
1737+
clearClick2();
1738+
clearClick3();
1739+
clearClick4();
17161740
};
17171741
});
17181742

packages/react-reconciler/src/ReactFiberWorkLoop.new.js

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2844,6 +2844,22 @@ export function captureCommitPhaseError(sourceFiber: Fiber, error: mixed) {
28442844
}
28452845
fiber = fiber.return;
28462846
}
2847+
2848+
if (__DEV__) {
2849+
// TODO: Until we re-land skipUnmountedBoundaries (see #20147), this warning
2850+
// will fire for errors that are thrown by destroy functions inside deleted
2851+
// trees. What it should instead do is propagate the error to the parent of
2852+
// the deleted tree. In the meantime, do not add this warning to the
2853+
// allowlist; this is only for our internal use.
2854+
console.error(
2855+
'Internal React error: Attempted to capture a commit phase error ' +
2856+
'inside a detached tree. This indicates a bug in React. Likely ' +
2857+
'causes include deleting the same fiber more than once, committing an ' +
2858+
'already-finished tree, or an inconsistent return pointer.\n\n' +
2859+
'Error message:\n\n%s',
2860+
error,
2861+
);
2862+
}
28472863
}
28482864

28492865
export function pingSuspendedRoot(

0 commit comments

Comments
 (0)