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.
Andrew Clark committed
Nov 18, 2020 at 11:23 UTC
ac2cff4b10624b87b14333d9b7c5d41a51fac0ec
2 files changed
+56
-16
packages/react-dom/src/events/__tests__/DOMPluginEventSystem-test.internal.js
+40
-16
@@ -1669,16 +1669,28 @@ describe('DOMPluginEventSystem', () => {
1669
1670
function Test() {
1671
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
+ );
1688
1689
return () => {
1678
- setClick1();
1679
- setClick2();
1680
- setClick3();
1681
- setClick4();
1690
+ clearClick1();
1691
+ clearClick2();
1692
+ clearClick3();
1693
+ clearClick4();
1694
};
1695
});
1696
@@ -1703,16 +1715,28 @@ describe('DOMPluginEventSystem', () => {
1715
1716
function Test2() {
1717
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
+ );
1734
1735
return () => {
1712
- setClick1();
1713
- setClick2();
1714
- setClick3();
1715
- setClick4();
1736
+ clearClick1();
1737
+ clearClick2();
1738
+ clearClick3();
1739
+ clearClick4();
1740
};
1741
});
1742
packages/react-reconciler/src/ReactFiberWorkLoop.new.js
+16
@@ -2844,6 +2844,22 @@ export function captureCommitPhaseError(sourceFiber: Fiber, error: mixed) {
2844
}
2845
fiber = fiber.return;
2846
}
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
+ }
2863
}
2864
2865
export function pingSuspendedRoot(