Recheck reactive variable's value in commit phase #7652
Merged
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
8e34eaa to
1d216a3
Compare
benjamn
approved these changes
Feb 4, 2021
There was a problem hiding this comment.
Makes sense to me (after our discussion). Thanks @jcreighton!
This was referenced Mar 11, 2021
benjamn
added a commit
that referenced
this pull request
May 4, 2021
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.
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.
Fixes #7609. Because #7581 amended
useReactiveVarto set theonNextChangehandler in a useEffect, an end user could see inconsistent data if updating a reactive var in a useEffect higher in the component tree. That change wouldn't be caught lower in the tree asonNextChangewouldn't be set yet. (useEffect is called during the commit phase and also in order from top to bottom.) This PR adds an additional check for a change in the value, scheduling an update if the value has changed.Checklist: