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

Sync state #9

Closed
wants to merge 5 commits into from
Closed

Sync state #9

wants to merge 5 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Dec 15, 2017

Add a new method which will allow to synchronously do a set state, e.g.

this.setStateSync({val:x})

or replace old asynchronous method with this one.

more details in the RFC.

PS. I apologize, due to some circumstance change, I might not be able to keep an eye on RFC but I will still leave it here, maybe someone will make a new pull request out of it and keep working with that.

PPS. I plan to cancel my account and since I am not sure if that will also delete my pull request I will leave a copy of the RFC here too.

I apologize if I caused inconvenience.


Start Date: (2017-12-15)
RFC PR: (leave this empty)
React Issue: (leave this empty)

Summary
Add support for synchronous state in React: (1) Either replace the existing asynchronous setState method, or (2) Add a new function which should synchronously set state.

Basic example
this.setState({val: newVal}) // Now react should synchronously update state instead of asynchronously

or

this.setStateSync({val: newVal}) // Now react should synchronously update state instead of asynchronously

Motivation
The motivation is that it is much easier to reason about application state when the function which modifies state does this synchronously.

Nobody will disagree that react's asynchronous setState has caused lot of confusion among developers.

Even given the solution react has - like setState with a function(which gives access to a so called "pending state"), still doesn't prove that there can't be any gotchas (or edge cases) when managing state asynchronously.

Ask people why are they using mobx for example.

Drawbacks
Honestly I can't think of any drawbacks which would outweigh the advantages that synchronous setState would have.

Adoption strategy
If you directly change the existing setState with a synchronous one this seems like a breaking change, if you add a new synchronous setState function this doesn't seem to be a breaking change.

How we teach this
It will be much easier to teach about it than the existing asynchronous setState.

@ghost ghost changed the title New rfc Sync state Dec 15, 2017
@j-f1
Copy link

j-f1 commented Dec 15, 2017

Can you revert your first commit (a0a8563) please? I think they’d like to keep the template for future RFCs.

revert

This reverts commit cc3ec12.
@ghost
Copy link
Author

ghost commented Dec 15, 2017

@j-f1 what will happen with rfc doc I created then in that case? (I am not very seasoned in git)

@ghost
Copy link

ghost commented Dec 16, 2017

Async state one of the major performance boosts in the React, When I asked about this in the past my request was rejected from Dan Abramov by explaining to me how should I correctly update the state, then I didn't faced any problems with the state management

@kyleshevlin
Copy link

I'd like to push back on this RFC just a bit. Async state is a huge performance boost to React overall. A synchronous state setter would block the thread, and that's not something we really want to do. You may have many other components competing for resources at the same time and being able to defer computation is a big win.

I will agree that setState has been confusing for many people, but there are very few quirks in React to know about. I think knowing that setState is async and has two function signatures is an acceptable quirk to have. Learning how to use these two signatures appropriately is important for React devs and honestly something I believe every person using React should know. Personally, I'd rather improve education on the topic than create a third method and force people to understand all three.

Lastly, not many computations in setState should be synchronous in general. You should be able to write your functions in such a way as to derive your future computations based on the current state and what you wish to do. If, you need synchronicity, this is where the second argument callback is useful, as it will essentially act as a means of making the nextState available to you since we know it will have been computed by the time the callback is fired. If you are chaining many computations in the callbacks of setState, it may be a code smell. There is probably a better architecture available to solve your problems.

@ghost
Copy link
Author

ghost commented Dec 19, 2017

referring to setState callback which @kyleshevlin mentioned.
In this example


 constructor(props)
  {
   super(props);
   this.state={value:0}
  }
  
  click(){
    this.setState({value:1}, ()=>{console.log(this.state.val)})
     this.setState({value:2})
  }

Say you want to get value on console but are interested in that only when you assigned 1 to value - hence the callback. What will be output in your opinion of above code? answer: 2.

So is this not confusing that user might have thought that hey in setState I am assigning value 1, now I specify a function in callback which should fire when the value is set to 1. However, in reality when that function was called value is already 2.

Isn't that confusing?

Maybe those are the kind of things OP referred to.

@kyleshevlin
Copy link

