Skip to content

Commit

Permalink
Warn if unsafe lifecycle methods are found in an async subtree (#12060)
Browse files Browse the repository at this point in the history
  • Loading branch information
bvaughn authored Jan 23, 2018
1 parent 4d65408 commit cba51ba
Show file tree
Hide file tree
Showing 10 changed files with 529 additions and 12 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -290,7 +290,10 @@ describe('ReactCallReturn', () => {
<Return value={2} />
</Call>,
);
ReactNoop.flush();
expect(ReactNoop.flush).toWarnDev(
'componentWillMount: Please update the following components ' +
'to use componentDidMount instead: Return',
);

expect(ops).toEqual([
'Mount Return 1',
Expand Down
166 changes: 166 additions & 0 deletions packages/react-reconciler/src/ReactDebugAsyncWarnings.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,166 @@
/**
* Copyright (c) 2013-present, Facebook, Inc.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*
* @flow
*/

import type {Fiber} from './ReactFiber';

import getComponentName from 'shared/getComponentName';
import {getStackAddendumByWorkInProgressFiber} from 'shared/ReactFiberComponentTreeHook';
import {AsyncUpdates} from './ReactTypeOfInternalContext';
import warning from 'fbjs/lib/warning';

type LIFECYCLE =
| 'UNSAFE_componentWillMount'
| 'UNSAFE_componentWillReceiveProps'
| 'UNSAFE_componentWillUpdate';
type LifecycleToComponentsMap = {[lifecycle: LIFECYCLE]: Array<Fiber>};
type FiberToLifecycleMap = Map<Fiber, LifecycleToComponentsMap>;

const ReactDebugAsyncWarnings = {
discardPendingWarnings(): void {},
flushPendingAsyncWarnings(): void {},
recordLifecycleWarnings(fiber: Fiber, instance: any): void {},
};

if (__DEV__) {
const LIFECYCLE_SUGGESTIONS = {
UNSAFE_componentWillMount: 'componentDidMount',
UNSAFE_componentWillReceiveProps: 'static getDerivedStateFromProps',
UNSAFE_componentWillUpdate: 'componentDidUpdate',
};

let pendingWarningsMap: FiberToLifecycleMap = new Map();

// Tracks components we have already warned about.
const didWarnSet = new Set();

ReactDebugAsyncWarnings.discardPendingWarnings = () => {
pendingWarningsMap = new Map();
};

ReactDebugAsyncWarnings.flushPendingAsyncWarnings = () => {
((pendingWarningsMap: any): FiberToLifecycleMap).forEach(
(lifecycleWarningsMap, asyncRoot) => {
const lifecyclesWarningMesages = [];

Object.keys(lifecycleWarningsMap).forEach(lifecycle => {
const lifecycleWarnings = lifecycleWarningsMap[lifecycle];
if (lifecycleWarnings.length > 0) {
const componentNames = new Set();
lifecycleWarnings.forEach(fiber => {
componentNames.add(getComponentName(fiber) || 'Component');
didWarnSet.add(fiber.type);
});

const formatted = lifecycle.replace('UNSAFE_', '');
const suggestion = LIFECYCLE_SUGGESTIONS[lifecycle];
const sortedComponentNames = Array.from(componentNames)
.sort()
.join(', ');

lifecyclesWarningMesages.push(
`${formatted}: Please update the following components to use ` +
`${suggestion} instead: ${sortedComponentNames}`,
);
}
});

if (lifecyclesWarningMesages.length > 0) {
const asyncRootComponentStack = getStackAddendumByWorkInProgressFiber(
asyncRoot,
);

warning(
false,
'Unsafe lifecycle methods were found within the following async tree:%s' +
'\n\n%s' +
'\n\nLearn more about this warning here:' +
'\nhttps://fb.me/react-async-component-lifecycle-hooks',
asyncRootComponentStack,
lifecyclesWarningMesages.join('\n\n'),
);
}
},
);

pendingWarningsMap = new Map();
};

const getAsyncRoot = (fiber: Fiber): Fiber => {
let maybeAsyncRoot = null;

while (fiber !== null) {
if (fiber.internalContextTag & AsyncUpdates) {
maybeAsyncRoot = fiber;
}

fiber = fiber.return;
}

return maybeAsyncRoot;
};

ReactDebugAsyncWarnings.recordLifecycleWarnings = (
fiber: Fiber,
instance: any,
) => {
const asyncRoot = getAsyncRoot(fiber);

// Dedup strategy: Warn once per component.
// This is difficult to track any other way since component names
// are often vague and are likely to collide between 3rd party libraries.
// An expand property is probably okay to use here since it's DEV-only,
// and will only be set in the event of serious warnings.
if (didWarnSet.has(fiber.type)) {
return;
}

let warningsForRoot;
if (!pendingWarningsMap.has(asyncRoot)) {
warningsForRoot = {
UNSAFE_componentWillMount: [],
UNSAFE_componentWillReceiveProps: [],
UNSAFE_componentWillUpdate: [],
};

pendingWarningsMap.set(asyncRoot, warningsForRoot);
} else {
warningsForRoot = pendingWarningsMap.get(asyncRoot);
}

const unsafeLifecycles = [];
if (
typeof instance.componentWillMount === 'function' ||
typeof instance.UNSAFE_componentWillMount === 'function'
) {
unsafeLifecycles.push('UNSAFE_componentWillMount');
}
if (
typeof instance.componentWillReceiveProps === 'function' ||
typeof instance.UNSAFE_componentWillReceiveProps === 'function'
) {
unsafeLifecycles.push('UNSAFE_componentWillReceiveProps');
}
if (
typeof instance.componentWillUpdate === 'function' ||
typeof instance.UNSAFE_componentWillUpdate === 'function'
) {
unsafeLifecycles.push('UNSAFE_componentWillUpdate');
}

if (unsafeLifecycles.length > 0) {
unsafeLifecycles.forEach(lifecycle => {
((warningsForRoot: any): LifecycleToComponentsMap)[lifecycle].push(
fiber,
);
});
}
};
}

export default ReactDebugAsyncWarnings;
12 changes: 12 additions & 0 deletions packages/react-reconciler/src/ReactFiberClassComponent.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import {
enableAsyncSubtreeAPI,
warnAboutDeprecatedLifecycles,
} from 'shared/ReactFeatureFlags';
import ReactDebugAsyncWarnings from './ReactDebugAsyncWarnings';
import {isMounted} from 'react-reconciler/reflection';
import * as ReactInstanceMap from 'shared/ReactInstanceMap';
import emptyObject from 'fbjs/lib/emptyObject';
Expand Down Expand Up @@ -642,6 +643,17 @@ export default function(
workInProgress.internalContextTag |= AsyncUpdates;
}

if (__DEV__) {
// If we're inside of an async sub-tree,
// Warn about any unsafe lifecycles on this class component.
if (workInProgress.internalContextTag & AsyncUpdates) {
ReactDebugAsyncWarnings.recordLifecycleWarnings(
workInProgress,
instance,
);
}
}

if (
typeof instance.UNSAFE_componentWillMount === 'function' ||
typeof instance.componentWillMount === 'function'
Expand Down
9 changes: 9 additions & 0 deletions packages/react-reconciler/src/ReactFiberScheduler.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import type {ExpirationTime} from './ReactFiberExpirationTime';
import {getStackAddendumByWorkInProgressFiber} from 'shared/ReactFiberComponentTreeHook';
import ReactErrorUtils from 'shared/ReactErrorUtils';
import {ReactCurrentOwner} from 'shared/ReactGlobalSharedState';
import ReactDebugAsyncWarnings from './ReactDebugAsyncWarnings';
import {
PerformedWork,
Placement,
Expand Down Expand Up @@ -310,6 +311,10 @@ export default function<T, P, I, TI, HI, PI, C, CC, CX, PL>(
}

function commitAllLifeCycles() {
if (__DEV__) {
ReactDebugAsyncWarnings.flushPendingAsyncWarnings();
}

while (nextEffect !== null) {
const effectTag = nextEffect.effectTag;

Expand Down Expand Up @@ -651,6 +656,10 @@ export default function<T, P, I, TI, HI, PI, C, CC, CX, PL>(
}

function performFailedUnitOfWork(workInProgress: Fiber): Fiber | null {
if (__DEV__) {
ReactDebugAsyncWarnings.discardPendingWarnings();
}

// The current, flushed, state of this fiber is the alternate.
// Ideally nothing should rely on this, but relying on it here
// means that we don't need an additional field on the work in
Expand Down
11 changes: 7 additions & 4 deletions packages/react-reconciler/src/__tests__/ReactIncremental-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -1430,8 +1430,8 @@ describe('ReactIncremental', () => {
changeState() {
this.setState({foo: 'bar'});
}
componentWillUpdate() {
ops.push('componentWillUpdate');
componentDidUpdate() {
ops.push('componentDidUpdate');
}
render() {
ops.push('render');
Expand All @@ -1451,7 +1451,7 @@ describe('ReactIncremental', () => {
instance.changeState();
ReactNoop.flush();

expect(ops).toEqual(['componentWillUpdate', 'render']);
expect(ops).toEqual(['render', 'componentDidUpdate']);
expect(instance.state).toEqual({foo: 'bar'});
});

Expand Down Expand Up @@ -2335,7 +2335,10 @@ describe('ReactIncremental', () => {
}

ReactNoop.render(<MyComponent />);
ReactNoop.flush();
expect(ReactNoop.flush).toWarnDev(
'componentWillReceiveProps: Please update the following components ' +
'to use static getDerivedStateFromProps instead: MyComponent',
);

expect(ops).toEqual([
'render',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -254,7 +254,12 @@ describe('ReactDebugFiberPerf', () => {
</Parent>,
);
addComment('Should not print a warning');
ReactNoop.flush();
expect(ReactNoop.flush).toWarnDev([
'componentWillMount: Please update the following components ' +
'to use componentDidMount instead: NotCascading' +
'\n\ncomponentWillReceiveProps: Please update the following components ' +
'to use static getDerivedStateFromProps instead: NotCascading',
]);
ReactNoop.render(
<Parent>
<NotCascading />
Expand Down Expand Up @@ -288,7 +293,14 @@ describe('ReactDebugFiberPerf', () => {
}
ReactNoop.render(<AllLifecycles />);
addComment('Mount');
ReactNoop.flush();
expect(ReactNoop.flush).toWarnDev(
'componentWillMount: Please update the following components ' +
'to use componentDidMount instead: AllLifecycles' +
'\n\ncomponentWillReceiveProps: Please update the following components ' +
'to use static getDerivedStateFromProps instead: AllLifecycles' +
'\n\ncomponentWillUpdate: Please update the following components ' +
'to use componentDidUpdate instead: AllLifecycles',
);
ReactNoop.render(<AllLifecycles />);
addComment('Update');
ReactNoop.flush();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,10 @@ describe('ReactIncrementalReflection', () => {
ops = [];

// Render the rest and commit the updates.
ReactNoop.flush();
expect(ReactNoop.flush).toWarnDev(
'componentWillMount: Please update the following components ' +
'to use componentDidMount instead: Component',
);

expect(ops).toEqual(['componentDidMount', true]);

Expand Down Expand Up @@ -97,7 +100,10 @@ describe('ReactIncrementalReflection', () => {
}

ReactNoop.render(<Foo mount={true} />);
ReactNoop.flush();
expect(ReactNoop.flush).toWarnDev(
'componentWillMount: Please update the following components ' +
'to use componentDidMount instead: Component',
);

expect(ops).toEqual(['Component']);
ops = [];
Expand Down Expand Up @@ -184,7 +190,12 @@ describe('ReactIncrementalReflection', () => {
// not find any host nodes in it.
expect(ReactNoop.findInstance(classInstance)).toBe(null);

ReactNoop.flush();
expect(ReactNoop.flush).toWarnDev(
'componentWillMount: Please update the following components ' +
'to use componentDidMount instead: Component' +
'\n\ncomponentWillUpdate: Please update the following components ' +
'to use componentDidUpdate instead: Component',
);

const hostSpan = classInstance.span;
expect(hostSpan).toBeDefined();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -346,7 +346,10 @@ describe('ReactIncrementalScheduling', () => {
}

ReactNoop.render(<Foo step={1} />);
ReactNoop.flush();
expect(ReactNoop.flush).toWarnDev(
'componentWillReceiveProps: Please update the following components ' +
'to use static getDerivedStateFromProps instead: Foo',
);

ReactNoop.render(<Foo step={2} />);
expect(ReactNoop.flush()).toEqual([
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -307,12 +307,16 @@ describe('ReactIncrementalUpdates', () => {
}
}
ReactNoop.render(<Foo />);
ReactNoop.flush();
expect(ReactNoop.flush).toWarnDev(
'componentWillReceiveProps: Please update the following components ' +
'to use static getDerivedStateFromProps instead: Foo',
);

ops = [];

ReactNoop.flushSync(() => {
instance.setState({a: 'a'});

ReactNoop.render(<Foo />); // Trigger componentWillReceiveProps
});

Expand Down
Loading

0 comments on commit cba51ba

Please sign in to comment.