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.

Sign up for GitHub

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

Jump to bottom

Land skipUnmountedBoundaries experiment #23322

Merged
merged 1 commit into from Feb 18, 2022
Merged

Land skipUnmountedBoundaries experiment #23322

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 0 additions & 1 deletion packages/react-dom/src/__tests__/ReactErrorBoundaries-test.internal.js
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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 ' +
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made a trivial change to this message to remind us to update the warning allowlist the next time we sync

'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
5 changes: 0 additions & 5 deletions packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js
View file
Open in desktop
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
1 change: 0 additions & 1 deletion packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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.test-renderer.native.js
View file
Open in desktop
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
1 change: 0 additions & 1 deletion packages/shared/forks/ReactFeatureFlags.test-renderer.www.js
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
Expand Up @@ -24,7 +24,6 @@ export const {
enableLegacyFBSupport,
deferRenderPhaseUpdateToNextBatch,
enableDebugTracing,
skipUnmountedBoundaries,
createRootStrictEffectsByDefault,
enableUseRefAccessWarning,
disableNativeComponentFrames,
Expand Down