-
Notifications
You must be signed in to change notification settings - Fork 50.4k
Adding optional callback argument to setState
#93
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
Conversation
Change api docs to reflect presence of the new argument. In addition, callback was change to require only a "truthy" value.
Small problem with markdown syntax in syntax-highlighted block.
|
👍 |
src/core/ReactCompositeComponent.js
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should probably be flowed through as a 2nd arg to this.replaceState, another function accessible by client code and where the ultimate work of generalizing this will go.
All public facing {set,replace,force}{props,state} methods now support
callbacks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if we put these in ReactCompositeComponent._performComponentUpdate() instead? That way we won't have to copy-paste it everywhere, and it'll be closer to where the batching logic will eventually live.
|
A simple test case here would be great too, since it's unlikely this will have many callsites in prod code for a while it may break and go unnoticed without one. |
|
Will add unit tests. About putting it _performComponentUpdate(), that's where I originally was trying to put it, but I see a small problem with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
De-dupe this with the preceding comment paragraph? Could just delete the first sentence and smash them together.
Preserve state when switching tabs
We eventually want to batch calls to
setStatefor performance reasons, and this no longer guarantees immediate, synchronous execution of these calls tosetState. As such, this commit adds an optional callback parameter so that actions can be taken once the call actually runs.@phunt says we shouldn't make the synchronous operation guarantee and @jordow agrees.