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

Enable getDerivedStateFromError #13746

Merged
merged 10 commits into from
Sep 28, 2018
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'}),
);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test was useful for me since my initial "fix" to sync mode broke concurrent mode. I'm not sure about this amount of abstraction though. On the one hand, it makes it very easy to spot what's different between these otherwise very similar tests. On the other hand, it may be harder to read.


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