Skip to content

Commit

Permalink
Deduplicated many warnings (facebook#11140) (facebook#11216)
Browse files Browse the repository at this point in the history
*  Deduplicated many warnings (facebook#11140)

*  Deduplicated the following warnings:

1.  Can only update a mounted or mounting component.
    This usually means you called setState, replaceState,
    or forceUpdate on an unmounted component. This is a no-op

2.  %s.componentWillReceiveProps(): Assigning directly to
    this.state is deprecated (except inside a component's
    constructor). Use setState instead.'

3.  An update (setState, replaceState, or forceUpdate) was scheduled
    from inside an update function. Update functions should be pure,
    with zero side-effects. Consider using componentDidUpdate or a
    callback.

4.  setState(...): Cannot call setState() inside getChildContext()

* Code review changes made for facebook#11140

* Minor style fix

* Test deduplication for noop updates in server renderer

* Test deduplication for cWRP warning

* Test deduplication for cWM setState warning

* Test deduplication for unnmounted setState warning

* Fix existing Flow typing

* Test deduplication for invalid updates

* Test deduplication of update-in-updater warning
  • Loading branch information
anushreesubramani authored and Ethan-Arrowood committed Dec 8, 2017
1 parent ed86315 commit 3b02564
Show file tree
Hide file tree
Showing 10 changed files with 96 additions and 24 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,10 @@ describe('ReactComponentLifeCycle', () => {
'unmounted component. This is a no-op.\n\nPlease check the code for the ' +
'StatefulComponent component.',
);

// Check deduplication
ReactTestUtils.renderIntoDocument(<StatefulComponent />);
expectDev(console.error.calls.count()).toBe(1);
});

it('should correctly determine if a component is mounted', () => {
Expand Down
13 changes: 13 additions & 0 deletions packages/react-dom/src/__tests__/ReactCompositeComponent-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -252,6 +252,9 @@ describe('ReactCompositeComponent', () => {
'component. This is a no-op.\n\nPlease check the code for the ' +
'Component component.',
);

instance.forceUpdate();
expectDev(console.error.calls.count()).toBe(1);
});

it('should warn about `setState` on unmounted components', () => {
Expand Down Expand Up @@ -391,6 +394,11 @@ describe('ReactCompositeComponent', () => {
expect(instance).toBe(instance2);
expect(renderedState).toBe(1);
expect(instance2.state.value).toBe(1);

// Test deduplication
ReactDOM.unmountComponentAtNode(container);
ReactDOM.render(<Component prop={123} />, container);
expectDev(console.error.calls.count()).toBe(1);
});

it('should warn about `setState` in getChildContext', () => {
Expand Down Expand Up @@ -424,6 +432,11 @@ describe('ReactCompositeComponent', () => {
expectDev(console.error.calls.argsFor(0)[0]).toBe(
'Warning: setState(...): Cannot call setState() inside getChildContext()',
);

// Test deduplication
ReactDOM.unmountComponentAtNode(container);
ReactDOM.render(<Component />, container);
expectDev(console.error.calls.count()).toBe(1);
});

it('should cleanup even if render() fatals', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -421,6 +421,10 @@ describe('ReactCompositeComponent-state', () => {
"this.state is deprecated (except inside a component's constructor). " +
'Use setState instead.',
);

// Check deduplication
ReactDOM.render(<Test />, container);
expect(console.error.calls.count()).toEqual(1);
});

it('should treat assigning to this.state inside cWM as a replaceState, with a warning', () => {
Expand Down
3 changes: 3 additions & 0 deletions packages/react-dom/src/__tests__/ReactServerRendering-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -656,8 +656,11 @@ describe('ReactDOMServer', () => {
' This usually means you called setState() outside componentWillMount() on the server.' +
' This is a no-op.\n\nPlease check the code for the Foo component.',
);

var markup = ReactDOMServer.renderToStaticMarkup(<Foo />);
expect(markup).toBe('<div>hello</div>');
jest.runOnlyPendingTimers();
expectDev(console.error.calls.count()).toBe(1);
});

it('warns with a no-op when an async forceUpdate is triggered', () => {
Expand Down
11 changes: 10 additions & 1 deletion packages/react-dom/src/server/ReactPartialRenderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,7 @@ var didWarnDefaultChecked = false;
var didWarnDefaultSelectValue = false;
var didWarnDefaultTextareaValue = false;
var didWarnInvalidOptionChildren = false;
var didWarnAboutNoopUpdateForComponent = {};
var valuePropNames = ['value', 'defaultValue'];
var newlineEatingTags = {
listing: true,
Expand Down Expand Up @@ -181,15 +182,23 @@ function warnNoop(
) {
if (__DEV__) {
var constructor = publicInstance.constructor;
const componentName =
(constructor && getComponentName(constructor)) || 'ReactClass';
const warningKey = `${componentName}.${callerName}`;
if (didWarnAboutNoopUpdateForComponent[warningKey]) {
return;
}

warning(
false,
'%s(...): Can only update a mounting component. ' +
'This usually means you called %s() outside componentWillMount() on the server. ' +
'This is a no-op.\n\nPlease check the code for the %s component.',
callerName,
callerName,
(constructor && getComponentName(constructor)) || 'ReactClass',
componentName,
);
didWarnAboutNoopUpdateForComponent[warningKey] = true;
}
}

Expand Down
19 changes: 12 additions & 7 deletions packages/react-reconciler/src/ReactFiberClassComponent.js
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ if (__DEV__) {
var warning = require('fbjs/lib/warning');

var {startPhaseTimer, stopPhaseTimer} = require('./ReactDebugFiberPerf');
var didWarnAboutStateAssignmentForComponent = {};

var warnOnInvalidCallback = function(callback: mixed, callerName: string) {
warning(
Expand Down Expand Up @@ -393,13 +394,17 @@ module.exports = function(

if (instance.state !== oldState) {
if (__DEV__) {
warning(
false,
'%s.componentWillReceiveProps(): Assigning directly to ' +
"this.state is deprecated (except inside a component's " +
'constructor). Use setState instead.',
getComponentName(workInProgress),
);
const componentName = getComponentName(workInProgress) || 'Component';
if (!didWarnAboutStateAssignmentForComponent[componentName]) {
warning(
false,
'%s.componentWillReceiveProps(): Assigning directly to ' +
"this.state is deprecated (except inside a component's " +
'constructor). Use setState instead.',
componentName,
);
didWarnAboutStateAssignmentForComponent[componentName] = true;
}
}
updater.enqueueReplaceState(instance, instance.state, null);
}
Expand Down
32 changes: 20 additions & 12 deletions packages/react-reconciler/src/ReactFiberScheduler.js
Original file line number Diff line number Diff line change
Expand Up @@ -101,41 +101,49 @@ if (__DEV__) {
} = require('./ReactDebugFiberPerf');

var didWarnAboutStateTransition = false;
var didWarnSetStateChildContext = false;
var didWarnStateUpdateForUnmountedComponent = {};

var warnAboutUpdateOnUnmounted = function(
instance: React$ComponentType<any>,
) {
const ctor = instance.constructor;
var warnAboutUpdateOnUnmounted = function(fiber: Fiber) {
const componentName = getComponentName(fiber) || 'ReactClass';
if (didWarnStateUpdateForUnmountedComponent[componentName]) {
return;
}
warning(
false,
'Can only update a mounted or mounting component. This usually means ' +
'you called setState, replaceState, or forceUpdate on an unmounted ' +
'component. This is a no-op.\n\nPlease check the code for the ' +
'%s component.',
(ctor && (ctor.displayName || ctor.name)) || 'ReactClass',
'Can only update a mounted or mounting ' +
'component. This usually means you called setState, replaceState, ' +
'or forceUpdate on an unmounted component. This is a no-op.\n\nPlease ' +
'check the code for the %s component.',
componentName,
);
didWarnStateUpdateForUnmountedComponent[componentName] = true;
};

var warnAboutInvalidUpdates = function(instance: React$ComponentType<any>) {
var warnAboutInvalidUpdates = function(instance: React$Component<any>) {
switch (ReactDebugCurrentFiber.phase) {
case 'getChildContext':
if (didWarnSetStateChildContext) {
return;
}
warning(
false,
'setState(...): Cannot call setState() inside getChildContext()',
);
didWarnSetStateChildContext = true;
break;
case 'render':
if (didWarnAboutStateTransition) {
return;
}
didWarnAboutStateTransition = true;
warning(
false,
'Cannot update during an existing state transition (such as within ' +
"`render` or another component's constructor). Render methods should " +
'be a pure function of props and state; constructor side-effects are ' +
'an anti-pattern, but can be moved to `componentWillMount`.',
);
didWarnAboutStateTransition = true;
break;
}
};
Expand Down Expand Up @@ -1229,7 +1237,7 @@ module.exports = function<T, P, I, TI, PI, C, CC, CX, PL>(
} else {
if (__DEV__) {
if (!isErrorRecovery && fiber.tag === ClassComponent) {
warnAboutUpdateOnUnmounted(fiber.stateNode);
warnAboutUpdateOnUnmounted(fiber);
}
}
return;
Expand Down
7 changes: 6 additions & 1 deletion packages/react-reconciler/src/ReactFiberUpdateQueue.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ const {NoWork} = require('./ReactFiberExpirationTime');

if (__DEV__) {
var warning = require('fbjs/lib/warning');
var didWarnUpdateInsideUpdate = false;
}

type PartialState<State, Props> =
Expand Down Expand Up @@ -132,14 +133,18 @@ function insertUpdateIntoFiber<State>(

// Warn if an update is scheduled from inside an updater function.
if (__DEV__) {
if (queue1.isProcessing || (queue2 !== null && queue2.isProcessing)) {
if (
(queue1.isProcessing || (queue2 !== null && queue2.isProcessing)) &&
!didWarnUpdateInsideUpdate
) {
warning(
false,
'An update (setState, replaceState, or forceUpdate) was scheduled ' +
'from inside an update function. Update functions should be pure, ' +
'with zero side-effects. Consider using componentDidUpdate or a ' +
'callback.',
);
didWarnUpdateInsideUpdate = true;
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -341,6 +341,19 @@ describe('ReactIncrementalUpdates', () => {
expect(instance.state).toEqual({a: 'a', b: 'b'});

expectDev(console.error.calls.count()).toBe(1);
console.error.calls.reset();
expectDev(console.error.calls.argsFor(0)[0]).toContain(
'An update (setState, replaceState, or forceUpdate) was scheduled ' +
'from inside an update function. Update functions should be pure, ' +
'with zero side-effects. Consider using componentDidUpdate or a ' +
'callback.',
);

// Test deduplication
instance.setState(function a() {
this.setState({a: 'a'});
return {b: 'b'};
});
ReactNoop.flush();
expectDev(console.error.calls.count()).toBe(1);
});
});
12 changes: 10 additions & 2 deletions packages/react/src/ReactNoopUpdateQueue.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,21 +9,29 @@

if (__DEV__) {
var warning = require('fbjs/lib/warning');
var didWarnStateUpdateForUnmountedComponent = {};
}

function warnNoop(publicInstance, callerName) {
if (__DEV__) {
var constructor = publicInstance.constructor;
const componentName =
(constructor && (constructor.displayName || constructor.name)) ||
'ReactClass';
const warningKey = `${componentName}.${callerName}`;
if (didWarnStateUpdateForUnmountedComponent[warningKey]) {
return;
}
warning(
false,
'%s(...): Can only update a mounted or mounting component. ' +
'This usually means you called %s() on an unmounted component. ' +
'This is a no-op.\n\nPlease check the code for the %s component.',
callerName,
callerName,
(constructor && (constructor.displayName || constructor.name)) ||
'ReactClass',
componentName,
);
didWarnStateUpdateForUnmountedComponent[warningKey] = true;
}
}

Expand Down

0 comments on commit 3b02564

Please sign in to comment.