Skip to content

(#2272) adds warning for setting state through this.state #2283

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

(#2272) adds warning for setting state through this.state #2283

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
61 changes: 39 additions & 22 deletions src/core/ReactCompositeComponent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ var ReactCompositeComponentInterface = {
* `this.props` if that prop is not specified (i.e. using an `in` check).
*
* This method is invoked before `getInitialState` and therefore cannot rely
* on `this.state` or use `this.setState`.
* on `this._state` or use `this.setState`.
*
* @return {object}
* @optional
Expand All @@ -155,7 +155,7 @@ var ReactCompositeComponentInterface = {

/**
* Invoked once before the component is mounted. The return value will be used
* as the initial value of `this.state`.
* as the initial value of `this._state`.
*
* getInitialState: function() {
* return {
Expand All @@ -176,7 +176,7 @@ var ReactCompositeComponentInterface = {
getChildContext: SpecPolicy.DEFINE_MANY_MERGED,

/**
* Uses props from `this.props` and state from `this.state` to render the
* Uses props from `this.props` and state from `this._state` to render the
* structure of the component.
*
* No guarantees are made about when or how often this method is invoked, so
Expand Down Expand Up @@ -249,7 +249,7 @@ var ReactCompositeComponentInterface = {
*
* shouldComponentUpdate: function(nextProps, nextState, nextContext) {
* return !equal(nextProps, this.props) ||
* !equal(nextState, this.state) ||
* !equal(nextState, this._state) ||
* !equal(nextContext, this.context);
* }
*
Expand All @@ -263,7 +263,7 @@ var ReactCompositeComponentInterface = {

/**
* Invoked when the component is about to update due to a transition from
* `this.props`, `this.state` and `this.context` to `nextProps`, `nextState`
* `this.props`, `this._state` and `this.context` to `nextProps`, `nextState`
* and `nextContext`.
*
* Use this as an opportunity to perform preparation before an update occurs.
Expand Down Expand Up @@ -722,7 +722,24 @@ var ReactCompositeComponentMixin = {
ReactComponent.Mixin.construct.apply(this, arguments);
ReactOwner.Mixin.construct.apply(this, arguments);

this.state = null;
Object.defineProperty(this, 'state', {
get: function () {
return this._state;
},

set: function (newState) {
if (__DEV__){
warning(
false,
'this.state: Setting state directly from this.state is not ' +
'recommended. Instead, use this.setState().'
);
}
this._state = newState;
}
});

this._state = null;
this._pendingState = null;

// This is the public post-processed context. The real context and pending
Expand Down Expand Up @@ -772,9 +789,9 @@ var ReactCompositeComponentMixin = {
this.context = this._processContext(this._descriptor._context);
this.props = this._processProps(this.props);

this.state = this.getInitialState ? this.getInitialState() : null;
this._state = this.getInitialState ? this.getInitialState() : null;
invariant(
typeof this.state === 'object' && !Array.isArray(this.state),
typeof this._state === 'object' && !Array.isArray(this._state),
'%s.getInitialState(): must return an object or null',
this.constructor.displayName || 'ReactCompositeComponent'
);
Expand All @@ -787,7 +804,7 @@ var ReactCompositeComponentMixin = {
// When mounting, calls to `setState` by `componentWillMount` will set
// `this._pendingState` without triggering a re-render.
if (this._pendingState) {
this.state = this._pendingState;
this._state = this._pendingState;
this._pendingState = null;
}
}
Expand Down Expand Up @@ -831,15 +848,15 @@ var ReactCompositeComponentMixin = {
// Some existing components rely on this.props even after they've been
// destroyed (in event handlers).
// TODO: this.props = null;
// TODO: this.state = null;
// TODO: this._state = null;
},

/**
* Sets a subset of the state. Always use this or `replaceState` to mutate
* state. You should treat `this.state` as immutable.
* state. You should treat `this._state` as immutable.
*
* There is no guarantee that `this.state` will be immediately updated, so
* accessing `this.state` after calling this method may return the old value.
* There is no guarantee that `this._state` will be immediately updated, so
* accessing `this._state` after calling this method may return the old value.
*
* There is no guarantee that calls to `setState` will run synchronously,
* as they may eventually be batched together. You can provide an optional
Expand All @@ -865,17 +882,17 @@ var ReactCompositeComponentMixin = {
}
// Merge with `_pendingState` if it exists, otherwise with existing state.
this.replaceState(
merge(this._pendingState || this.state, partialState),
merge(this._pendingState || this._state, partialState),
callback
);
},

/**
* Replaces all of the state. Always use this or `setState` to mutate state.
* You should treat `this.state` as immutable.
* You should treat `this._state` as immutable.
*
* There is no guarantee that `this.state` will be immediately updated, so
* accessing `this.state` after calling this method may return the old value.
* There is no guarantee that `this._state` will be immediately updated, so
* accessing `this._state` after calling this method may return the old value.
*
* @param {object} completeState Next state.
* @param {?function} callback Called after state is updated.
Expand Down Expand Up @@ -1043,7 +1060,7 @@ var ReactCompositeComponentMixin = {

this._compositeLifeCycleState = null;

var nextState = this._pendingState || this.state;
var nextState = this._pendingState || this._state;
this._pendingState = null;

var shouldUpdate =
Expand All @@ -1063,7 +1080,7 @@ var ReactCompositeComponentMixin = {

if (shouldUpdate) {
this._pendingForceUpdate = false;
// Will set `this.props`, `this.state` and `this.context`.
// Will set `this.props`, `this._state` and `this.context`.
this._performComponentUpdate(
nextDescriptor,
nextProps,
Expand All @@ -1076,7 +1093,7 @@ var ReactCompositeComponentMixin = {
// to set props and state.
this._descriptor = nextDescriptor;
this.props = nextProps;
this.state = nextState;
this._state = nextState;
this.context = nextContext;

// Owner cannot change because shouldUpdateReactComponent doesn't allow
Expand Down Expand Up @@ -1105,7 +1122,7 @@ var ReactCompositeComponentMixin = {
) {
var prevDescriptor = this._descriptor;
var prevProps = this.props;
var prevState = this.state;
var prevState = this._state;
var prevContext = this.context;

if (this.componentWillUpdate) {
Expand All @@ -1114,7 +1131,7 @@ var ReactCompositeComponentMixin = {

this._descriptor = nextDescriptor;
this.props = nextProps;
this.state = nextState;
this._state = nextState;
this.context = nextContext;

// Owner cannot change because shouldUpdateReactComponent doesn't allow
Expand Down
24 changes: 24 additions & 0 deletions src/core/__tests__/ReactCompositeComponent-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -945,6 +945,30 @@ describe('ReactCompositeComponent', function() {
}
});

it('should warn when trying to directly set state via this.state', function() {
var warn = console.warn;
console.warn = mocks.getMockFunction();

try {
var Component = React.createClass({
render: function() {
this.state = 'something';
return <div />;
}
});

var instance = ReactTestUtils.renderIntoDocument(<Component />);

expect(console.warn.mock.calls.length).toBe(1);
expect(console.warn.mock.calls[0][0]).toBe(
'Warning: this.state: Setting state directly from this.state is not ' +
'recommended. Instead, use this.setState().'
);
} finally {
console.warn = warn;
}
});

it('should warn when mispelling shouldComponentUpdate', function() {
var warn = console.warn;
console.warn = mocks.getMockFunction();
Expand Down