Skip to content

Commit

Permalink
Enable getDerivedStateFromError (facebook#13746)
Browse files Browse the repository at this point in the history
* Removed the enableGetDerivedStateFromCatch feature flag (aka permanently enabled the feature)
* Forked/copied ReactErrorBoundaries to ReactLegacyErrorBoundaries for testing componentDidCatch
* Updated error boundaries tests to apply to getDerivedStateFromCatch
* Renamed getDerivedStateFromCatch -> getDerivedStateFromError
* Warn if boundary with only componentDidCatch swallows error
* Fixed a subtle reconciliation bug with render phase error boundary
  • Loading branch information
bvaughn authored and acdlite committed Oct 5, 2018
1 parent f0d79de commit 87619f4
Show file tree
Hide file tree
Showing 25 changed files with 2,639 additions and 242 deletions.
342 changes: 185 additions & 157 deletions packages/react-dom/src/__tests__/ReactErrorBoundaries-test.internal.js

Large diffs are not rendered by default.

2,130 changes: 2,130 additions & 0 deletions packages/react-dom/src/__tests__/ReactLegacyErrorBoundaries-test.internal.js

Large diffs are not rendered by default.

3 changes: 3 additions & 0 deletions packages/react-dom/src/__tests__/ReactUpdates-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -1343,6 +1343,9 @@ describe('ReactUpdates', () => {

class ErrorBoundary extends React.Component {
componentDidCatch() {
// Schedule a no-op state update to avoid triggering a DEV warning in the test.
this.setState({});

this.props.parent.remount();
}
render() {
Expand Down
100 changes: 71 additions & 29 deletions packages/react-reconciler/src/ReactFiberBeginWork.js
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,6 @@ import {
import {captureWillSyncRenderPlaceholder} from './ReactFiberScheduler';
import ReactSharedInternals from 'shared/ReactSharedInternals';
import {
enableGetDerivedStateFromCatch,
enableSuspense,
debugRenderPhaseSideEffects,
debugRenderPhaseSideEffectsForStrictMode,
Expand Down Expand Up @@ -156,6 +155,38 @@ export function reconcileChildren(
}
}

function forceUnmountCurrentAndReconcile(
current: Fiber,
workInProgress: Fiber,
nextChildren: any,
renderExpirationTime: ExpirationTime,
) {
// This function is fork of reconcileChildren. It's used in cases where we
// want to reconcile without matching against the existing set. This has the
// effect of all current children being unmounted; even if the type and key
// are the same, the old child is unmounted and a new child is created.
//
// To do this, we're going to go through the reconcile algorithm twice. In
// the first pass, we schedule a deletion for all the current children by
// passing null.
workInProgress.child = reconcileChildFibers(
workInProgress,
current.child,
null,
renderExpirationTime,
);
// In the second pass, we mount the new children. The trick here is that we
// pass null in place of where we usually pass the current child set. This has
// the effect of remounting all children regardless of whether their their
// identity matches.
workInProgress.child = reconcileChildFibers(
workInProgress,
null,
nextChildren,
renderExpirationTime,
);
}

function updateForwardRef(
current: Fiber | null,
workInProgress: Fiber,
Expand Down Expand Up @@ -444,8 +475,7 @@ function finishClassComponent(
let nextChildren;
if (
didCaptureError &&
(!enableGetDerivedStateFromCatch ||
typeof Component.getDerivedStateFromCatch !== 'function')
typeof Component.getDerivedStateFromError !== 'function'
) {
// If we captured an error, but getDerivedStateFrom catch is not defined,
// unmount all the children. componentDidCatch will schedule an update to
Expand Down Expand Up @@ -477,20 +507,25 @@ function finishClassComponent(
// React DevTools reads this flag.
workInProgress.effectTag |= PerformedWork;
if (current !== null && didCaptureError) {
// If we're recovering from an error, reconcile twice: first to delete
// all the existing children.
reconcileChildren(current, workInProgress, null, renderExpirationTime);
workInProgress.child = null;
// Now we can continue reconciling like normal. This has the effect of
// remounting all children regardless of whether their their
// identity matches.
// If we're recovering from an error, reconcile without reusing any of
// the existing children. Conceptually, the normal children and the children
// that are shown on error are two different sets, so we shouldn't reuse
// normal children even if their identities match.
forceUnmountCurrentAndReconcile(
current,
workInProgress,
nextChildren,
renderExpirationTime,
);
} else {
reconcileChildren(
current,
workInProgress,
nextChildren,
renderExpirationTime,
);
}
reconcileChildren(
current,
workInProgress,
nextChildren,
renderExpirationTime,
);

// Memoize props and state using the values we just used to render.
// TODO: Restructure so we never read values from the instance.
memoizeState(workInProgress, instance.state);
Expand Down Expand Up @@ -930,13 +965,6 @@ function updatePlaceholderComponent(
// suspended during the last commit. Switch to the placholder.
workInProgress.updateQueue = null;
nextDidTimeout = true;
// If we're recovering from an error, reconcile twice: first to delete
// all the existing children.
reconcileChildren(current, workInProgress, null, renderExpirationTime);
current.child = null;
// Now we can continue reconciling like normal. This has the effect of
// remounting all children regardless of whether their their
// identity matches.
} else {
nextDidTimeout = !alreadyCaptured;
}
Expand All @@ -963,14 +991,28 @@ function updatePlaceholderComponent(
nextChildren = nextDidTimeout ? nextProps.fallback : children;
}

if (current !== null && nextDidTimeout !== workInProgress.memoizedState) {
// We're about to switch from the placeholder children to the normal
// children, or vice versa. These are two different conceptual sets that
// happen to be stored in the same set. Call this special function to
// force the new set not to match with the current set.
// TODO: The proper way to model this is by storing each set separately.
forceUnmountCurrentAndReconcile(
current,
workInProgress,
nextChildren,
renderExpirationTime,
);
} else {
reconcileChildren(
current,
workInProgress,
nextChildren,
renderExpirationTime,
);
}
workInProgress.memoizedProps = nextProps;
workInProgress.memoizedState = nextDidTimeout;
reconcileChildren(
current,
workInProgress,
nextChildren,
renderExpirationTime,
);
return workInProgress.child;
} else {
return null;
Expand Down
4 changes: 2 additions & 2 deletions packages/react-reconciler/src/ReactFiberClassComponent.js
Original file line number Diff line number Diff line change
Expand Up @@ -466,10 +466,10 @@ function checkClassInstance(workInProgress: Fiber, ctor: any, newProps: any) {
name,
);
const noInstanceGetDerivedStateFromCatch =
typeof instance.getDerivedStateFromCatch !== 'function';
typeof instance.getDerivedStateFromError !== 'function';
warningWithoutStack(
noInstanceGetDerivedStateFromCatch,
'%s: getDerivedStateFromCatch() is defined as an instance method ' +
'%s: getDerivedStateFromError() is defined as an instance method ' +
'and will be ignored. Instead, declare it as a static method.',
name,
);
Expand Down
2 changes: 1 addition & 1 deletion packages/react-reconciler/src/ReactFiberScheduler.js
Original file line number Diff line number Diff line change
Expand Up @@ -1464,7 +1464,7 @@ function dispatch(
const ctor = fiber.type;
const instance = fiber.stateNode;
if (
typeof ctor.getDerivedStateFromCatch === 'function' ||
typeof ctor.getDerivedStateFromError === 'function' ||
(typeof instance.componentDidCatch === 'function' &&
!isAlreadyFailedLegacyErrorBoundary(instance))
) {
Expand Down
40 changes: 22 additions & 18 deletions packages/react-reconciler/src/ReactFiberUnwindWork.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ import type {CapturedValue} from './ReactCapturedValue';
import type {Update} from './ReactUpdateQueue';
import type {Thenable} from './ReactFiberScheduler';

import getComponentName from 'shared/getComponentName';
import warningWithoutStack from 'shared/warningWithoutStack';
import {
IndeterminateComponent,
FunctionalComponent,
Expand All @@ -33,11 +35,7 @@ import {
Update as UpdateEffect,
LifecycleEffectMask,
} from 'shared/ReactSideEffectTags';
import {
enableGetDerivedStateFromCatch,
enableSuspense,
enableSchedulerTracing,
} from 'shared/ReactFeatureFlags';
import {enableSuspense, enableSchedulerTracing} from 'shared/ReactFeatureFlags';
import {StrictMode, ConcurrentMode} from './ReactTypeOfMode';

import {createCapturedValue} from './ReactCapturedValue';
Expand Down Expand Up @@ -104,28 +102,22 @@ function createClassErrorUpdate(
): Update<mixed> {
const update = createUpdate(expirationTime);
update.tag = CaptureUpdate;
const getDerivedStateFromCatch = fiber.type.getDerivedStateFromCatch;
if (
enableGetDerivedStateFromCatch &&
typeof getDerivedStateFromCatch === 'function'
) {
const getDerivedStateFromError = fiber.type.getDerivedStateFromError;
if (typeof getDerivedStateFromError === 'function') {
const error = errorInfo.value;
update.payload = () => {
return getDerivedStateFromCatch(error);
return getDerivedStateFromError(error);
};
}

const inst = fiber.stateNode;
if (inst !== null && typeof inst.componentDidCatch === 'function') {
update.callback = function callback() {
if (
!enableGetDerivedStateFromCatch ||
getDerivedStateFromCatch !== 'function'
) {
if (typeof getDerivedStateFromError !== 'function') {
// To preserve the preexisting retry behavior of error boundaries,
// we keep track of which ones already failed during this batch.
// This gets reset before we yield back to the browser.
// TODO: Warn in strict mode if getDerivedStateFromCatch is
// TODO: Warn in strict mode if getDerivedStateFromError is
// not defined.
markLegacyErrorBoundaryAsFailed(this);
}
Expand All @@ -135,6 +127,19 @@ function createClassErrorUpdate(
this.componentDidCatch(error, {
componentStack: stack !== null ? stack : '',
});
if (__DEV__) {
if (typeof getDerivedStateFromError !== 'function') {
// If componentDidCatch is the only error boundary method defined,
// then it needs to call setState to recover from errors.
// If no state update is scheduled then the boundary will swallow the error.
warningWithoutStack(
fiber.expirationTime === Sync,
'%s: Error boundaries should implement getDerivedStateFromError(). ' +
'In that method, return a state update to display an error message or fallback UI.',
getComponentName(fiber.type) || 'Unknown',
);
}
}
};
}
return update;
Expand Down Expand Up @@ -364,8 +369,7 @@ function throwException(
const instance = workInProgress.stateNode;
if (
(workInProgress.effectTag & DidCapture) === NoEffect &&
((typeof ctor.getDerivedStateFromCatch === 'function' &&
enableGetDerivedStateFromCatch) ||
(typeof ctor.getDerivedStateFromError === 'function' ||
(instance !== null &&
typeof instance.componentDidCatch === 'function' &&
!isAlreadyFailedLegacyErrorBoundary(instance)))
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,111 @@
const jestDiff = require('jest-diff');

describe('ErrorBoundaryReconciliation', () => {
let BrokenRender;
let DidCatchErrorBoundary;
let GetDerivedErrorBoundary;
let React;
let ReactFeatureFlags;
let ReactTestRenderer;
let span;

beforeEach(() => {
jest.resetModules();

ReactFeatureFlags = require('shared/ReactFeatureFlags');
ReactFeatureFlags.replayFailedUnitOfWorkWithInvokeGuardedCallback = false;
ReactTestRenderer = require('react-test-renderer');
React = require('react');

DidCatchErrorBoundary = class extends React.Component {
state = {error: null};
componentDidCatch(error) {
this.setState({error});
}
render() {
return this.state.error
? React.createElement(this.props.fallbackTagName, {
prop: 'ErrorBoundary',
})
: this.props.children;
}
};

GetDerivedErrorBoundary = class extends React.Component {
state = {error: null};
static getDerivedStateFromError(error) {
return {error};
}
render() {
return this.state.error
? React.createElement(this.props.fallbackTagName, {
prop: 'ErrorBoundary',
})
: this.props.children;
}
};

const InvalidType = undefined;
BrokenRender = ({fail}) =>
fail ? <InvalidType /> : <span prop="BrokenRender" />;

function toHaveRenderedChildren(renderer, children) {
let actual, expected;
try {
actual = renderer.toJSON();
expected = ReactTestRenderer.create(children).toJSON();
expect(actual).toEqual(expected);
} catch (error) {
return {
message: () => jestDiff(expected, actual),
pass: false,
};
}
return {pass: true};
}
expect.extend({toHaveRenderedChildren});
});

[true, false].forEach(isConcurrent => {
function sharedTest(ErrorBoundary, fallbackTagName) {
const renderer = ReactTestRenderer.create(
<ErrorBoundary fallbackTagName={fallbackTagName}>
<BrokenRender fail={false} />
</ErrorBoundary>,
{unstable_isConcurrent: isConcurrent},
);
if (isConcurrent) {
renderer.unstable_flushAll();
}
expect(renderer).toHaveRenderedChildren(<span prop="BrokenRender" />);

expect(() => {
renderer.update(
<ErrorBoundary fallbackTagName={fallbackTagName}>
<BrokenRender fail={true} />
</ErrorBoundary>,
);
if (isConcurrent) {
renderer.unstable_flushAll();
}
}).toWarnDev(isConcurrent ? ['invalid', 'invalid'] : ['invalid']);
expect(renderer).toHaveRenderedChildren(
React.createElement(fallbackTagName, {prop: 'ErrorBoundary'}),
);
}

describe(isConcurrent ? 'concurrent' : 'sync', () => {
it('componentDidCatch can recover by rendering an element of the same type', () =>
sharedTest(DidCatchErrorBoundary, 'span'));

it('componentDidCatch can recover by rendering an element of a different type', () =>
sharedTest(DidCatchErrorBoundary, 'div'));

it('getDerivedStateFromError can recover by rendering an element of the same type', () =>
sharedTest(GetDerivedErrorBoundary, 'span'));

it('getDerivedStateFromError can recover by rendering an element of a different type', () =>
sharedTest(GetDerivedErrorBoundary, 'div'));
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -2398,7 +2398,10 @@ describe('ReactIncremental', () => {
instance.setState({
throwError: true,
});
ReactNoop.flush();
expect(ReactNoop.flush).toWarnDev(
'Error boundaries should implement getDerivedStateFromError()',
{withoutStack: true},
);
});

it('should not recreate masked context unless inputs have changed', () => {
Expand Down
Loading

0 comments on commit 87619f4

Please sign in to comment.