Skip to content
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.

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

setState now creates new versions instead of mutating state #1170

Merged
merged 2 commits into from Aug 2, 2018
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 6 additions & 4 deletions src/component.js
Expand Up @@ -46,14 +46,16 @@ extend(Component.prototype, {

/**
* Update component state and schedule a re-render.
* @param {object} state A hash of state properties to update with new values
* @param {object} state A dict of state properties to be shallowly merged
* into the current state, or a function that will produce such a dict. The
* function is called with the current state and props.
* @param {() => void} callback A function to be called once component state is
* updated
*/
setState(state, callback) {
let s = this.state;
if (!this.prevState) this.prevState = extend({}, s);
extend(s, typeof state==='function' ? state(s, this.props) : state);
const prev = this.prevState = this.state;
if (typeof state === 'function') state = state(prev, this.props);
this.state = extend(extend({}, prev), state);
if (callback) this._renderCallbacks.push(callback);
enqueueRender(this);
},
Expand Down
4 changes: 2 additions & 2 deletions src/vdom/component.js
Expand Up @@ -84,8 +84,8 @@ export function renderComponent(component, renderMode, mountAll, isChild) {
rendered, inst, cbase;

if (component.constructor.getDerivedStateFromProps) {
previousState = extend({}, previousState);
component.state = extend(state, component.constructor.getDerivedStateFromProps(props, state));
state = extend(extend({}, state), component.constructor.getDerivedStateFromProps(props, state));
component.state = state;
}

// if updating
Expand Down
57 changes: 56 additions & 1 deletion test/browser/lifecycle.js
Expand Up @@ -505,6 +505,31 @@ describe('Lifecycle methods', () => {
value: 3
});
});

it('should NOT mutate state, only create new versions', () => {
const stateConstant = {};
let componentState;

class Stateful extends Component {
static getDerivedStateFromProps() {
return {key: 'value'};
}

constructor() {
super(...arguments);
this.state = stateConstant;
}

componentDidMount() {
componentState = this.state;
}
}

render(<Stateful />, scratch);

expect(componentState).to.deep.equal({key: 'value'});
expect(stateConstant).to.deep.equal({});
});
});

describe("#getSnapshotBeforeUpdate", () => {
Expand Down Expand Up @@ -1186,7 +1211,7 @@ describe('Lifecycle methods', () => {
});


describe('shouldComponentUpdate', () => {
describe('#shouldComponentUpdate', () => {
let setState;

class Should extends Component {
Expand Down Expand Up @@ -1325,6 +1350,36 @@ describe('Lifecycle methods', () => {
});


describe('#setState', () => {
Copy link

Choose a reason for hiding this comment

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

Could I suggest also including a test of the function parameter version of the function. It looks like you are passing prev by reference here, so wouldn't any changes to it also mutate prev? It would be interesting to explore the effects of that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good suggestion, I just feel like a full setState test suite is somewhat out of scope for the PR. Strange that it's missing though.

Yes it passes the previous state by reference, exactly as it should. Treating the previous state as immutable is the responsibility of the user.

it('should NOT mutate state, only create new versions', () => {
const stateConstant = {};
let didMount = false;
let componentState;

class Stateful extends Component {
constructor() {
super(...arguments);
this.state = stateConstant;
}

componentDidMount() {
didMount = true;
this.setState({key: 'value'}, () => {
componentState = this.state;
});
}
}

render(<Stateful />, scratch);
rerender();

expect(didMount).to.equal(true);
expect(componentState).to.deep.equal({key: 'value'});
expect(stateConstant).to.deep.equal({});
});
}),


describe('Lifecycle DOM Timing', () => {
it('should be invoked when dom does (DidMount, WillUnmount) or does not (WillMount, DidUnmount) exist', () => {
let setState;
Expand Down