From 4a591cf335ffc82d6213fa7e3623e7c2d83da1c1 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Mon, 28 Feb 2022 11:14:30 -0500 Subject: [PATCH] Add back skipUnmountedBoundaries flag only for www (#23383) There are a few internal tests that still need to be updated, so I'm adding this flag back for www only. The desired behavior rolled out to 10% public, so we're confident there are no issues. The open source behavior remains (skipUnmountedBoundaries = true). --- .../ReactErrorBoundaries-test.internal.js | 1 + .../src/ReactFiberWorkLoop.new.js | 16 ++++++++++++++-- .../src/ReactFiberWorkLoop.old.js | 16 ++++++++++++++-- .../__tests__/ReactHooksWithNoopRenderer-test.js | 5 +++++ ...eactIncrementalErrorHandling-test.internal.js | 1 + packages/shared/ReactFeatureFlags.js | 4 ++++ .../shared/forks/ReactFeatureFlags.native-fb.js | 1 + .../shared/forks/ReactFeatureFlags.native-oss.js | 1 + .../forks/ReactFeatureFlags.test-renderer.js | 1 + .../ReactFeatureFlags.test-renderer.native.js | 1 + .../forks/ReactFeatureFlags.test-renderer.www.js | 1 + .../shared/forks/ReactFeatureFlags.testing.js | 1 + .../forks/ReactFeatureFlags.testing.www.js | 1 + .../forks/ReactFeatureFlags.www-dynamic.js | 1 + packages/shared/forks/ReactFeatureFlags.www.js | 1 + 15 files changed, 48 insertions(+), 4 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactErrorBoundaries-test.internal.js b/packages/react-dom/src/__tests__/ReactErrorBoundaries-test.internal.js index 8b1936cc8e11b..c5a1403ddc52a 100644 --- a/packages/react-dom/src/__tests__/ReactErrorBoundaries-test.internal.js +++ b/packages/react-dom/src/__tests__/ReactErrorBoundaries-test.internal.js @@ -42,6 +42,7 @@ describe('ReactErrorBoundaries', () => { PropTypes = require('prop-types'); ReactFeatureFlags = require('shared/ReactFeatureFlags'); ReactFeatureFlags.replayFailedUnitOfWorkWithInvokeGuardedCallback = false; + ReactFeatureFlags.skipUnmountedBoundaries = true; ReactDOM = require('react-dom'); React = require('react'); act = require('jest-react').act; diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js index a529454dcd37c..7223ad7d052b0 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js @@ -35,6 +35,7 @@ import { enableSchedulingProfiler, disableSchedulerTimeoutInWorkLoop, enableStrictEffects, + skipUnmountedBoundaries, enableUpdaterTracking, enableCache, enableTransitionTracing, @@ -2537,7 +2538,13 @@ export function captureCommitPhaseError( return; } - let fiber = nearestMountedAncestor; + let fiber = null; + if (skipUnmountedBoundaries) { + fiber = nearestMountedAncestor; + } else { + fiber = sourceFiber.return; + } + while (fiber !== null) { if (fiber.tag === HostRoot) { captureCommitPhaseErrorOnRoot(fiber, sourceFiber, error); @@ -2570,9 +2577,14 @@ export function captureCommitPhaseError( } if (__DEV__) { + // TODO: Until we re-land skipUnmountedBoundaries (see #20147), this warning + // will fire for errors that are thrown by destroy functions inside deleted + // trees. What it should instead do is propagate the error to the parent of + // the deleted tree. In the meantime, do not add this warning to the + // allowlist; this is only for our internal use. console.error( 'Internal React error: Attempted to capture a commit phase error ' + - 'inside a detached tree. This indicates a bug in React. Potential ' + + 'inside a detached tree. This indicates a bug in React. Likely ' + 'causes include deleting the same fiber more than once, committing an ' + 'already-finished tree, or an inconsistent return pointer.\n\n' + 'Error message:\n\n%s', diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js index 86ad6216d1286..d8bb6b16e29fb 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js @@ -35,6 +35,7 @@ import { enableSchedulingProfiler, disableSchedulerTimeoutInWorkLoop, enableStrictEffects, + skipUnmountedBoundaries, enableUpdaterTracking, enableCache, enableTransitionTracing, @@ -2537,7 +2538,13 @@ export function captureCommitPhaseError( return; } - let fiber = nearestMountedAncestor; + let fiber = null; + if (skipUnmountedBoundaries) { + fiber = nearestMountedAncestor; + } else { + fiber = sourceFiber.return; + } + while (fiber !== null) { if (fiber.tag === HostRoot) { captureCommitPhaseErrorOnRoot(fiber, sourceFiber, error); @@ -2570,9 +2577,14 @@ export function captureCommitPhaseError( } if (__DEV__) { + // TODO: Until we re-land skipUnmountedBoundaries (see #20147), this warning + // will fire for errors that are thrown by destroy functions inside deleted + // trees. What it should instead do is propagate the error to the parent of + // the deleted tree. In the meantime, do not add this warning to the + // allowlist; this is only for our internal use. console.error( 'Internal React error: Attempted to capture a commit phase error ' + - 'inside a detached tree. This indicates a bug in React. Potential ' + + 'inside a detached tree. This indicates a bug in React. Likely ' + 'causes include deleting the same fiber more than once, committing an ' + 'already-finished tree, or an inconsistent return pointer.\n\n' + 'Error message:\n\n%s', diff --git a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js index 97fffdcbd480b..811be09639158 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js +++ b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js @@ -2351,6 +2351,7 @@ describe('ReactHooksWithNoopRenderer', () => { }; }); + // @gate skipUnmountedBoundaries it('should use the nearest still-mounted boundary if there are no unmounted boundaries', () => { act(() => { ReactNoop.render( @@ -2376,6 +2377,7 @@ describe('ReactHooksWithNoopRenderer', () => { ]); }); + // @gate skipUnmountedBoundaries it('should skip unmounted boundaries and use the nearest still-mounted boundary', () => { function Conditional({showChildren}) { if (showChildren) { @@ -2418,6 +2420,7 @@ describe('ReactHooksWithNoopRenderer', () => { ]); }); + // @gate skipUnmountedBoundaries it('should call getDerivedStateFromError in the nearest still-mounted boundary', () => { function Conditional({showChildren}) { if (showChildren) { @@ -2461,6 +2464,7 @@ describe('ReactHooksWithNoopRenderer', () => { ]); }); + // @gate skipUnmountedBoundaries it('should rethrow error if there are no still-mounted boundaries', () => { function Conditional({showChildren}) { if (showChildren) { @@ -3186,6 +3190,7 @@ describe('ReactHooksWithNoopRenderer', () => { ]); }); + // @gate skipUnmountedBoundaries it('catches errors thrown in useLayoutEffect', () => { class ErrorBoundary extends React.Component { state = {error: null}; diff --git a/packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js b/packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js index 3aa27ceaba690..55ce08b45450b 100644 --- a/packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js @@ -1017,6 +1017,7 @@ describe('ReactIncrementalErrorHandling', () => { expect(Scheduler).toFlushAndYield(['Foo']); }); + // @gate skipUnmountedBoundaries it('should not attempt to recover an unmounting error boundary', () => { class Parent extends React.Component { componentWillUnmount() { diff --git a/packages/shared/ReactFeatureFlags.js b/packages/shared/ReactFeatureFlags.js index ba8a3bd437cc9..d33d2a92d29c7 100644 --- a/packages/shared/ReactFeatureFlags.js +++ b/packages/shared/ReactFeatureFlags.js @@ -28,6 +28,10 @@ export const enablePersistentOffscreenHostContainer = false; // like migrating internal callers or performance testing. // ----------------------------------------------------------------------------- +// This rolled out to 10% public in www, so we should be able to land, but some +// internal tests need to be updated. The open source behavior is correct. +export const skipUnmountedBoundaries = true; + // Destroy layout effects for components that are hidden because something // suspended in an update and recreate them when they are shown again (after the // suspended boundary has resolved). Note that this should be an uncommon use diff --git a/packages/shared/forks/ReactFeatureFlags.native-fb.js b/packages/shared/forks/ReactFeatureFlags.native-fb.js index 5bf0f37db9eb0..1eb2086c896ba 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-fb.js +++ b/packages/shared/forks/ReactFeatureFlags.native-fb.js @@ -56,6 +56,7 @@ export const enableComponentStackLocations = false; export const enableLegacyFBSupport = false; export const enableFilterEmptyStringAttributesDOM = false; export const disableNativeComponentFrames = false; +export const skipUnmountedBoundaries = false; export const deletedTreeCleanUpLevel = 3; export const enableSuspenseLayoutEffectSemantics = false; export const enableGetInspectorDataForInstanceInProduction = true; diff --git a/packages/shared/forks/ReactFeatureFlags.native-oss.js b/packages/shared/forks/ReactFeatureFlags.native-oss.js index 5c97146a82b20..e9850c400a9a7 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-oss.js +++ b/packages/shared/forks/ReactFeatureFlags.native-oss.js @@ -47,6 +47,7 @@ export const enableComponentStackLocations = false; export const enableLegacyFBSupport = false; export const enableFilterEmptyStringAttributesDOM = false; export const disableNativeComponentFrames = false; +export const skipUnmountedBoundaries = false; export const deletedTreeCleanUpLevel = 3; export const enableSuspenseLayoutEffectSemantics = false; export const enableGetInspectorDataForInstanceInProduction = false; diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.js index b44bc97206d04..cd488a2b7e5d6 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.js @@ -47,6 +47,7 @@ export const enableComponentStackLocations = true; export const enableLegacyFBSupport = false; export const enableFilterEmptyStringAttributesDOM = false; export const disableNativeComponentFrames = false; +export const skipUnmountedBoundaries = false; export const deletedTreeCleanUpLevel = 3; export const enableSuspenseLayoutEffectSemantics = false; export const enableGetInspectorDataForInstanceInProduction = false; diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.native.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.native.js index 4b8c1b774179f..54828d7ebd6aa 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.native.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.native.js @@ -43,6 +43,7 @@ export const enableComponentStackLocations = false; export const enableLegacyFBSupport = false; export const enableFilterEmptyStringAttributesDOM = false; export const disableNativeComponentFrames = false; +export const skipUnmountedBoundaries = false; export const deletedTreeCleanUpLevel = 3; export const enableSuspenseLayoutEffectSemantics = false; export const enableGetInspectorDataForInstanceInProduction = false; diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js index b6f620e2fb706..6d1dcfa842cc5 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js @@ -47,6 +47,7 @@ export const enableComponentStackLocations = true; export const enableLegacyFBSupport = false; export const enableFilterEmptyStringAttributesDOM = false; export const disableNativeComponentFrames = false; +export const skipUnmountedBoundaries = false; export const deletedTreeCleanUpLevel = 3; export const enableSuspenseLayoutEffectSemantics = false; export const enableGetInspectorDataForInstanceInProduction = false; diff --git a/packages/shared/forks/ReactFeatureFlags.testing.js b/packages/shared/forks/ReactFeatureFlags.testing.js index dbfc8fb704532..e6c46ee341f01 100644 --- a/packages/shared/forks/ReactFeatureFlags.testing.js +++ b/packages/shared/forks/ReactFeatureFlags.testing.js @@ -47,6 +47,7 @@ export const enableComponentStackLocations = true; export const enableLegacyFBSupport = false; export const enableFilterEmptyStringAttributesDOM = false; export const disableNativeComponentFrames = false; +export const skipUnmountedBoundaries = false; export const deletedTreeCleanUpLevel = 3; export const enableSuspenseLayoutEffectSemantics = false; export const enableGetInspectorDataForInstanceInProduction = false; diff --git a/packages/shared/forks/ReactFeatureFlags.testing.www.js b/packages/shared/forks/ReactFeatureFlags.testing.www.js index 95fa25cf592ce..0ae82026016ac 100644 --- a/packages/shared/forks/ReactFeatureFlags.testing.www.js +++ b/packages/shared/forks/ReactFeatureFlags.testing.www.js @@ -47,6 +47,7 @@ export const enableComponentStackLocations = true; export const enableLegacyFBSupport = !__EXPERIMENTAL__; export const enableFilterEmptyStringAttributesDOM = false; export const disableNativeComponentFrames = false; +export const skipUnmountedBoundaries = true; export const deletedTreeCleanUpLevel = 3; export const enableSuspenseLayoutEffectSemantics = false; export const enableGetInspectorDataForInstanceInProduction = false; diff --git a/packages/shared/forks/ReactFeatureFlags.www-dynamic.js b/packages/shared/forks/ReactFeatureFlags.www-dynamic.js index 36627a5d50d30..173713b36cf18 100644 --- a/packages/shared/forks/ReactFeatureFlags.www-dynamic.js +++ b/packages/shared/forks/ReactFeatureFlags.www-dynamic.js @@ -17,6 +17,7 @@ export const warnAboutSpreadingKeyToJSX = __VARIANT__; export const disableInputAttributeSyncing = __VARIANT__; export const enableFilterEmptyStringAttributesDOM = __VARIANT__; export const enableLegacyFBSupport = __VARIANT__; +export const skipUnmountedBoundaries = __VARIANT__; export const enableUseRefAccessWarning = __VARIANT__; export const deletedTreeCleanUpLevel = __VARIANT__ ? 3 : 1; export const enableProfilerNestedUpdateScheduledHook = __VARIANT__; diff --git a/packages/shared/forks/ReactFeatureFlags.www.js b/packages/shared/forks/ReactFeatureFlags.www.js index ac50fe4fd9f3b..cf61f17054d41 100644 --- a/packages/shared/forks/ReactFeatureFlags.www.js +++ b/packages/shared/forks/ReactFeatureFlags.www.js @@ -24,6 +24,7 @@ export const { enableLegacyFBSupport, deferRenderPhaseUpdateToNextBatch, enableDebugTracing, + skipUnmountedBoundaries, createRootStrictEffectsByDefault, enableUseRefAccessWarning, disableNativeComponentFrames,