Skip to content
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

Not clear how setState in this case works #9439

Closed
ghost opened this issue Apr 15, 2017 · 30 comments
Closed

Not clear how setState in this case works #9439

ghost opened this issue Apr 15, 2017 · 30 comments

Comments

@ghost
Copy link

ghost commented Apr 15, 2017

Docs don't make following clear.

Imagine I have


this.setState({
counter:1
});
this.setState((prevState, props) => ({
  console.log(prevState.counter)
}));
this.setState({
counter:2
});
this.setState((prevState, props) => ({
  console.log(prevState.counter)
}));

What will be output above?

also in this case

this.setState({
counter:1
});
this.setState({
counter:2
});
this.setState((prevState, props) => ({
  console.log(prevState.counter)
}));

What will be output now?

@bytetwin
Copy link
Contributor

@giorgim

Think of setState() as a request rather than an immediate command to update the component. For better perceived performance, React may delay it, and then update several components in a single pass. React does not guarantee that the state changes are applied immediately.

Assuming React is enqueuing the state changes and executing them, they will be executed in order.

1st example output -1 , 2

2nd example output - 2

@ghost
Copy link
Author

ghost commented Apr 15, 2017

@hanumanthan So if I want to ensure I always keep track of the latest value in state, each setState should be followed by a read operation like: setState(function(prevState){...log(prevState.val...})?

Also maybe it is time react puts a doc explaining all the pitfalls one can encounter with managing state.

@gaearon
Copy link
Collaborator

gaearon commented Apr 15, 2017

Why do you need to “keep track of the latest value in state”? It would be easier to answer if you explained the use case in more detail. Right now it’s confusing why you call setState just to read it. Generally you shouldn’t ever need to read the state like this: since you’re the one who set it, you already have the value 😉 .

@gaearon
Copy link
Collaborator

gaearon commented Apr 15, 2017

In other words: this sounds like an abstract question that is hard to answer without confusing you further. It would be much easier to answer if you provided a snippet of real product code where you’re trying to solve a specific problem.

@gaearon
Copy link
Collaborator

gaearon commented Apr 15, 2017

The simplest answer to your question is: if you want to keep track of the latest value in the state, you can log it in componentDidUpdate:

componentDidUpdate() {
  console.log('state is', this.state);
}

@ghost
Copy link
Author

ghost commented Apr 15, 2017

@gaearon Thanks I will think about it. Btw. asynchronicity of setState I have seen has confused many developers, was making setState async worth it? maybe it is better if it is synchronous? For example this:http://stackoverflow.com/questions/43428456/how-to-correctly-handle-state-in-this-case, I would be grateful if you can respond on that question.

@gaearon
Copy link
Collaborator

gaearon commented Apr 15, 2017

asynchronicity of setState I have seen has confused many developers, was making setState async worth it?

Yes, it’s totally worth it, especially in bigger apps. We should improve the API to make it less confusing in the future, but definitely not go back to synchronous updates.

For example this:http://stackoverflow.com/questions/43428456/how-to-correctly-handle-state-in-this-case, I would be grateful if you can respond on that question.

I looked at your question but the way you schedule a timeout inside the updater function is confusing to me. Updater functions should be “pure”, that is, they should only describe how the state changes. You’re not supposed to trigger any side effects in them (like setting timeouts), or mutate parts of the state (like you do with .shown long after you exited the setState callback).

The way you keep track of an async operation with this.inProcess is also confusing to me. What if event fires too soon and several cells set this flag? I think this code will show bugs in this case. Not because of React APIs, but because it relies on wrong assumptions (e.g. that two cells can’t be clicked faster than one second).

It would be easier to answer this question if you created a small example (e.g. on CodePen or JSBin) and explained what exactly the code is supposed to do. It is likely there’s a simpler way to satisfy the requirements, and model the state in a different way. But it’s hard to help when the code example is too abstract, and the line between the original requirements and the implementation is not clear.

@gaearon
Copy link
Collaborator

gaearon commented Apr 15, 2017

Ah, I see, yes. Sorry I missed that. I guess I’m confused by the overall flow of the code. It would really help to know how you want it to work, and I could then write a more React-y version.

@ghost
Copy link
Author

ghost commented Apr 15, 2017

@gaearon Dan, how can many cells set inProcess flag, in the beginning of function you can see I am checking it and quiting the function if it is set.

@gaearon
Copy link
Collaborator

gaearon commented Apr 15, 2017

^^ see my reply above 😉

@ghost
Copy link
Author

ghost commented Apr 15, 2017

@gaearon So now you think this is OKAY? PS. this is code from my project: https://github.com/giorgim/MatchingGame. the timeout there I need for UI effect.

@gaearon
Copy link
Collaborator

gaearon commented Apr 15, 2017

I think the confusing part here is that you’re both setting the state and want to set a timeout based on that state (and in the timeout, set the state again).

I think that if you split the timeout-related part of this logic into the componentDidUpdate lifecycle hook, the code could be easier to understand. There might also be a better way to model the state itself. The matching game seems like a “state machine” with a few different valid states (nothing selected, one item selected and waiting, two right items selected, two wrong items selected).

It might be worth encoding these possible game states more directly into your component state and think more carefully about how to represent them with objects. For example, it might be that instead of an array of cell values it is easier to think about an explicit state like:

{
  openedCells: [1, 2], // array of ids
  firstSelectedCell: 5, // could be null
  secondSelectedCell: 7, // could be null
}

and then implement conditional logic in componentDidUpdate, e.g.

handleClick(e) {
  // Are we waiting for a timeout? Reset it.
  if (this.resetTimeout) {
    clearTimeout(this.resetTimeout);
  }

  const id = ... // get it from target node, or bind event handler to ID in render()
  this.setState(prevState => {
    if (prevState.firstSelectedCell !== null && prevState.secondSelectedCell === null) {
      // There is just one selected cell. We clicked on the second one.
      return {
        secondSelectedCell: id
      };
    }
    // We are selecting the first cell
    // (either because we clicked to reset both or because none were selected).
    return {
      firstSelectedCell: id,
      secondSelectedCell: null
    };
}

componentDidUpdate(prevState) {
  if (prevState.secondSelectedCell !== this.state.secondSelectedCell) {
    // We just picked the second cell.
    if (isSamePicture(
      this.state.secondSelectedCell,
      this.state.firstSelectedCell
    ) {
      // Same picture! Keep them open.
      this.setState(prevState => {
         // Add them both to opened cells and reset.
         return {
           firstSelectedCell: null,
           secondSelectedCell: null,
           openedCells: [
             ...prevState.openedCells,
             prevState.firstSelectedCell,
             prevState.secondSelectedCell
           ]
         };
    } else {
      // Clear both in a second.
      this.resetTimeout = setTimeout(() => {
        this.setState({
          firstSelectedCell: null,
          secondSelectedCell: null,
        });
      }, 1000);
    }
}

Then, in the render method, you can show cells if either they are in openedCells or they are firstSelectedCell or secondSelectedCell.

I omitted the details but I hope this gives you something to get started with, and hopefully provides a view into a more idiomatic React approach to this.

@ghost
Copy link
Author

ghost commented Apr 16, 2017

@gaearon Thanks I will think about your suggestion. I think since my code works (it doesn't have any obvious bugs, does it?) at least I may leave as it is and incorporate your suggestion maybe in future projects.

@gaearon
Copy link
Collaborator

gaearon commented Apr 16, 2017

Yea sure the original code looks okay (before you started using the updater form of setState). At least in current version of React it will work fine.

The way you were using the updater form is wrong (as I said, you shouldn't trigger side effects from it, and also shouldn't reference parts of the state and later mutate them independently). So I'd just suggest sticking to the simpler setState version with objects in this case.

In today's version of React the difference only matters if setState is called multiple times during the event handler. That's when the updater form works better. In your case it's only triggered once so I wouldn't bother using it there. But if you do decide to use it, please make sure the function you pass is pure. I hope the approach I suggested shows how to do it.

@gaearon gaearon closed this as completed Apr 16, 2017
@ghost
Copy link
Author

ghost commented Apr 16, 2017

@gaearon "before you started using the updater form of setState" what do you mean here Dan? I got confused in your last message at which times to which part of my code you refer to :)

PS.I think it is time Facebook puts out official doc named smth like "state management in react" where it explains all the pitfalls and best practices about state management, given so much confusion about it on the net

@gaearon
Copy link
Collaborator

gaearon commented Apr 16, 2017

I meant that the code in your GitHub project that just calls setState with object made more sense to me than the one in StackOveflow question.

Using the function version of setState is generally preferred but only if you use it right (it must be pure). Otherwise it complicates the code without any benefits.

PS Everything we have to say on this topic is already in the docs. Please check "State and Lifecycle" 😉. The setState functional overload is a little confusing and we'll be working on that, but it's solved with better APIs rather than documentation. We keep this in mind.

@ghost
Copy link
Author

ghost commented Apr 16, 2017

@gaearon If you look at my SO question, there are two versions, so you say 1st makes more sense? But in that case can't I theoretically run into case that where I clone the state (third or fourth statement) in clickHandler, I can get old state? (because some previous setState-s were pending?)

@gaearon
Copy link
Collaborator

gaearon commented Apr 16, 2017

Not in the current version of React, no. If we're inside an event handler then this.state is the current state. Now, if you call setState after that, this.state won't get updated immediately.

@ghost
Copy link
Author

ghost commented Apr 16, 2017

"Now, if you call setState after that, this.state won't get updated immediately."

-- where could be that "after that"?

PS. So you endorse first version of code which was included in my SO question, if you give an answer on Stack Overflow about that I will accept that - it may also help others.

@gaearon
Copy link
Collaborator

gaearon commented Apr 16, 2017

@ghost
Copy link
Author

ghost commented Apr 18, 2017

@gaearon Dan how you explain that here: http://stackoverflow.com/a/42994068/3963067, in the first code snippet (click first "show code snippet") by the author, inside increment, when he reads state, it reads old state (because value gets incremented only once)? even though increment is an eventHandler.

Because this seems to contradict with what you put on SO answer

At least in React 16 (or earlier), reading this.state before the first setState() call in the event handler will give you the current state. But don’t expect it to be updated immediately after setState().

@bytetwin
Copy link
Contributor

reading this.state before the first setState() call in the event handler will give you the current state.

In the code snippet, there are 2 event handlers called simultaneously, this.state in 1st event handler gets the correct value whereas this.state in 2nd event handler is read after setState have been called. This is inline with the above answer as it says this.state before the first setState will have correct value @giorgim

@ghost
Copy link
Author

ghost commented Apr 18, 2017

@hanumanthan , @gaearon Not really. Citing

No. At the time of this reply (React 16 and any earlier versions), this.state in the event handler is safe to read

The second time the event handler is called, we are entering a brand new event handler, so what Dan said above should hold.

Of course after I read state in event handler at some point I will modify it, and if when event handler is called again, what dan said above doesn't hold, then the question and answer also doesn't make much sense. because in that case it appears that is is safe to read from event handler only once. Isn't it?

@ghost
Copy link
Author

ghost commented Apr 18, 2017

@hanumanthan @gaearon Here is another citation form dan which makes us doubt what he meant in above quote:

You only call setState() once in your event handler (timeout happens later), so the pitfall about multiple consecutive calls doesn’t apply.

However as we saw with that code snippet, where you clicked + button, despite two independent event handlers were called, the state went out of sync (e.g. they weren't called consecutively as dan says above in the same event handler - and still went out of sync).

Would be nice if dan can clear all this up.

@ghost
Copy link
Author

ghost commented Jun 23, 2017

@gaearon Dan, say I am inside some function, how can I get current version of state? Assume I just want to get current version of state - just to read it, not modify.

@gaearon
Copy link
Collaborator

gaearon commented Jun 24, 2017

this.state gives you the currently rendered state.
We don't expose the pending state (it might not even be calculated by that point).

There's some discussion about this in #122.
cc @spicyj @sebmarkbage if they have thoughts.

Particularly, #122 (comment).

@ghost
Copy link
Author

ghost commented Jun 25, 2017

@gaearon Dan can you say why below code prints: 0, 2, 4 on the console (see console.log in render) if I click button twice, instead of 0,1,2,3,4?

class GoodCounter extends React.Component{
  constructor(props){
    super(props);
    this.state = {count : 0} 
    this.incrementCount = this.incrementCount.bind(this)
  }
  incrementCount(){
   this.setState((prevState, props) => ({
      count: prevState.count + 1
    }));
   this.setState((prevState, props) => ({
      count: prevState.count + 1
    }));
  }
  render(){
    console.log(this.state.count)
    return <div>
              <button onClick={this.incrementCount}>Increment</button>
          </div>
  }
}

It seems in this case the prevState you receive in the functional setState is not always the state rendered on the UI

@InternetExplorer7
Copy link

I assume you see that you're setting state twice in your incrementCount function, therefore increasing count by a magnitude of 2 rather than 1 each time that function is called.

React doesn't actually re-render until event-handling is completed, which is why setState can be called twice in your function before re-rendering.

@ghost
Copy link
Author

ghost commented Jun 25, 2017

@InternetExplorer7

React doesn't actually re-render until event-handling is completed, which is why setState can be called twice in your function before re-rendering.

Yeah for a moment one could think react would call render, after first call to functional set state. apparently it doesn't.

@gaearon
Copy link
Collaborator

gaearon commented Jan 24, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants