From ca01f359b9236292c749075bb2fd41bb7b569308 Mon Sep 17 00:00:00 2001 From: Ricky Date: Thu, 30 Mar 2023 20:58:13 -0400 Subject: [PATCH] Remove skipUnmountedBoundaries (#26489) # Overview Landing this flag internally, will test this PR in React Native before merging. --- .../ReactErrorBoundaries-test.internal.js | 1 - .../react-reconciler/src/ReactFiberWorkLoop.js | 16 ++-------------- .../__tests__/ReactHooksWithNoopRenderer-test.js | 9 --------- ...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 - .../forks/ReactFeatureFlags.www-dynamic.js | 1 - packages/shared/forks/ReactFeatureFlags.www.js | 1 - 12 files changed, 2 insertions(+), 36 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactErrorBoundaries-test.internal.js b/packages/react-dom/src/__tests__/ReactErrorBoundaries-test.internal.js index 4124534f9c38a..040234f5c8059 100644 --- a/packages/react-dom/src/__tests__/ReactErrorBoundaries-test.internal.js +++ b/packages/react-dom/src/__tests__/ReactErrorBoundaries-test.internal.js @@ -43,7 +43,6 @@ 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('internal-test-utils').act; diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.js b/packages/react-reconciler/src/ReactFiberWorkLoop.js index 32ffdbfefdfa2..f1810f8a785c1 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.js @@ -33,7 +33,6 @@ import { enableDebugTracing, enableSchedulingProfiler, disableSchedulerTimeoutInWorkLoop, - skipUnmountedBoundaries, enableUpdaterTracking, enableCache, enableTransitionTracing, @@ -3517,13 +3516,7 @@ export function captureCommitPhaseError( return; } - let fiber = null; - if (skipUnmountedBoundaries) { - fiber = nearestMountedAncestor; - } else { - fiber = sourceFiber.return; - } - + let fiber = nearestMountedAncestor; while (fiber !== null) { if (fiber.tag === HostRoot) { captureCommitPhaseErrorOnRoot(fiber, sourceFiber, error); @@ -3555,14 +3548,9 @@ 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. Likely ' + + 'inside a detached tree. This indicates a bug in React. Potential ' + '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 e7dd2a2eef6b8..c62d12075f7b0 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js +++ b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js @@ -2254,7 +2254,6 @@ describe('ReactHooksWithNoopRenderer', () => { }; }); - // @gate skipUnmountedBoundaries it('should use the nearest still-mounted boundary if there are no unmounted boundaries', async () => { await act(() => { ReactNoop.render( @@ -2280,7 +2279,6 @@ describe('ReactHooksWithNoopRenderer', () => { ]); }); - // @gate skipUnmountedBoundaries it('should skip unmounted boundaries and use the nearest still-mounted boundary', async () => { function Conditional({showChildren}) { if (showChildren) { @@ -2323,7 +2321,6 @@ describe('ReactHooksWithNoopRenderer', () => { ]); }); - // @gate skipUnmountedBoundaries it('should call getDerivedStateFromError in the nearest still-mounted boundary', async () => { function Conditional({showChildren}) { if (showChildren) { @@ -2367,7 +2364,6 @@ describe('ReactHooksWithNoopRenderer', () => { ); }); - // @gate skipUnmountedBoundaries it('should rethrow error if there are no still-mounted boundaries', async () => { function Conditional({showChildren}) { if (showChildren) { @@ -2531,10 +2527,6 @@ describe('ReactHooksWithNoopRenderer', () => { assertLog(['layout destroy', 'passive destroy']); }); - // TODO: This test fails when skipUnmountedBoundaries is disabled. However, - // it's also rolled out to open source already and partially to www. So - // we should probably just land it. - // @gate skipUnmountedBoundaries it('assumes passive effect destroy function is either a function or undefined', async () => { function App(props) { useEffect(() => { @@ -3117,7 +3109,6 @@ describe('ReactHooksWithNoopRenderer', () => { assertLog(['Unmount normal [current: 1]', 'Mount normal [current: 1]']); }); - // @gate skipUnmountedBoundaries it('catches errors thrown in useLayoutEffect', async () => { 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 2e7bdf0064080..2f6b225f2f60e 100644 --- a/packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js @@ -962,7 +962,6 @@ describe('ReactIncrementalErrorHandling', () => { await waitForAll(['Foo']); }); - // @gate skipUnmountedBoundaries it('should not attempt to recover an unmounting error boundary', async () => { class Parent extends React.Component { componentWillUnmount() { diff --git a/packages/shared/ReactFeatureFlags.js b/packages/shared/ReactFeatureFlags.js index 7a77a48f4eec3..dcd7eb6b27876 100644 --- a/packages/shared/ReactFeatureFlags.js +++ b/packages/shared/ReactFeatureFlags.js @@ -34,10 +34,6 @@ export const revertRemovalOfSiblingPrerendering = 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; - // TODO: Finish rolling out in www export const enableClientRenderFallbackOnTextMismatch = true; diff --git a/packages/shared/forks/ReactFeatureFlags.native-fb.js b/packages/shared/forks/ReactFeatureFlags.native-fb.js index 073f2f2041396..43946c62d6080 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-fb.js +++ b/packages/shared/forks/ReactFeatureFlags.native-fb.js @@ -58,7 +58,6 @@ export const enableClientRenderFallbackOnTextMismatch = true; export const enableComponentStackLocations = false; export const enableLegacyFBSupport = false; export const enableFilterEmptyStringAttributesDOM = false; -export const skipUnmountedBoundaries = false; export const enableGetInspectorDataForInstanceInProduction = true; export const createRootStrictEffectsByDefault = false; diff --git a/packages/shared/forks/ReactFeatureFlags.native-oss.js b/packages/shared/forks/ReactFeatureFlags.native-oss.js index e1cae707580f9..3f9aa248a92d5 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-oss.js +++ b/packages/shared/forks/ReactFeatureFlags.native-oss.js @@ -48,7 +48,6 @@ export const enableClientRenderFallbackOnTextMismatch = true; export const enableComponentStackLocations = false; export const enableLegacyFBSupport = false; export const enableFilterEmptyStringAttributesDOM = false; -export const skipUnmountedBoundaries = false; export const enableGetInspectorDataForInstanceInProduction = false; export const createRootStrictEffectsByDefault = false; diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.js index 06dc5c495d68c..bf50fbbf7cbc9 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.js @@ -48,7 +48,6 @@ export const enableClientRenderFallbackOnTextMismatch = true; export const enableComponentStackLocations = true; export const enableLegacyFBSupport = false; export const enableFilterEmptyStringAttributesDOM = false; -export const skipUnmountedBoundaries = false; export const enableGetInspectorDataForInstanceInProduction = false; export const createRootStrictEffectsByDefault = false; diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.native.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.native.js index bc8fc2630a81f..4c746f191e550 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.native.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.native.js @@ -39,7 +39,6 @@ export const disableModulePatternComponents = false; export const enableComponentStackLocations = false; export const enableLegacyFBSupport = false; export const enableFilterEmptyStringAttributesDOM = false; -export const skipUnmountedBoundaries = false; export const enableGetInspectorDataForInstanceInProduction = false; export const enableSuspenseAvoidThisFallback = false; export const enableSuspenseAvoidThisFallbackFizz = false; diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js index 98ba1f4a1ed1e..439b7b9d9be89 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js @@ -48,7 +48,6 @@ export const enableClientRenderFallbackOnTextMismatch = true; export const enableComponentStackLocations = true; export const enableLegacyFBSupport = false; export const enableFilterEmptyStringAttributesDOM = true; -export const skipUnmountedBoundaries = false; export const enableGetInspectorDataForInstanceInProduction = false; export const createRootStrictEffectsByDefault = false; diff --git a/packages/shared/forks/ReactFeatureFlags.www-dynamic.js b/packages/shared/forks/ReactFeatureFlags.www-dynamic.js index d567eedb9755d..9682825117a4b 100644 --- a/packages/shared/forks/ReactFeatureFlags.www-dynamic.js +++ b/packages/shared/forks/ReactFeatureFlags.www-dynamic.js @@ -16,7 +16,6 @@ export const disableInputAttributeSyncing = __VARIANT__; export const disableIEWorkarounds = __VARIANT__; export const enableLegacyFBSupport = __VARIANT__; -export const skipUnmountedBoundaries = __VARIANT__; export const enableUseRefAccessWarning = __VARIANT__; export const enableProfilerNestedUpdateScheduledHook = __VARIANT__; export const disableSchedulerTimeoutInWorkLoop = __VARIANT__; diff --git a/packages/shared/forks/ReactFeatureFlags.www.js b/packages/shared/forks/ReactFeatureFlags.www.js index 762b96c2cf08a..b379a4001c49e 100644 --- a/packages/shared/forks/ReactFeatureFlags.www.js +++ b/packages/shared/forks/ReactFeatureFlags.www.js @@ -22,7 +22,6 @@ export const { replayFailedUnitOfWorkWithInvokeGuardedCallback, enableLegacyFBSupport, enableDebugTracing, - skipUnmountedBoundaries, enableUseRefAccessWarning, enableLazyContextPropagation, enableUnifiedSyncLane,