Include watch.callback in maybeBroadcastWatch cache keys. #5747
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
Fixes #5733, which was caused by multiple watches having the same cache key, so none except the first were ever re-broadcast, since the first broadcast had the side effect of marking the later watches as clean, before this.watches.forEach had a chance to visit them. Background: PR #5644 reenabled an important optimization for the InMemoryCache#broadcastWatches method, allowing it to skip watches whose results have not changed. Unfortunately, while this optimization correctly determined whether the result had changed, it did not account for the possibility of multiple distinct consumers of the same result, which can happen (for example) when multiple components use the same query and variables via different useQuery calls. Fortunately, the fix is straightforward (if not exactly obvious): in order to assign distinct consumers different cache keys, it suffices to include the provided watch.callback function in the cache key. A more drastic way to fix #5733 would be to remove the caching of maybeBroadcastWatch entirely, which would not be a huge loss because the underlying cache.diff method is also cached. For now, though, with that backup option in mind, I'd like to preserve this optimization unless it causes any further problems.
benjamn
force-pushed
the
issue-5733-segment-maybeBroadcastWatch-by-callback
branch
from
726c0a1
to
16405a6
Compare
benjamn
force-pushed
the
issue-5733-segment-maybeBroadcastWatch-by-callback
branch
from
16405a6
to
1c57170
Compare
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 #5733, which was caused by multiple watches having the same cache key, so none except the first were ever re-broadcast, since the first broadcast had the side effect of marking the later watches as clean, before
this.watches.forEachhad a chance to visit them.Background: PR #5644 reenabled an important optimization for the
InMemoryCache#broadcastWatchesmethod, allowing it to skip watches whose results have not changed. Unfortunately, while this optimization correctly determined whether the result had changed, it did not account for the possibility of multiple distinct consumers of the same result, which can happen (for example) when multiple components use the same query and variables via differentuseQuerycalls.Fortunately, the fix is straightforward (if not exactly obvious): in order to assign distinct consumers different cache keys, it suffices to include the provided
watch.callbackfunction in the cache key.A more drastic way to fix #5733 would be to remove the caching of
maybeBroadcastWatchentirely, which would not be a huge loss because the underlyingcache.diffmethod is also cached. For now, though, with that backup option in mind, I'd like to preserve this optimization unless it causes any further problems.