bonham000 / fcc-react-tests-module

The original freeCodeCamp React/Redux alpha curriculum.
http://hysterical-amusement.surge.sh/
BSD 3-Clause "New" or "Revised" License
77 stars 38 forks source link

React setState #174

Closed Vozf closed 7 years ago

Vozf commented 7 years ago

When I started building real react app after completing your module it took me a lot of time to understand that setState is async and even more time to determine that i shouldn't have multiple successive calls to it To my mind it should be highlighted or even has it's own exercise especially about it's second form (https://kevinmccarthy.org/2015/07/05/multiple-async-callbacks-updating-state-in-react/ ) and optional second argument

bonham000 commented 7 years ago

Interesting, could you give me an example of how this has been problematic for you? This would help me better understand how to try and add some content around this. In that blog post he is also talking about performing set state after asynchronous actions, which of course requires an understanding of the asynchronous actions first. Also, you typically would not directly modify state, as in his line:

let newState = this.state.results.push(result)

That can create problems for React. My understanding of what he is trying to do (multiple async calls at once) is that it would be better handled with a functionality like Promise.all, and you would only update React's state when all the promises resolve.

Vozf commented 7 years ago

Yeah that's obviously incorrect but I did really look on how he invoked setState rather than what he did inside So I had something like this

    //call to api1 

   this.setState(Object.assign({},this.state,api,()=>{
      console.log(this.state); //shows that only next setState was performed and the this one is ignored
   });

    //call to api2 

    this.setState(Object.assign({},this.state,api2);
    //call to api1 

  this.setState((oldState)=>Object.assign({},oldState,api1));

    //call to api2 

     this.setState((oldState)=>Object.assign({},oldState,api2));
//Everything ok
bonham000 commented 7 years ago

So in that code snippet, I think what would happen is that this.state is a reference to the React state. So in the first block if api2 resolved and set the state before api1 resolved, then when api1 resolved this.state would reference React state, which is now filled with the result from api2.

If, instead, in api1 you first made a copy of the current state, then you could be confident that if you console.log that state it will represent the current state at the time that code initially runs, not when the api call resolves (if the state at that time has been updated elsewhere). So I don't think that this problem here is necessarily a result of React's setState method being asynchronous.

I think it may be good to make a mention of setState's asynchronous behavior (as explained here decently) somewhere in the challenges. In practice, I've almost never run into a situation where this actually caused a problem (I've never written setState using that alternative syntax). In short, you would have to try to set the same values in state multiple times in a method call, which typically wouldn't make a lot of sense. Multiple async actions is a good example of an exception to this, but again this would more properly be handled with one state update after all the promises resolve.

I still think it would be worth making a mention of this.

Does that all make sense?

Vozf commented 7 years ago

Can't understand what you mean in 2nd paragraph. may be you can try to explain on this codepen.I reversed my fix for now.All you need is the App's constructor and componentDidMount. Everything else is irrelevant http://codepen.io/Vozman/pen/JEBQJq

bonham000 commented 7 years ago

What I mean is that in your first block of code this.state is just a reference to the state.

So if this.state was 5, and then you ran an api call that updated the state to 10, the reference this.state in the api1 call would be 10.

So it should be expected that this.state shows the result of api2, provided that api2 has resolved and already set the state at the time api1 resolves.

Vozf commented 7 years ago

No,that's not actually what I meant.I have 2 independent properties in state for each api.So there should be no collision.Easier to look at the example in my previous answer

bonham000 commented 7 years ago

So take a look at this CodePen I just made which illustrates calling two APIs that resolve at different times and setting state within both with the result. Each API updates a different property in state and there are no problems with writing the code this way in terms of state being updated properly.

bonham000 commented 7 years ago

I did just push an update to Challenge 24 which now includes a note on the possibility of setState calls being asynchronous!

Vozf commented 7 years ago

I understand that it works with some time between calls to setState.But in my codepen they are following practically without delay and (as I understand) this causes that only one of 2 calls actually made changes to state

Vozf commented 7 years ago

Eh sorry,I've understood what i was doing wrong.I thought that in react we should fully copy our state and then call to setState with that copy(like in Redux with object.assign), when it setstate is merging instead of replacing I tried to fully copy the state when I should have only passed the changed part of state. Async nature may not be noted as I thought, as it was my problem,so consider reversing those changes you made if you did them to satisfy me :) What should be noted is that in Redux you have to make a copy and in React you can pass only the changed part of the object(redux is replacing,react is merging) Sorry again If you still don't understand what I meant I'll write an example

bonham000 commented 7 years ago

Ok, that makes sense and yes setState being asynchronous has nothing to do with this problem here. What was happening is you were just overwriting the entire state with the Object.assign.

But you definitely are supposed to copy state in React and then update the state with the new version — what I mean is you are definitely not supposed to modify state directly, for instance with something like:

this.state.counter++

or

this.state.users.push(newUser)

In that last example, you would instead copy the state and then update, like so:

var users = this.state.users.slice();
users.push(newUser);
this.setState({ users });

Hopefully, that makes sense. I will leave the changes because I think it's still good to mention this sidenote about setState asynchrony. Going to close this now!