useSelect: improve transition from async to sync mode #40680
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
youknowriad
reviewed
Apr 28, 2022
| // initial render + rerender with isAsync=false + store state update | ||
| expect( TestComponent ).toHaveBeenCalledTimes( 3 ); | ||
| // initial render + rerender with isAsync=false | ||
| expect( TestComponent ).toHaveBeenCalledTimes( 2 ); |
There was a problem hiding this comment.
This sounds like a potential performance improvement.
There was a problem hiding this comment.
Unfortunately this happens only in a very rare condition where there actually was a pending store update when the component was switching from async to sync mode.
|
Size Change: -6 B (0%) Total Size: 1.23 MB
ℹ️ View Unchanged
|
youknowriad
approved these changes
Apr 28, 2022
bph
added
the
Needs Dev Note
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This PR refactors
useSelectcode that makes sure that when the component is switching from async to sync mode, then the state updates scheduled while still in async mode won't be forgotten.The old code achieved that by flushing the render queue during the transition, which means synchronously calling
onStoreChangeand reading the new state from stores.The new code reads the new state from stores, too, but does it by doing a
hasLeftAsyncModecheck and callingmapSelectif the check is true. It's an addition to the set of existing checks that trigger call ofmapSelect: change of registry, change of themapSelectfunction itself (and its dependencies), and failure on previous call.Why is this change for the better? Because it removes a
renderQueue.flushcall, and concentrates allrenderQueuereferences into the effect that maintains store subscriptions and calls theonStoreChangecallback. That makes it easier to extract that effect to a separate hook, i.e., finishing the work in #40093.How to test:
Covered by unit tests, namely the "catches updates while switching from async to sync" one. See also #19286 which describes the conditions where doing the async->sync transition incorrectly can lead to UI bugs.