Pending state updates may be confusing

[facebook]

Pending state updates may be confusing

[jordwalke]

It might be best to think of setState as being enqueueStateUpdate. With that understanding, much of that confusion seems to go away. I have been pondering the exact same thing as you - should we let you "read" the state updates immediately after you write them, even if the UI has not been synchronized with it? At first I thought we should let you read the update immediately. But then I realized something: Not allowing reads of state after enqueueing the updates has an interesting effect. It encourage you to aggregate the entire update payload into a single object and execute a single setState. Since mutations are the hardest things in the world to understand, it makes sense to encourage squeezing them into small, centralized parts of your code. There's a theory that it's better to have all the crap in one pile, as opposed to spreading it out all over your code. Depending on how you feel about mutations, that stance might apply here. I haven't thought enough about this to feel strongly in either way, but I thought I'd share my recent thoughts - I've gone back and forth on this one.

[azoerb]

I just ran into this problem as well.

setAlarmTime: function(time) {
  this.setState({ alarmTime: time });
  this.checkAlarm();
},
checkAlarm: function() {
  this.setState({
    alarmSet: this.state.alarmTime > 0 && this.state.elapsedTime < this.state.alarmTime
  });
} ...

When calling setAlarmTime, I assumed that this.state.alarmTime would have been updated before the call to checkAlarm. It seems a bit ridiculous to have to manually track the actual state of alarmTime in order to be able to correctly call checkAlarm, and this is really making me worry about how often my "state" is actually correct now. Batch the state updates behind the scenes for rendering, sure, but don't make me have to keep track of what state is up-to-date!

Please fix this!

EDIT: So I can get around it in this case by placing the call to this.checkAlarm in the callback on setState, but there are other cases where the callback workaround isn't as simple / clean, and it just still seems counter-intuitive to have to do this.

[gaearon]

Wrote some thoughts about why React works this way: #11527 (comment)

[sophiebits]

Agree that isolating mutations to one place is good, but it sounds like you're suggesting making state-setting more awkward just to discourage it, which doesn't make sense to me.

Will continue to think about this as I finish up the setState batching. My gut feeling right now is that updating this.state immediately may simplify our code as well as being less confusing to the user – I'll see if that turns out to be the case.

[jordwalke]

One issue with updating this.state, even though you haven't flushed it accordingly, is that your system (React) is not consistent with itself. So for example, if you have a ref="myThing", and it accepts props that should be influenced by your state, this.refs.myThing.answerQuestion() returns an answer that does not take into account your state, but this.answerMyOwnQuestion() does take into account the change in this.state. I think we talked about adding a setStateSync() method for the times when you really want it to flush - though you don't get the performance benefit of your diff.

[sebmarkbage]

@azoerb's checkAlarm example can be rewritten as:

checkAlarm: function() {
  this.setState(state => ({
    alarmSet: state.alarmTime > 0 && state.elapsedTime < state.alarmTime
  }));
}

One of the most important performance improvements that React introduced was a way to avoid read/write thrash through batching using a declarative update model that doesn't depend on the order of execution. This solidifies part of the behavior that made React fast enough to begin with.

The mental model should be that state is part of the return value. You can introduce mutable data structures that allow you to mutate things as you go, but non-local mutable state is very difficult to reason about at scale so we would recommend that you use the functional approach.

This scheduling work is also part of what makes React performant and how we can make it even more performant.

[arcanis]

