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

Large diffs are not rendered by default.

Oops, something went wrong.

Large diffs are not rendered by default.

Oops, something went wrong.
@@ -46,7 +46,6 @@ import {
import {captureWillSyncRenderPlaceholder} from './ReactFiberScheduler';
import ReactSharedInternals from 'shared/ReactSharedInternals';
import {
enableGetDerivedStateFromCatch,
enableSuspense,
debugRenderPhaseSideEffects,
debugRenderPhaseSideEffectsForStrictMode,
@@ -444,8 +443,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
@@ -480,7 +478,15 @@ function finishClassComponent(
// If we're recovering from an error, reconcile twice: first to delete
// all the existing children.
reconcileChildren(current, workInProgress, null, renderExpirationTime);
workInProgress.child = null;
// Forcefully reset children so that a subsequent reconciliation will always re-add.
// This is important if e.g. an error boundary renders an element of the same type.
// Concurrent renderer mode will always retry an extra time on failure,
// so the fiber we need to reset varies.
if (workInProgress.mode & ConcurrentMode) {
workInProgress.child = null;
} else {
current.child = null;
}

This comment has been minimized.

Copy link
@bvaughn

bvaughn Sep 27, 2018

Author Contributor

I'm not super confident about this fix ^

This comment has been minimized.

Copy link
@acdlite

acdlite Sep 27, 2018

Member

Hmm, yeah I don't think this is right. I think I know how to fix it though. Let's pair on it tomorrow morning.

// Now we can continue reconciling like normal. This has the effect of
// remounting all children regardless of whether their their
// identity matches.
@@ -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,
);
@@ -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))
) {
@@ -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,
@@ -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';
@@ -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);
}
@@ -135,6 +127,21 @@ 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.
const updateQueue = fiber.updateQueue;
warningWithoutStack(
updateQueue !== null && updateQueue.firstUpdate !== null,

This comment has been minimized.

Copy link
@acdlite

acdlite Sep 27, 2018

Member

This check isn't sufficient because firstUpdate might be a low priority update. It needs to be sync. You can check fiber.expirationTime === Sync instead.

This comment has been minimized.

Copy link
@bvaughn

bvaughn Sep 28, 2018

Author Contributor

Nice, thanks!

'%s: Error boundaries should implement getDerivedStateFromError(). ' +
'In that method, return a state update to display an error message or fallback UI, ' +
'or rethrow the error to let parent components handle it.',
getComponentName(fiber.type) || 'Unknown',
);
}
}
};
}
return update;
@@ -364,8 +371,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)))
@@ -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'}),
);
}

This comment has been minimized.

Copy link
@bvaughn

bvaughn Sep 27, 2018

