Pending state updates may be confusing about react HOT 43 CLOSED

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.

from react.

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
  });
} ...

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

from react.

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.

from react.

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.

from react.

@azoerb's checkAlarm example can be rewritten as:

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

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 });
    };

}

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

from react.

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?

from react.

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

from react.

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.

from react.

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)

from react.

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

from react.

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.

from react.

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);
          }
        }
      )
    );
  }
}

I ran into the same problem as @azoerb

from react.

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

from react.

@th0r forceUpdate will bypass shouldComponentUpdate

from react.

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.

from react.

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.

from react.

@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?

from react.

@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.

from react.

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

from react.

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.

from react.

@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.

from react.

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.

from react.

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.

from react.

Yes, to clarify:

This is okay:

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

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

But this is preferable:

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

Take it to the mailing list please guys

from react.

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.

from react.

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;

@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.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

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

from react.

@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.

from react.

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.

from react.

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
  }));
}

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

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.

from react.

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();
},

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

@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);
},

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?

from react.

@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.

from react.

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}));