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

Add back skipUnmountedBoundaries flag only for www #23383

Merged
merged 1 commit into from
Feb 28, 2022
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 @@ -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;
Expand Down
16 changes: 14 additions & 2 deletions packages/react-reconciler/src/ReactFiberWorkLoop.new.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ import {
enableSchedulingProfiler,
disableSchedulerTimeoutInWorkLoop,
enableStrictEffects,
skipUnmountedBoundaries,
enableUpdaterTracking,
enableCache,
enableTransitionTracing,
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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',
Expand Down
16 changes: 14 additions & 2 deletions packages/react-reconciler/src/ReactFiberWorkLoop.old.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ import {
enableSchedulingProfiler,
disableSchedulerTimeoutInWorkLoop,
enableStrictEffects,
skipUnmountedBoundaries,
enableUpdaterTracking,
enableCache,
enableTransitionTracing,
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -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) {
Expand Down Expand Up @@ -2418,6 +2420,7 @@ describe('ReactHooksWithNoopRenderer', () => {
]);
});

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

// @gate skipUnmountedBoundaries
it('should rethrow error if there are no still-mounted boundaries', () => {
function Conditional({showChildren}) {
if (showChildren) {
Expand Down Expand Up @@ -3186,6 +3190,7 @@ 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,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() {
Expand Down
4 changes: 4 additions & 0 deletions packages/shared/ReactFeatureFlags.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
1 change: 1 addition & 0 deletions packages/shared/forks/ReactFeatureFlags.native-fb.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
1 change: 1 addition & 0 deletions packages/shared/forks/ReactFeatureFlags.native-oss.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
1 change: 1 addition & 0 deletions packages/shared/forks/ReactFeatureFlags.test-renderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
1 change: 1 addition & 0 deletions packages/shared/forks/ReactFeatureFlags.testing.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
1 change: 1 addition & 0 deletions packages/shared/forks/ReactFeatureFlags.testing.www.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
1 change: 1 addition & 0 deletions packages/shared/forks/ReactFeatureFlags.www-dynamic.js
Original file line number Diff line number Diff line change
Expand Up @@ -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__;
Expand Down
1 change: 1 addition & 0 deletions packages/shared/forks/ReactFeatureFlags.www.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ export const {
enableLegacyFBSupport,
deferRenderPhaseUpdateToNextBatch,
enableDebugTracing,
skipUnmountedBoundaries,
createRootStrictEffectsByDefault,
enableUseRefAccessWarning,
disableNativeComponentFrames,
Expand Down