Skip to content

Commit

Permalink
adds warning for this.state, enchances this.state
Browse files Browse the repository at this point in the history 
This commit changes this.state to this._state so that the user never
really touches this._state directly without being warned. It uses ES5
Object.defineProperty for catching the `set` operation. A test is also
added for that.

Closes facebook#2272
  • Loading branch information
Ciro S. Costa committed Oct 2, 2014
1 parent 35764c5 commit 3edfedb
Show file tree
Hide file tree
Showing 2 changed files with 63 additions and 22 deletions.
61 changes: 39 additions & 22 deletions src/core/ReactCompositeComponent.js
View file
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
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

0 comments on commit 3edfedb

Please sign in to comment.