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

Pending state updates may be confusing #122

Closed
sophiebits opened this Issue Jun 23, 2013 · 43 comments

Comments

Projects
None yet
@sophiebits
Member

sophiebits commented Jun 23, 2013

If you have

componentWillReceiveProps: function() {
    this.setState({x: this.state.x + 1});
    this.setState({x: this.state.x + 1});
}

then x will only be incremented once because the new state will just be stored in _pendingState until receiveProps finishes. After my refactoring in #115, this will never increment twice, which is probably confusing.

Maybe this isn't a bug. We can fix this behavior by instead updating this.state immediately (but still updating the UI later). Right now I can't think of any situations where having this.state out of sync with the UI is a problem.

@vjeux suggested I open an issue, so here I am. Probably easiest for me to fix while updating #115 though.

@jordwalke

This comment has been minimized.

Show comment
Hide comment
@jordwalke

jordwalke Jun 24, 2013

Member

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.

Member

jordwalke commented Jun 24, 2013

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.

@sophiebits

This comment has been minimized.

Show comment
Hide comment
@sophiebits

sophiebits Jun 24, 2013

Member

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.

Member

sophiebits commented Jun 24, 2013

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.

@sophiebits

This comment has been minimized.

Show comment
Hide comment
@sophiebits

sophiebits Jun 28, 2013

Member

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

Member

sophiebits commented Jun 28, 2013

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

@zpao

This comment has been minimized.

Show comment
Hide comment
@zpao

zpao Aug 14, 2013

Member

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?

Member

zpao commented Aug 14, 2013

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

This comment has been minimized.

Show comment
Hide comment
@sophiebits

sophiebits Aug 14, 2013

Member

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

Member

sophiebits commented Aug 14, 2013

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

@jordwalke

This comment has been minimized.

Show comment
Hide comment
@jordwalke

jordwalke Aug 14, 2013

Member

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.

Member

jordwalke commented Aug 14, 2013

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.

sophiebits added a commit to sophiebits/react that referenced this issue Dec 4, 2013

Store pending state updates directly in this.state
...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...)

Fixes #122.
@sebmarkbage

This comment has been minimized.

Show comment
Hide comment
@sebmarkbage

sebmarkbage Apr 29, 2014

Member

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.

Member

sebmarkbage commented Apr 29, 2014

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

This comment has been minimized.

Show comment
Hide comment
@dustingetz

dustingetz May 22, 2014

Contributor

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)

Contributor

dustingetz commented May 22, 2014

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

This comment has been minimized.

Show comment
Hide comment
@dustingetz

dustingetz Jul 8, 2014

Contributor

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

Contributor

dustingetz commented Jul 8, 2014

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

@azoerb

This comment has been minimized.

Show comment
Hide comment
@azoerb

azoerb Aug 4, 2014

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.

azoerb commented Aug 4, 2014

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.

@nambrot

This comment has been minimized.

Show comment
Hide comment
@nambrot

nambrot Aug 5, 2014

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.

nambrot commented Aug 5, 2014

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

This comment has been minimized.

Show comment
Hide comment
@azoerb

azoerb Aug 6, 2014

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.

azoerb commented Aug 6, 2014

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

This comment has been minimized.

Show comment
Hide comment
@abergs

abergs Aug 7, 2014

I ran into the same problem as @azoerb

abergs commented Aug 7, 2014

I ran into the same problem as @azoerb

@th0r

This comment has been minimized.

Show comment
Hide comment
@th0r

th0r Sep 10, 2014

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

th0r commented Sep 10, 2014

+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

This comment has been minimized.

Show comment
Hide comment
@zpao

zpao Sep 13, 2014

Member

@th0r forceUpdate will bypass shouldComponentUpdate

Member

zpao commented Sep 13, 2014

@th0r forceUpdate will bypass shouldComponentUpdate

@BurntCaramel

This comment has been minimized.

Show comment
Hide comment
@BurntCaramel

BurntCaramel Sep 23, 2014

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.

BurntCaramel commented Sep 23, 2014

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

This comment has been minimized.

Show comment
Hide comment
@briandipalma

briandipalma Sep 25, 2014

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.

briandipalma commented Sep 25, 2014

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

This comment has been minimized.

Show comment
Hide comment
@ptomasroos

ptomasroos Oct 13, 2014

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

ptomasroos commented Oct 13, 2014

@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

This comment has been minimized.

Show comment
Hide comment
@briandipalma

briandipalma Oct 13, 2014

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

briandipalma commented Oct 13, 2014

@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

This comment has been minimized.

Show comment
Hide comment
@ptomasroos

ptomasroos Oct 13, 2014

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

ptomasroos commented Oct 13, 2014

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

@aldendaniels

This comment has been minimized.

Show comment
Hide comment
@aldendaniels

aldendaniels Feb 2, 2015

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.

aldendaniels commented Feb 2, 2015

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

