Skip to content

Commit

Permalink
Land skipUnmountedBoundaries experiment (facebook#23322)
Browse files Browse the repository at this point in the history
This has been rolled out to 10% of Facebook users for months without
any issues.
  • Loading branch information
acdlite committed Feb 18, 2022
1 parent 54f785b commit 419ccc2
Show file tree
Hide file tree
Showing 15 changed files with 4 additions and 50 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,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('jest-react').act;
Expand Down
16 changes: 2 additions & 14 deletions packages/react-reconciler/src/ReactFiberWorkLoop.new.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@ import {
enableSchedulingProfiler,
disableSchedulerTimeoutInWorkLoop,
enableStrictEffects,
skipUnmountedBoundaries,
enableUpdaterTracking,
warnOnSubscriptionInsideStartTransition,
enableCache,
Expand Down Expand Up @@ -2445,13 +2444,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 @@ -2484,14 +2477,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
16 changes: 2 additions & 14 deletions packages/react-reconciler/src/ReactFiberWorkLoop.old.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@ import {
enableSchedulingProfiler,
disableSchedulerTimeoutInWorkLoop,
enableStrictEffects,
skipUnmountedBoundaries,
enableUpdaterTracking,
warnOnSubscriptionInsideStartTransition,
enableCache,
Expand Down Expand Up @@ -2445,13 +2444,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 @@ -2484,14 +2477,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 @@ -2351,7 +2351,6 @@ describe('ReactHooksWithNoopRenderer', () => {
};
});

// @gate skipUnmountedBoundaries
it('should use the nearest still-mounted boundary if there are no unmounted boundaries', () => {
act(() => {
ReactNoop.render(
Expand All @@ -2377,7 +2376,6 @@ describe('ReactHooksWithNoopRenderer', () => {
]);
});

// @gate skipUnmountedBoundaries
it('should skip unmounted boundaries and use the nearest still-mounted boundary', () => {
function Conditional({showChildren}) {
if (showChildren) {
Expand Down Expand Up @@ -2420,7 +2418,6 @@ describe('ReactHooksWithNoopRenderer', () => {
]);
});

// @gate skipUnmountedBoundaries
it('should call getDerivedStateFromError in the nearest still-mounted boundary', () => {
function Conditional({showChildren}) {
if (showChildren) {
Expand Down Expand Up @@ -2464,7 +2461,6 @@ describe('ReactHooksWithNoopRenderer', () => {
]);
});

// @gate skipUnmountedBoundaries
it('should rethrow error if there are no still-mounted boundaries', () => {
function Conditional({showChildren}) {
if (showChildren) {
Expand Down Expand Up @@ -3190,7 +3186,6 @@ describe('ReactHooksWithNoopRenderer', () => {
]);
});

// @gate skipUnmountedBoundaries
it('catches errors thrown in useLayoutEffect', () => {
class ErrorBoundary extends React.Component {
state = {error: null};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1017,7 +1017,6 @@ describe('ReactIncrementalErrorHandling', () => {
expect(Scheduler).toFlushAndYield(['Foo']);
});

// @gate skipUnmountedBoundaries
it('should not attempt to recover an unmounting error boundary', () => {
class Parent extends React.Component {
componentWillUnmount() {
Expand Down
6 changes: 0 additions & 6 deletions packages/shared/ReactFeatureFlags.js
Original file line number Diff line number Diff line change
Expand Up @@ -121,12 +121,6 @@ export const disableNativeComponentFrames = false;
// Internal only.
export const enableGetInspectorDataForInstanceInProduction = false;

// Errors that are thrown while unmounting (or after in the case of passive effects)
// should bypass any error boundaries that are also unmounting (or have unmounted)
// and be handled by the nearest still-mounted boundary.
// If there are no still-mounted boundaries, the errors should be rethrown.
export const skipUnmountedBoundaries = false;

// When a node is unmounted, recurse into the Fiber subtree and clean out
// references. Each level cleans up more fiber fields than the previous level.
// As far as we know, React itself doesn't leak, but because the Fiber contains
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 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;
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 @@ -49,7 +49,6 @@ 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;
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 @@ -49,7 +49,6 @@ 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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,6 @@ 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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,6 @@ 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;
Expand Down
1 change: 0 additions & 1 deletion packages/shared/forks/ReactFeatureFlags.testing.js
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,6 @@ 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;
Expand Down
1 change: 0 additions & 1 deletion packages/shared/forks/ReactFeatureFlags.testing.www.js
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,6 @@ 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;
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 @@ -17,7 +17,6 @@ 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__;
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 @@ -24,7 +24,6 @@ export const {
enableLegacyFBSupport,
deferRenderPhaseUpdateToNextBatch,
enableDebugTracing,
skipUnmountedBoundaries,
createRootStrictEffectsByDefault,
enableUseRefAccessWarning,
disableNativeComponentFrames,
Expand Down

0 comments on commit 419ccc2

Please sign in to comment.