Recheck rv() against previous value in useEffect callback in useReactiveVar
#8135
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
While rereading the code @jcreighton added in #7652, I remembered it's safe to call rv() in a useEffect callback (without useReactiveVar), which allows us to compare the latest variable value to the previous value before deciding whether to call rv.onNextChange(setValue) in the effect callback function. If the values already disagree, we can skip listening and just call setValue right away. In React <StrictMode>, the two initial rv() values generated prior to the useIsomorphicEffect callback should always be the same, unless the reactive variable somehow gets updated in between those renders, which I don't think is possible, since React invokes those renders sychronously, one after the other: https://github.com/facebook/react/blob/0e100ed00fb52cfd107db1d1081ef18fe4b9167f/packages/react-reconciler/src/ReactUpdateQueue.new.js#L391-L399 The key difference between this approach and calling rv.onNextChange synchronously in render is that (in this new approach) we can ignore the extra value without doing anything to clean up its resources, whereas with rv.onNextChange we were also responsible for calling the extra cancel function produced by the extra rv.onNextChange call.
rv() against previous value in useEffect callback in useReactiveVar
|
Hurts my brain but the tests pass! Nice to cut down the comments in that hook. |
brainkim
approved these changes
May 4, 2021
brainkim
reviewed
May 5, 2021
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
While rereading the code @jcreighton added in #7652, I remembered it's safe to access a reactive variable
rvby callingrv()in auseEffectcallback (without going throughuseReactiveVar).Taking advantage of this insight, we should be able to compare the latest variable value to the previous
valuebefore deciding whether to callrv.onNextChange(setValue)in the effect callback function, if/when that callback fires. If the values already disagree, we can skip listening and just callsetValueright away.In React
<StrictMode>, there will be two initialrv()values generated prior to theuseIsomorphicEffectcallback (which may never fire), but those values should always be the same, unless the reactive variable somehow gets updated in between the double renders. I don't think that's possible, since React invokes those renders synchronously, one after the other.The key difference between this approach and calling
rv.onNextChangesynchronously inrenderis that (in this new approach) we can safely ignore the extravaluewithout doing anything to clean up its resources, whereas callingrv.onNextChangeinrendermade us additionally responsible for calling the extracancelfunction returned by the extrarv.onNextChangecall, which was not easy/possible.If I'm on the right track here, then we finally have a way to "listen" for reactive variable changes that happen before the effect callback fires, even if that callback is noticeably delayed.