Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove skipUnmountedBoundaries #26489

Merged
merged 2 commits into from
Mar 31, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
16 changes: 2 additions & 14 deletions packages/react-reconciler/src/ReactFiberWorkLoop.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@ import {
enableDebugTracing,
enableSchedulingProfiler,
disableSchedulerTimeoutInWorkLoop,
skipUnmountedBoundaries,
enableUpdaterTracking,
enableCache,
enableTransitionTracing,
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -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) {
Expand Down Expand Up @@ -2323,7 +2321,6 @@ describe('ReactHooksWithNoopRenderer', () => {
]);
});

// @gate skipUnmountedBoundaries
it('should call getDerivedStateFromError in the nearest still-mounted boundary', async () => {
function Conditional({showChildren}) {
if (showChildren) {
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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(() => {
Expand Down Expand Up @@ -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};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down
4 changes: 0 additions & 4 deletions packages/shared/ReactFeatureFlags.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
1 change: 0 additions & 1 deletion packages/shared/forks/ReactFeatureFlags.native-fb.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
1 change: 0 additions & 1 deletion packages/shared/forks/ReactFeatureFlags.native-oss.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
1 change: 0 additions & 1 deletion packages/shared/forks/ReactFeatureFlags.test-renderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
1 change: 0 additions & 1 deletion packages/shared/forks/ReactFeatureFlags.www-dynamic.js
Original file line number Diff line number Diff line change
Expand Up @@ -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__;
Expand Down
1 change: 0 additions & 1 deletion packages/shared/forks/ReactFeatureFlags.www.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ export const {
replayFailedUnitOfWorkWithInvokeGuardedCallback,
enableLegacyFBSupport,
enableDebugTracing,
skipUnmountedBoundaries,
enableUseRefAccessWarning,
enableLazyContextPropagation,
enableUnifiedSyncLane,
Expand Down