Author Contributor

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'));
});
});
});
@@ -19,7 +19,6 @@ describe('ReactIncrementalErrorHandling', () => {
beforeEach(() => {
jest.resetModules();
ReactFeatureFlags = require('shared/ReactFeatureFlags');
ReactFeatureFlags.enableGetDerivedStateFromCatch = true;
ReactFeatureFlags.debugRenderPhaseSideEffectsForStrictMode = false;
ReactFeatureFlags.replayFailedUnitOfWorkWithInvokeGuardedCallback = false;
PropTypes = require('prop-types');
@@ -1442,10 +1441,10 @@ describe('ReactIncrementalErrorHandling', () => {
]);
});

it('does not provide component stack to the error boundary with getDerivedStateFromCatch', () => {
it('does not provide component stack to the error boundary with getDerivedStateFromError', () => {
class ErrorBoundary extends React.Component {
state = {error: null};
static getDerivedStateFromCatch(error, errorInfo) {
static getDerivedStateFromError(error, errorInfo) {
expect(errorInfo).toBeUndefined();
return {error};
}
@@ -129,15 +129,15 @@ describe 'ReactCoffeeScriptClass', ->
).toWarnDev 'Foo: getDerivedStateFromProps() is defined as an instance method and will be ignored. Instead, declare it as a static method.', {withoutStack: true}
undefined

it 'warns if getDerivedStateFromCatch is not static', ->
it 'warns if getDerivedStateFromError is not static', ->
class Foo extends React.Component
render: ->
div()
getDerivedStateFromCatch: ->
getDerivedStateFromError: ->
{}
expect(->
ReactDOM.render(React.createElement(Foo, foo: 'foo'), container)
).toWarnDev 'Foo: getDerivedStateFromCatch() is defined as an instance method and will be ignored. Instead, declare it as a static method.', {withoutStack: true}
).toWarnDev 'Foo: getDerivedStateFromError() is defined as an instance method and will be ignored. Instead, declare it as a static method.', {withoutStack: true}
undefined

it 'warns if getSnapshotBeforeUpdate is static', ->
@@ -147,17 +147,17 @@ describe('ReactES6Class', () => {
);
});

it('warns if getDerivedStateFromCatch is not static', () => {
it('warns if getDerivedStateFromError is not static', () => {
class Foo extends React.Component {
getDerivedStateFromCatch() {
getDerivedStateFromError() {
return {};
}
render() {
return <div />;
}
}
expect(() => ReactDOM.render(<Foo foo="foo" />, container)).toWarnDev(
'Foo: getDerivedStateFromCatch() is defined as an instance method ' +
'Foo: getDerivedStateFromError() is defined as an instance method ' +
'and will be ignored. Instead, declare it as a static method.',
{withoutStack: true},
);
@@ -36,7 +36,6 @@ function loadModules({
ReactFeatureFlags.debugRenderPhaseSideEffects = false;
ReactFeatureFlags.debugRenderPhaseSideEffectsForStrictMode = false;
ReactFeatureFlags.enableProfilerTimer = enableProfilerTimer;
ReactFeatureFlags.enableGetDerivedStateFromCatch = true;
ReactFeatureFlags.enableSchedulerTracing = enableSchedulerTracing;
ReactFeatureFlags.enableSuspense = enableSuspense;
ReactFeatureFlags.replayFailedUnitOfWorkWithInvokeGuardedCallback = replayFailedUnitOfWorkWithInvokeGuardedCallback;
@@ -985,7 +984,7 @@ describe('Profiler', () => {
);
});

it('should accumulate actual time after an error handled by getDerivedStateFromCatch()', () => {
it('should accumulate actual time after an error handled by getDerivedStateFromError()', () => {
const callback = jest.fn();

const ThrowsError = () => {
@@ -995,7 +994,7 @@ describe('Profiler', () => {

class ErrorBoundary extends React.Component {
state = {error: null};
static getDerivedStateFromCatch(error) {
static getDerivedStateFromError(error) {
return {error};
}
render() {
@@ -397,9 +397,9 @@ describe('ReactTypeScriptClass', function() {
);
});

it('warns if getDerivedStateFromCatch is not static', function() {
it('warns if getDerivedStateFromError is not static', function() {
class Foo extends React.Component {
getDerivedStateFromCatch() {
getDerivedStateFromError() {
return {};
}
render() {
@@ -409,7 +409,7 @@ describe('ReactTypeScriptClass', function() {
expect(function() {
ReactDOM.render(React.createElement(Foo, {foo: 'foo'}), container);
}).toWarnDev(
'Foo: getDerivedStateFromCatch() is defined as an instance method ' +
'Foo: getDerivedStateFromError() is defined as an instance method ' +
'and will be ignored. Instead, declare it as a static method.',
{withoutStack: true}
);
@@ -459,9 +459,9 @@ describe('create-react-class-integration', () => {
);
});

it('warns if getDerivedStateFromCatch is not static', () => {
it('warns if getDerivedStateFromError is not static', () => {
const Foo = createReactClass({
getDerivedStateFromCatch() {
getDerivedStateFromError() {
return {};
},
render() {
@@ -471,7 +471,7 @@ describe('create-react-class-integration', () => {
expect(() =>
ReactDOM.render(<Foo foo="foo" />, document.createElement('div')),
).toWarnDev(
'Component: getDerivedStateFromCatch() is defined as an instance method ' +
'Component: getDerivedStateFromError() is defined as an instance method ' +
'and will be ignored. Instead, declare it as a static method.',
{withoutStack: true},
);
@@ -10,9 +10,6 @@
// Exports ReactDOM.createRoot
export const enableUserTimingAPI = __DEV__;

// Experimental error-boundary API that can recover from errors within a single
// render phase
export const enableGetDerivedStateFromCatch = false;
// Suspense
export const enableSuspense = false;
// Helps identify side effects in begin-phase lifecycle hooks and setState reducers:
Oops, something went wrong.
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.