Transactional updates are a thing, but what if we only need to read the actual value (without actually writing into it)? For example, in the following pseudo-component I listen on some sort of global event listener to toggle a "selectable" internal flag. However, I can't trust this field to be actually set in the onClickHandler call, which means that as far as I know, I either have to manually keep track of the current state (which seems like a quite nasty way to solve this issue), or wrap the code inside a dummy setState call that wouldn't do anything (and that doesn't seem really elegant either).

Don't you think that, should redefining this.state as the pending state be too BC-breaking, it could still be possible to add an official way to get access to the pending state under another name, to improve this use case? this.pendingState, maybe?

Not being able to trust this.state anywhere else than in a render call feels like a waste ;(

class Foo extends Component {

    static contextTypes = {
        globalEmitter : React.PropTypes.object
    };

    componentDidMount() {
        this.context.globalEmitter.on('selectstart', this.onSelectStart);
        this.context.globalEmitter.on('selectstop', this.onSelectStop);
    }

    componentWillUnmount() {
        this.context.globalEmitter.off('selectstart', this.onSelectStart);
        this.context.globalEmitter.off('selectstop', this.onSelectStop);
    }

    render() { return (
        <button onClick={this.onClickHandler} />
    ); }

    onClickHandler = () => {
        if (!this.state.selectable)
            return ;
        this.props.onSelected();
    };

    onSelectStart = () => {
        this.setState({ selectable: true });
    };

    onSelectStop = () => {
        this.setState({ selectable: false });
    };

}

[sophiebits]

Unfortunately this looks hard to change due to the arguments that the component lifecycle methods take.

[zpao]

Any more concerns about this? I'm guessing this is still an issue, though I'm not sure how often people are hitting it.

I think another option would be to simply restrict you to a single setState call at certain points in the lifecycle. We'd count in __DEV__, have invariants as needed, and then reset at the right times. Does that make sense?

[sophiebits]

Until we hear more, I think it's fine to close this out. I haven't heard of anyone actually getting confused.

[sebmarkbage]

If you include closures in your render function as callbacks you can have similar problems. Likewise refs can be inconsistent, but also props if your parent doesn't flush down to you before your callback is invoked.

Deferred reconciliation (batching) puts the whole system in an inconsistent state and to do this correctly you really have to reconcile anything that you may have dependencies on during your side-effectful operation.

It turns out that this is a much more complex issue that this.state alone.

[dustingetz]

I think I have made progress on the double setState problem using a cursor-based approach. http://jsfiddle.net/dustingetz/SFGU5/

(Past discussion is also here: #629)

[dustingetz]

If anyone is paying attention, here is an improved cursor example that I consider idiomatic, no double setState problem, and uses === to implement shouldComponentUpdate
https://github.com/dustingetz/wingspan-cursor/blob/master/examples/helloworld/webapp/js/Page.js

[nambrot]

I'm also currently having the issue where I'm not really sure what this.state refers too. I agree that this.state should ideally refer to the latest state, regardless of whether it's visible in the UI.

[azoerb]

Edit: also check out Douglas' answer on this StackOverflow question as that may be enough to solve your problems without this workaround.

@nambrot if you're looking for a quick fix, what I ended up doing is creating a Utility function which wraps React.createClass and creates a separate version of the state that will always be up-to-date:

var Utils = new function() {
  this.createClass = function(object) {
    return React.createClass(
      $.extend(
        object,
        {
          _state: object.getInitialState ? object.getInitialState() : {},
          _setState: function(newState) {
            $.extend(this._state, newState);
            this.setState(newState);
          }
        }
      )
    );
  }
}

then just change your React.createClass(obj) calls to Utils.createClass(obj) and replace all calls to this.state with this._state and this.setState with this._setState. It's not super optimal, but unfortunately I was getting errors when trying to do a direct swap of setState with my new function.

You will also lose the generated displayName property (when using jsx they transform var anotherComponent = React.createClass({ into var anotherComponent = React.createClass({displayName: 'anotherComponent'),
so you'll just have to define it yourself if you want the correct tag names to show up in the react tools.

Disclaimer: this isn't super well tested, so let me know if you have any problems with it.

[abergs]

I ran into the same problem as @azoerb

[th0r]

+1, the same problem, as @azoerb
My current hack is to update this.state directly and call this.forceUpdate later. Can it break something?

[zpao]

@th0r forceUpdate will bypass shouldComponentUpdate

[RoyalIcing]

Could there just be a queuing function like changeState (or queueStateChange) that takes a closure that is queued? Your closure then returns the changed state to be merged. The closure could even be passed the current state as an argument possibly.

[briandipalma]

The other workaround is to move your state out of React and into flux stores. As long as you keep state of out React then you shouldn't have this problem.

[ptomasroos]

@briandipalma Can you explore this?
And just have everything passed through props and set no state within the react view?

I thought a common convention was to setState when flux stores change otherwise there will be a hard time getting the change to render from the event handler?

[briandipalma]

@ptomasroos Yes and the setState call is passed the state provided by the store, the React component doesn't calculate itself based on this.state.

[ptomasroos]

@briandipalma Good then we agree! Just wanted to be explicit. 👍

[aldendaniels]

DISCLAIMER: I'm new to React.

If I understand correctly, React's setState() method's only advantage over directly modifying component state and calling forceUpdate() is that setState() lets the developer avoid unnecessary re-renders by overloading shouldComponentUpdate().

For shouldComponentUpdate() to be effective, however, we need to use immutable data structures (e.g. ClosureScript + OM, React Immutability Helpers, ImmutableJS, etc.). Without immutability, shouldComponentUpdate() can't detect changes.

In fact, we often CAN'T use setState() when using mutable data. You can't remove an item from a list using setState() unless you've shallow cloned the whole list (think immutability).

Takeaways

1. If you're using immutable data structures, then use setState() and enjoy the confidence that this.state and this.refs are always in sync.
2. If you're using mutable data structures, modify this.state directly and call forceUpdate(). You'll have to live with this.state and this.refs being out of sync. On the bright side, you'll get to write VanillaJS update code and won't run into the confusion detailed in this issue.

Again - I'm new to React. Please clarify if I've got something wrong.

[leebyron]

@aldendaniels I think you've mixed two concepts.

You're 100% correct about PureRenderMixin's implementation of shouldComponentUpdate(). It is only safe if your props and state use immutable values (including string, number, bool).

However independent of any shouldComponentUpdate implementation, setState is still perfectly safe to use, even if you're using mutable data in your props and state.

In fact, you should never mutate this.state directly, as that is considered immutable.

[aldendaniels]

Hi @leebyron - thanks for reaching out.

you should never mutate this.state directly, as that is considered immutable

I'm a little dubious: from the React docs - "React lets you use whatever style of data management you want, including mutation". I also see that in Facebook's official TodoMVC Flux Example, state is not being treated as immutable.

Using immutable state in JavaScript without using a 3rd party library like those listed in my previous post quickly becomes slow and tedious when dealing with large, nested data sets.

If mutable state really is anti-pattern, then I'm all ears - but I'm also concerned by the invasiveness of the requirement.

[sebmarkbage]

There is a difference between this.state.foo = mutation; and this.state.nestedObject.foo = mutation;. The later is still accepted. The former is a bit of a gray area but is considered an anti-pattern since that object is considered "owned" by React.

[leebyron]

Yes, to clarify:

This is okay:

this.state.myArray.push(newValue);
this.forceUpdate();

This is not okay:

this.state.myArray = newArrayValue;
this.forceUpdate();

[leebyron]

But this is preferable:

var pushedArray = this.state.myArray;
pushedArray.push(newValue);
this.setState({ myArray: pushedArray });

[dustingetz]

Take it to the mailing list please guys

[SystemParadox]

This should definitely be changed to update this.state immediately. The current behaviour is not only confusing- it forces us to pass the new state around to achieve sensible results.

[markmarijnissen]

Hey everybody,

Check out Meteor's implementation of their Tracker.

They queue all state changes and flush on a requestAnimationFrame, unless you explicitly call Tracker.flush() to proces all pending changes.

if you do

bob_bank_account -= 10;
alice_bank_account += 10;

you will never end up with an invalid state (i.e. 10$ appearing or dissapearing out of nowhere). Either all changes are pending, or they are all resolved (using Tracker.flush, or at the end of the event cycle).

So, to summarize:

• setState should read as enqueueStateChange
• Add a method to process queue if you want to force the state changes immediatly (for example when you need in-between results (updated state) for you calculations)

[SystemParadox]

@markmarijnissen, your example works the same in react:

this.setState({ bob_account: this.state.bob_account - 10 });
this.setState({ alice_account: this.state.alice_account + 10 });

This specific example will work correctly. BUT, this is not advised because react will currently give you broken results if there are any other state changes in between time.

The problem is that reading from this.state doesn't give you the result you expect whilst there are pending updates. Therefore, this example is broken:

this.setState({ bob_account: this.state.bob_account + 10 });
this.setState({ bob_account: this.state.bob_account + 10 });
// when state resolves, bob_account is only +10, not +20 like it should have been

setState should read as enqueueStateChange

No. Whilst more accurate to the current behaviour, this is not useful. We want this.state to be changed to the pending state.

[syranide]

@markmarijnissen @SystemParadox, setState can take a function as of React 0.13 and solves the problem of pending state.

[SystemParadox]

@syranide, that doesn't solve @azoerb's example, which is 99% of the problem.

I must say I am very disappointed with this change to setState in React 0.13. It doesn't solve the problem and just further solidifies the current behaviour.

[SystemParadox]

Oh I see. Thank you for the example.

I am aware that React batches rendering (this of course is why we have pending state at all), but I don't understand why the component API forces us to work with this all the time.

As @spicyj said at the beginning, why not update this.state immediately and leave the UI to update with its batching mechanism later? Anything that would be affected by this belongs in componentDidUpdate anyway.

[sebmarkbage]

That would only be possible with a synchronous flushing mechanism like @markmarijnissen proposed.

Take the check alarm example. It is safe for me to move elapsedTime to props like this:

checkAlarm: function() {
  this.setState((state, props) => ({
    alarmSet: state.alarmTime > 0 && props.elapsedTime < state.alarmTime
  }));
}

However, it is not safe in the case state is synchronously updated, but props are not. E.g. this is not safe even if state is up-to-date:

this.setState({
  alarmSet: this.state.alarmTime > 0 && this.props.elapsedTime < this.state.alarmTime
});

You would have to introduce a synchronous this.flush(); to be able to read from props synchronously.

Now if you have two of these in the same subtree, you will have one unnecessary rerender of the entire tree. If you have 15 of these updates in the same subtree you have 14 unnecessary rerenders.

[dubrowgn]

I think the part of this that makes it inherently confusing is that there are many consumers of state, not just render. Flushing state mutations on frame animation effectively means state has to know about render, even though render is supposed to be a pure function, taking state as one of its inputs. Render's output always represents a snapshot in time anyway, so mutating state at frame animation or all along the way with each state update shouldn't affect render at all.

As it stands, state mutation and render are tied together, which isn't immediately obvious to anyone who hasn't read the docs. I would also argue that it's much less useful to have them tied together.

[SystemParadox]

For myself and anyone else who keeps bumping into this...

When the state change is asynchronous, like this:

getData: function () {
    var self = this;
    var params = {
        foo: this.state.foo,
        bar: this.state.bar,
    };
    getSomeDataAsynchronously(params, function (data) {
        self.setState({ data: data });
    });
},

handleFooChange: function (value) {
    this.setState({ foo: value });
    this.getData();
},

handleBarChange: function (value) {
    this.setState({ bar: value });
    this.getData();
},

In this case, reading the state in getData ends up with the previous state. The correct solution is to use the SECOND callback argument to setState, to run the update after you've finished changing the state:

handleFooChange: function (value) {
    this.setState({ foo: value }, this.getData);
},

Of course, this waits for a re-render, which isn't really what we wanted here. What we wanted to do was change the state and fire off the async call in the same turn, but React doesn't let us do this.

I would love to know if there is a better solution.

It also doesn't make sense to me that the change handler functions have to deal with this - why can't getData just use the pending state?

I still don't understand the rationale for this situation. @sebmarkbage suggested something about props but I don't see how that is an issue (perhaps I am missing the context, a complete example might help). If anything it's possibly another example of the same confusing behaviour.

[jimfb]

@SystemParadox

getData: function (params) {
    var self = this;
    getSomeDataAsynchronously(params, function (data) {
        self.setState({ data: data });
    });
},

handleFooChange: function (value) {
    this.setState({ foo: value });
    var params = {
        foo: value,
        bar: this.state.bar,
    };
    this.getData(params);
},

handleBarChange: function (value) {
    this.setState({ bar: value });
    var params = {
        foo: this.state.foo,
        bar: value,
    };
    this.getData(params);
},

Does that do what you want?

I agree it's a slightly ugly hack/workaround, but assuming I'm understanding your question correctly, that should do what you want.

[sophiebits]

What we wanted to do was change the state and fire off the async call in the same turn

Is there a reason this is effectively different from doing them separately?

[SystemParadox]

@jimfb, that works, but duplicating the parameter handling is worse than having to pass the 'correct' state object each time.

@spicyj, arguably it doesn't make a lot of practical difference, but it's harder to reason about and it separates the async call from the handler that caused it, which can hinder debugging. If there's no reason to complicate things by separating them with an extra turn, why are we doing it?

I think it would really help this discussion to see a full example of a time when changing this.state to be the pending state would also lead to unexpected behaviour.

[sophiebits]

Closing this out because we're not going to change anything, at least with our current component syntax. The recommended way to do transactional updates is:

    this.setState((state) => ({x: state.x + 1}));
    this.setState((state) => ({x: state.x + 1}));