Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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 @@ -371,6 +374,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 @@ -404,6 +412,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 @@ -112,6 +112,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 @@ -175,15 +176,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';
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should put this assignment inside the if block. So that we don't spend time doing it if we don't use the result.

Copy link
Author

Choose a reason for hiding this comment

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

but the if condition is based on this value. So, we need this statement here

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oops, you're right. I didn't realize this is only called when we already know we're gonna warn. Disregard my previous comment.

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