Store pending state updates directly in this.state

[sophiebits]

...and leave the last rendered state in currentState, as suggested in https://groups.google.com/d/msg/reactjs/R61kPjs-yXE/bk5AGv78RYAJ.

This is a breaking change in that now componentWillUpdate and shouldComponentUpdate need to look at this.currentState when comparing, not this.state. (Perhaps that's an argument towards including prevProps and prevState as arguments to those lifecycle methods...)

I don't feel super strongly about this but it seems that others do.

Fixes #122.

[petehunt]

@sebmarkbage @jordwalke

[Daniel15]

👍 I like this.

[jordwalke]

I'd like the ability to access both state and pending state. But the question is, which should be the default? The primary benefit to treating this.state as the default, authoritative state that you base all of your state transition logic upon, is that it is guaranteed to be consistent with all your subcomponents' props.

For example:

var App = React.createClass({
  getInitialState: function() {
    return {name: 'joe'};
  },
  handleClick: function() {
    alert('My name ' + this.state.name + 'should equal ' + this.refs.sayer.sayName());
    this.setState({
      name: this.state.name + Math.random()
    });
  },
  render: function() {
    return (
      <div onClick={this.handleClick}>
        <NameSayer name={this.state.name}>
        </NameSayer>
      </div>
    );
  }
});

[jordwalke]

@spicyj Thoughts?

[sebmarkbage]

CSS's .style gives you the provided value and not the actually rendered value. You get that from computed style. This seems to be pretty idiomatic. That when you provide a value that will later be flush, the default is to read back the last provided value, not the latest flush.

People have this intuition of setter/getter semantics anyway because of the naming of this.setState.

Inside the render phase, this.state currently changes meaning. It's the rendered state outside of render and then the pending state inside of render because it hasn't flushed yet and it's inconsistent with what has been shown on the screen and inconsistent with the current value of the children.

It's very inconvenient to do counters and anything that relies on sequence of actions without easy access to pending state. It's also a very common use case.

IMO, the case that @jordwalke states is a special case use case. You should avoid refs as much as you can, and do top-down flushes.

For these reasons I feel strongly that this is an awesome change. I'll test it out a bit and see if something breaks.

[jordwalke]

Inside the render phase, this.state currently changes meaning. It's the rendered state outside of render and then the pending state inside of render because it hasn't flushed yet and it's inconsistent with what has been shown on the screen and inconsistent with the current value of the children.

I think that this is a good argument, but the definition of state that I've held to is "the last state to be invoked with render". By that definition, the definition doesn't ever change. However, you may be suggesting that we can chose another definition of state, namely: "the next state that our subcomponents will observe via a transformation from our state to their props". With this proposed definition, it also never changes. I think everyone is 100% on board with having both the pending state and the previously flushed state accessible. We could give this proposal a shot if it doesn't break everyone's apps.

This is a breaking change in that now componentWillUpdate and shouldComponentUpdate need to look at this.currentState when comparing, not this.state. (Perhaps that's an argument towards including prevProps and prevState as arguments to those lifecycle methods...)

This is definitely concerning. It may justify us "swapping the defaults" so that pendingState is the pending state, and state is what was observed in the last render call. Either way - we should expose the pending state as this diff accomplishes.

[sebmarkbage]

Ugh. OO bites us again.

It's important that pendingState is always an object with at least the current state. It should never be null. Since it's too inconvenient to check for the existence. this.setState({ x: (this.pendingState || this.currentState).x + 1 })

this.state is currentState is way too easy to misinterpret and get wrong. IMO, it's the single thing preventing us from moving to more aggressive batching strategies. Leaving it as is, is not an option.

However, this case that @spicyj bring up in shouldComponentUpdate is also too easy to get wrong. Ideally, I'd take state and props as arguments to all methods (including render).

The first thing I do in shouldComponentUpdate is:

var oldProps = this.props;
var oldState = this.state;

If I could just add those as arguments I could get rid of that boilerplate.

[jordwalke]

I do agree that having them passed explicitly to those should* methods would be best. I even agree about having them passed as args to render, but we decided not to only because it wouldn't match event handlers (who access state via this.state etc.)

[vjeux]

Ping

[jordwalke]

@vjeux: Did you encounter another situation where this would be helpful?

[jordwalke]

Can someone document the trouble with shouldComponentUpdate in this diff with a little example (and how it could be resolved with adding extra args)? It's not a bad idea to take all four variables as arguments to shouldComponentUpdate which could help us push this change through.

@spicyj Do you have a good example of where this change would be useful to you?

[jordwalke]

One other little suggestion: The world currentState may not be clear enough. Some might think of "current" as being the state that was "currently" updated yet not flushed. Maybe renderedState is a little more clear?

[sophiebits]

This change isn't useful to me personally.

Right now we have

var ReactComponentWithPureRenderMixin = {
  shouldComponentUpdate: function(nextProps, nextState) {
    return !shallowEqual(this.props, nextProps) ||
           !shallowEqual(this.state, nextState);
  }
};

This would have to change to

var ReactComponentWithPureRenderMixin = {
  shouldComponentUpdate: function(nextProps, nextState) {
    return !shallowEqual(this.props, nextProps) ||
           !shallowEqual(this.currentState, nextState);
  }
};

or more likely,

var ReactComponentWithPureRenderMixin = {
  shouldComponentUpdate: function(prevProps, prevState, nextProps, nextState) {
    return !shallowEqual(prevProps, nextProps) ||
           !shallowEqual(prevState, nextState);
  }
};

if we change the signature.

[sebmarkbage]

componentWillUpdate has the same issue. Although it's rarely ever used with prevState (we only have two cases).

[sebmarkbage]

Note that shouldComponentUpdate also takes a context now. It might make sense to have the signature be:

shouldComponentUpdate(prevProps, nextProps, prevState, nextState, prevContext, nextContext)

That way you can exclude context (and state) if you don't depend on those.

[petehunt]

I had brought this up with @sebmarkbage a while ago -- what if we made the API more monadic? I believe this would fix the dirty write problem:

var Clicker = React.createClass({
  getInitialState: function() { return {count: 0}; },
  handleClick: function() {
    this.stateTransition(function(state) {
      return {count: state.count + 1};
    });
  },
  render: function() { return <div onClick={this.handleClick}>{this.state.count}</div>; }
});

I like this API because it's clear that this.state isn't being mutated directly; setState() does not get this message across as well. It also forbids you from calling setState() multiple times which is an antipattern. We'd pass pendingState as the parameter to the function.

I don't like this API because it encourages more function allocations (but I think this is solvable by just passing a method) and because people will accidentally reference this.state. One option could be: if you're in a transitionState() block, we set this.state to be pendingState.

[sophiebits]

My initial reaction is that stateTransition is too much line noise. Now whenever we want to do

this.setState({count: this.state.count + 1});

we need to write this?

this.stateTransition(function(state) {
  return {count: state.count + 1};
});

Feels painful to me.

[petehunt]

It's always possible to build some syntactic sugar on top:

var Clicker = React.createClass({
  getInitialState: function() { return {count: 0}; },
  handleClick: React.stateTransition(function(pendingState, e) {
    // or maybe we don't pass pendingState and instead use this.state
    return {count: pendingState.count + 1};
  }),
  render: function() { return <div onClick={this.handleClick}>{this.state.count}</div>; }
});

[sebmarkbage]

I'm not against trying to include a more monadic alternative as a secondary state setter. I think we may be able to make even more powerful so we need to think about how to get it right.

In the short term, I think this will be a political and refactoring nightmare. The write is often separated from the read by multiple mixins and intermediate methods. There can also be side-effects and callbacks intermixed. The only (so called) benefit with the OOP model is that you have easy access to props and state in all your helpers methods without explicitly passing them.

We need a quick fix, if we can, so that we can move forward with more rAF and what not.

[markijbema]

I'd like to give my two cents. I've been using react for a few weeks now, and only now learned on the mailinglist that this doesn't work, unless subsequent calls have a guaranteed update in between:

this.setState({count: this.state.count + 1});

I actually made a program which does something similar, with lots of updates, and I didn't notice it not working, so I guess this only happens in some corner cases, where updates are very frequent in relation to rendering frequency, but this makes it even worse. This makes that I won't encounter the bugs, but users of my app will sometimes encounter it, in a non-reproducible manner.

I think it might depend on how you do state management, but to me, the props of the children of my component have nothing to do with my state, other than being affected by it. I have a state, which defines what my app should look like (combined with the props), and I add handlers to my components which change the state. Therefore, whenever I reason about the state of the object, I can only consider the state and the props of that component. The result of render is in a sense merely a side-effect.

I think pendingState tells the story of what it is really bad (at least in how I perceive React). In my perception, pendingState is the actual state, and the 'state' is the laste-rendered-state, or something like that, which in my mind is a minor detail.

So I would really like renderedState and state instead of state and pendingState.

Conceptually I like the monadic solution, but as said, syntactically it's suboptimal.

[zpao]

@sebmarkbage @jordwalke @petehunt - it's been a couple months... any plans to take this further?

[sebmarkbage]

Probably not this particular PR but this discussion is valuable and the issue needs to be solved. The refs work will help eliminate one pending issue. Now we just have to figure out what to do with the shouldComponentUpdate API.

[sophiebits]

Yeah, I'll close this out.

[pedroteixeira]

Hi there, is there such 'this.pendingState' or is it just an idea? Couldn't find it in the docs. thanks.

[sebmarkbage]

It's just an idea.

On Apr 29, 2014, at 8:42 AM, Pedro Henriques dos Santos Teixeira notifications@github.com wrote:

Hi there, is there such 'this.pendingState' or is it just an idea? Couldn't find it in the docs. thanks.

—
Reply to this email directly or view it on GitHub.

[pedroteixeira]

hmm.. ok. Just saw how Om's decided to do it omcljs/om#31 -- perhaps, it might be of interest.

[syranide]

@pedroteixeira this._pendingState exists and while it's marked as internal and probably (hopefully?) will go away in the future, I've come across a situation or two where I've had to make use of it (or write overly tedious code) and will simply update my code when the time comes.

[dhruvbhatia]

@syranide Thanks a lot. I wasn't aware that existed - it solves the issue for the time being. 👍

[zpao]

@dhruvbhatia We'll break it one day and won't tell you, just remember that. If you can find another solution, I would suggest that. Bad practices get copied and then before you know it you're stuck on an old version of React.

[markijbema]

@zpao you could wrap it in a function, and make an integration test for that function as contract test for react. I would expect you are always able to access in some way both states, unless they are the same? Then people could cargocult all they want, and you'd only need to change one function. (I think the statetransation function would be a good one, as it makes the least assumptions about how react would implement this).