@David-Jam I meant that you could nest another setState within your callback. The way you have written that, someone who knows and understands React would know that the objects { value: 1 } and { value: 2 } would be merged into one object before the update occurs.

@ghost
Copy link
Author

ghost commented Dec 19, 2017

@kyleshevlin then it is a bit confusing why tie a callback with a particular function invocation if that callback might not be called when the function change takes effect e.g. like with
this.setState({value:1}, ()=>{console.log(this.state.val)})
I am just saying naively a user might be expecting hey, I called set state and this is callback I want to fire when the value I specified in set state is set. otherwise why not just use componentDidUpdate?

@TrySound
Copy link
Contributor

@David-Jam Using setState callback should not be naive. It's advanced feature which is not required in regular code (I used it twice in large applications).

@ghost
Copy link
Author

ghost commented Dec 19, 2017

@TrySound so you say it is not possible to arrive at a gotcha situation (e.g. situation or needs where it would be difficult to architect your app as to manage state as you want) with current approach of async react state management? (that you would not arrive with a sync state). I am asking out of curiosity not criticizing someone or smth.

@TrySound
Copy link
Contributor

TrySound commented Dec 19, 2017

@David-Jam I wouldn't say it's difficult, however it requires some knowledge about how API works. That's I meant as "not naive". Just follow rules and nobody will get hurt.

@ghost
Copy link
Author

ghost commented Dec 20, 2017

@TrySound even if you know how API works I think it is beyond doubt that it is easier to manage app state when that is synchronous.

@TrySound
Copy link
Contributor

Easier !== better. Programming is hard AFAIK. But if you know things you CAN work with them.

@ghost
Copy link
Author

ghost commented Dec 20, 2017

@TrySound of course easier is better in this case, why introduce unnecessary complexity? Anyway we are heading in a different direction now so I will finish of my discussion here with this comment.
I don't have specific example which would show that in some situation using async set state could cause bug - or would not be suitable for some situation. If such cases don't exist, good for react. But I am not sure.

@TrySound
Copy link
Contributor

Here's the thing. It's necessary complexity for future react features.

@taion
Copy link
Member

taion commented Jan 7, 2018

The author of this RFC is apparently now a @ghost. Does this have any bearing on the RFC process?

@gaearon
Copy link
Member

gaearon commented Jan 7, 2018

It shouldn’t affect the initial review but if we don’t reject it and ask for a revisions then we’d need somebody to take over (assuming the author doesn’t want to).

@TrySound
Copy link
Contributor

TrySound commented Jan 7, 2018

@gaearon I guess there's nobody agreed with the author since there's a proposal from you and other react maintainers about async rendering.

@gaearon
Copy link
Member

gaearon commented Jan 24, 2018

I wrote a bit about why React works this way: facebook/react#11527 (comment)

@theKashey
Copy link

theKashey commented Jan 25, 2018

We have used something like React and something like Redux, but with sync API. What was wrong?

  • You are calling setState,
  • then it will call render with the new props,
  • then children's componentWillReceiveProps(or mount/unmount) dispatches an event,
  • then a parent of the initial component will propagate new props down, reflecting the change,
  • then you may call setState once again. Why not?

Sooner or later executing point will reach the end of the initial setState call, and run some code after it. The only problem - you just time-travelled back and doing something inside a component which does not reflect the reality. And you may face 2-3-4-5 past versions of yourself. Loopers, you know.
That is the first rule of a time traveller - don't meet yourself, or the universe will collapse.

So please don't destroy the universe, and use async version of a setState. And let the Event Horizon protect you.

@gaearon
Copy link
Member

gaearon commented Feb 10, 2020

To sum this up:

  1. React state is intentionally set asynchronously: RFClarification: why is setState asynchronous? facebook/react#11527 (comment). This is part of the reason React is fast enough for most use cases. We don't want to regress on that.
  2. For corner cases where you need to force a sync render due to some browser API, there is ReactDOM.flushSync(() => { /* call setState here */ }).
  3. Regardless, with Hooks setting state synchronously still wouldn’t update the captured values, negating the original motivation.

Therefore, closing.

@gaearon gaearon closed this Feb 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants