Coder Social home page Coder Social logo

Comments (43)

jordwalke avatar jordwalke commented on February 28, 2024 6

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.

azoerb avatar azoerb commented on February 28, 2024 3

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.

from react.

gaearon avatar gaearon commented on February 28, 2024 3

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

from react.

sophiebits avatar sophiebits commented on February 28, 2024 1

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.

jordwalke avatar jordwalke commented on February 28, 2024 1

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.

sebmarkbage avatar sebmarkbage commented on February 28, 2024 1

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

from react.

arcanis avatar arcanis commented on February 28, 2024 1

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

}

from react.

sophiebits avatar sophiebits commented on February 28, 2024

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

from react.

zpao avatar zpao commented on February 28, 2024

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.

sophiebits avatar sophiebits commented on February 28, 2024

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

from react.

sebmarkbage avatar sebmarkbage commented on February 28, 2024

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.

dustingetz avatar dustingetz commented on February 28, 2024

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.

dustingetz avatar dustingetz commented on February 28, 2024

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.

nambrot avatar nambrot commented on February 28, 2024

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.

azoerb avatar azoerb commented on February 28, 2024

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.

from react.

abergs avatar abergs commented on February 28, 2024

I ran into the same problem as @azoerb

from react.

th0r avatar th0r commented on February 28, 2024

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

zpao avatar zpao commented on February 28, 2024

@th0r forceUpdate will bypass shouldComponentUpdate

from react.

RoyalIcing avatar RoyalIcing commented on February 28, 2024

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.

briandipalma avatar briandipalma commented on February 28, 2024

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.

ptomasroos avatar ptomasroos commented on February 28, 2024

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

briandipalma avatar briandipalma commented on February 28, 2024

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

ptomasroos avatar ptomasroos commented on February 28, 2024

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

from react.

aldendaniels avatar aldendaniels commented on February 28, 2024

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.

leebyron avatar leebyron commented on February 28, 2024

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

aldendaniels avatar aldendaniels commented on February 28, 2024

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.

sebmarkbage avatar sebmarkbage commented on February 28, 2024

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.

leebyron avatar leebyron commented on February 28, 2024

Yes, to clarify:

This is okay:

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

This is not okay:

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

from react.

leebyron avatar leebyron commented on February 28, 2024

But this is preferable:

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

from react.

dustingetz avatar dustingetz commented on February 28, 2024

Take it to the mailing list please guys

from react.

SystemParadox avatar SystemParadox commented on February 28, 2024

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.

markmarijnissen avatar markmarijnissen commented on February 28, 2024

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)

from react.

SystemParadox avatar SystemParadox commented on February 28, 2024

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

from react.

syranide avatar syranide commented on February 28, 2024

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

from react.

SystemParadox avatar SystemParadox commented on February 28, 2024

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

SystemParadox avatar SystemParadox commented on February 28, 2024

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.

sebmarkbage avatar sebmarkbage commented on February 28, 2024

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.

from react.

dubrowgn avatar dubrowgn commented on February 28, 2024

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.

SystemParadox avatar SystemParadox commented on February 28, 2024

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.

from react.

jimfb avatar jimfb commented on February 28, 2024

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

from react.

sophiebits avatar sophiebits commented on February 28, 2024

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.

SystemParadox avatar SystemParadox commented on February 28, 2024

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

sophiebits avatar sophiebits commented on February 28, 2024

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

from react.

Related Issues (20)

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. 📊📈🎉

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❤️ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.