Skip to content
Permalink
Browse files

Enable getDerivedStateFromError (facebook#13746)

* 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 jetoneza committed Sep 28, 2018
1 parent e9b525f commit 4a56f6e70f1db05f303949de8cd76395fbe5a790
Showing with 2,639 additions and 242 deletions.
  1. +185 −157 packages/react-dom/src/__tests__/ReactErrorBoundaries-test.internal.js
  2. +2,130 −0 packages/react-dom/src/__tests__/ReactLegacyErrorBoundaries-test.internal.js
  3. +3 −0 packages/react-dom/src/__tests__/ReactUpdates-test.js
  4. +71 −29 packages/react-reconciler/src/ReactFiberBeginWork.js
  5. +2 −2 packages/react-reconciler/src/ReactFiberClassComponent.js
  6. +1 −1 packages/react-reconciler/src/ReactFiberScheduler.js
  7. +22 −18 packages/react-reconciler/src/ReactFiberUnwindWork.js
  8. +111 −0 packages/react-reconciler/src/__tests__/ErrorBoundaryReconciliation-test.internal.js
  9. +4 −1 packages/react-reconciler/src/__tests__/ReactIncremental-test.internal.js
  10. +95 −3 packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js
  11. +1 −5 packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js
  12. +3 −3 packages/react/src/__tests__/ReactCoffeeScriptClass-test.coffee
  13. +3 −3 packages/react/src/__tests__/ReactES6Class-test.js
  14. +2 −3 packages/react/src/__tests__/ReactProfiler-test.internal.js
  15. +3 −3 packages/react/src/__tests__/ReactTypeScriptClass-test.ts
  16. +3 −3 packages/react/src/__tests__/createReactClassIntegration-test.js
  17. +0 −3 packages/shared/ReactFeatureFlags.js
  18. +0 −1 packages/shared/forks/ReactFeatureFlags.native-fabric-fb.js
  19. +0 −1 packages/shared/forks/ReactFeatureFlags.native-fabric-oss.js
  20. +0 −1 packages/shared/forks/ReactFeatureFlags.native-fb.js
  21. +0 −1 packages/shared/forks/ReactFeatureFlags.native-oss.js
  22. +0 −1 packages/shared/forks/ReactFeatureFlags.persistent.js
  23. +0 −1 packages/shared/forks/ReactFeatureFlags.test-renderer.js
  24. +0 −1 packages/shared/forks/ReactFeatureFlags.test-renderer.www.js
  25. +0 −1 packages/shared/forks/ReactFeatureFlags.www.js

Large diffs are not rendered by default.

Oops, something went wrong.

Large diffs are not rendered by default.

Oops, something went wrong.
@@ -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() {
@@ -46,7 +46,6 @@ import {
import {captureWillSyncRenderPlaceholder} from './ReactFiberScheduler';
import ReactSharedInternals from 'shared/ReactSharedInternals';
import {
enableGetDerivedStateFromCatch,
enableSuspense,
debugRenderPhaseSideEffects,
debugRenderPhaseSideEffectsForStrictMode,
@@ -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,
@@ -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
@@ -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);
@@ -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;
}
@@ -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;
@@ -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,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;
@@ -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)))
@@ -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'));
});
});
});
@@ -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', () => {
Oops, something went wrong.

0 comments on commit 4a56f6e

Please sign in to comment.
You can’t perform that action at this time.