This comment has been minimized.

Show comment
Hide comment
@leebyron

leebyron Feb 2, 2015

Contributor

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

Contributor

leebyron commented Feb 2, 2015

@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

This comment has been minimized.

Show comment
Hide comment
@aldendaniels

aldendaniels Feb 2, 2015

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.

aldendaniels commented Feb 2, 2015

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

This comment has been minimized.

Show comment
Hide comment
@sebmarkbage

sebmarkbage Feb 2, 2015

Member

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.

Member

sebmarkbage commented Feb 2, 2015

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

This comment has been minimized.

Show comment
Hide comment
@leebyron

leebyron Feb 2, 2015

Contributor

Yes, to clarify:

This is okay:

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

This is not okay:

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

leebyron commented Feb 2, 2015

Yes, to clarify:

This is okay:

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

This is not okay:

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

This comment has been minimized.

Show comment
Hide comment
@leebyron

leebyron Feb 2, 2015

Contributor

But this is preferable:

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

leebyron commented Feb 2, 2015

But this is preferable:

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

This comment has been minimized.

Show comment
Hide comment
@dustingetz

dustingetz Feb 2, 2015

Contributor

Take it to the mailing list please guys

Contributor

dustingetz commented Feb 2, 2015

Take it to the mailing list please guys

@SystemParadox

This comment has been minimized.

Show comment
Hide comment
@SystemParadox

SystemParadox Feb 4, 2015

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.

SystemParadox commented Feb 4, 2015

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

This comment has been minimized.

Show comment
Hide comment
@markmarijnissen

markmarijnissen Mar 16, 2015

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)

markmarijnissen commented Mar 16, 2015

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

This comment has been minimized.

Show comment
Hide comment
@SystemParadox

SystemParadox Mar 16, 2015

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

SystemParadox commented Mar 16, 2015

@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

This comment has been minimized.

Show comment
Hide comment
@syranide

syranide Mar 16, 2015

Contributor

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

Contributor

syranide commented Mar 16, 2015

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

@SystemParadox

This comment has been minimized.

Show comment
Hide comment
@SystemParadox

SystemParadox Mar 16, 2015

@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 commented Mar 16, 2015

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

@sebmarkbage

This comment has been minimized.

Show comment
Hide comment
@sebmarkbage

sebmarkbage Mar 16, 2015

Member

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

Member

sebmarkbage commented Mar 16, 2015

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

@SystemParadox

This comment has been minimized.

Show comment
Hide comment
@SystemParadox

SystemParadox Mar 16, 2015

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.

SystemParadox commented Mar 16, 2015

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

This comment has been minimized.

Show comment
Hide comment
@sebmarkbage

sebmarkbage Mar 16, 2015

Member

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.

Member

sebmarkbage commented Mar 16, 2015

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

This comment has been minimized.

Show comment
Hide comment
@dubrowgn

dubrowgn May 20, 2015

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.

dubrowgn commented May 20, 2015

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.

ianobermiller added a commit to FormidableLabs/radium that referenced this issue Jul 13, 2015

Fix setStyleState regression in b1fa309
Oh React. If you call `setState` twice during the same execution frame,
the second time, `this.state` will be the same as the first time. Thus,
the second time will clobber the changes made the first time. This is a
known issue: facebook/react#122

Btw, I noticed this when running the examples and trying the one with
all the boxes; mouseLeave on one box gets called in the same frame as
mouseEnter on another, and since all the boxes are in one component,
they clobbered each others state, and everyone stayed hovered. Too lazy
to write a proper test for this right now.
@SystemParadox

This comment has been minimized.

Show comment
Hide comment
@SystemParadox

SystemParadox Oct 5, 2015

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.

SystemParadox commented Oct 5, 2015

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

This comment has been minimized.

Show comment
Hide comment
@jimfb

jimfb Oct 5, 2015

Contributor

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

Contributor

jimfb commented Oct 5, 2015

@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

This comment has been minimized.

Show comment
Hide comment
@sophiebits

sophiebits Oct 5, 2015

Member

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?

Member

sophiebits commented Oct 5, 2015

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

This comment has been minimized.

Show comment
Hide comment
@SystemParadox

SystemParadox Oct 7, 2015

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

SystemParadox commented Oct 7, 2015

@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

This comment has been minimized.

Show comment
Hide comment
@sophiebits

sophiebits Dec 16, 2015

Member

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

sophiebits commented Dec 16, 2015

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

@sophiebits sophiebits closed this Dec 16, 2015

@arcanis

This comment has been minimized.

Show comment
Hide comment
@arcanis

arcanis Mar 14, 2016

Member

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

}
Member

arcanis commented Mar 14, 2016

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

}
@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Jan 24, 2018

Member

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

Member

gaearon commented Jan 24, 2018

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

@kdnk kdnk referenced this issue May 17, 2018

Closed

frontend night #3 #3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment