-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Drastically simplify useQuery
default options processing
#9665
Merged
benjamn
merged 2 commits into
main
from
issue-9635-useQuery-stuck-in-standby-with-defaultOptions
May 3, 2022
Merged
Drastically simplify useQuery
default options processing
#9665
benjamn
merged 2 commits into
main
from
issue-9635-useQuery-stuck-in-standby-with-defaultOptions
May 3, 2022
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
Should fix #9635 and related issues.
benjamn
added
🐞 bug
🚨 high-priority
📟 regression
⚛️ React-related
🥚 backwards-compatible
for PRs that do not introduce any breaking changes
👩🏭 refactor
labels
May 3, 2022
benjamn
commented
May 3, 2022
Comment on lines
+371
to
+377
|| this.client.watchQuery(mergeOptions( | ||
// Any options.defaultOptions passed to useQuery serve as default | ||
// options because we use them only here, when first creating the | ||
// ObservableQuery by calling client.watchQuery. | ||
this.queryHookOptions.defaultOptions, | ||
this.watchQueryOptions, | ||
)); |
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 is the crux of the new strategy, allowing all the other simplifications.
hwillson
approved these changes
May 3, 2022
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.
Awesome - thanks @benjamn!
benjamn
deleted the
issue-9635-useQuery-stuck-in-standby-with-defaultOptions
branch
May 3, 2022 14:49
1 task
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Labels
🥚 backwards-compatible
for PRs that do not introduce any breaking changes
🐞 bug
🧞♂️ enhancement
🚨 high-priority
⚛️ React-related
👩🏭 refactor
📟 regression
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.
When implementing the new
useQuery(query, { defaultOptions })
functionality in #9563, I fell under the (thankfully mistaken) impression that I needed to handle all defaulting ofWatchQueryOptions
within thecreateWatchQueryOptions
method of theInternalState
class thatuseQuery
uses internally.This PR demonstrates we can avoid that computation entirely, and rely instead on existing defaulting mechanisms (in
watchQuery
) to do the same job. The trick is to use thedefaultOptions
fromuseQuery(query, { defaultOptions })
only when we first create theObservableQuery
(that is, only when we callclient.watchQuery
). That way, thedefaultOptions
are only relevant once, and do not continue to interfere withWatchQueryOptions
(re)computation after that.This simplification means we no longer need to consider the "latest" values of default options in
createWatchQueryOptions
, because we no longer consider default options at all increateWatchQueryOptions
. More generally, at the point in time when we providedefaultOptions
(once, when callingclient.watchQuery
), we have not yet created theObservableQuery
, so there are no other option values to worry about, latest or otherwise.By contrast, my previous code went to unnecessary lengths to give precedence to
this.observable.options
whenever there was a default option of the same name. Unfortunately, that trick (or at least the way I implemented it) did not work for thefetchPolicy
option in conjunction withskip
, sinceskip: true
setsthis.observable.options.fetchPolicy = "standby"
, and we do not want to recycle thatstandby
value (even though it is technically the "latest" value) when we switch back toskip: false
. Instead, we want the original/initialfetchPolicy
to be restored at that point. Fixing this problem fixes the reproduction in #9635.Should fix the following issues: