Skip to content
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

Fix defaultOptions.watchQuery.fetchPolicy not being used by useQuery #9210

Merged
merged 3 commits into from
Jan 10, 2022

Conversation

yasharzolmajdi
Copy link
Contributor

@yasharzolmajdi yasharzolmajdi commented Dec 16, 2021

This PR is intended to solve the issue of default options in client not setting the correct fetch policy in useQuery.

when user doesn't set a fetch policy option in useQuery, it is defaulted to cache-first before it attempts to use the default fetch policy that is set in by the client.

the fetch policy hierarchy currently is:

  1. user define fetch policy as parameter in useQuery
  2. default to cache-first in useQuery
    it completely ignores default fetch policy from the client

with this PR it will be:

  1. user define fetch policy as parameter in useQuery
  2. user define fetch policy as default value in client
  3. default to cache-first in useQuery

fixes #9105
fixes #2555

@apollo-cla
Copy link

@yasharzolmajdi: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/

@matrinox
Copy link

matrinox commented Jan 2, 2022

@yasharzolmajdi Tested this locally and it fixed it for me. The defaultOptions is used on the react hooks from 3.0.1 but I think somewhere between then and 3.5.6 (the one I'm using right now) it stopped using the defaults.

Copy link
Member

@hwillson hwillson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this @yasharzolmajdi! If you could add a test(s) to help verify this works as expected (and to help avoid regressions in the future), that would be helpful.

@yasharzolmajdi
Copy link
Contributor Author

Thanks for this @yasharzolmajdi! If you could add a test(s) to help verify this works as expected (and to help avoid regressions in the future), that would be helpful.

@hwillson Thanks, I have added a test which currently fails on the main branch but passes with my changes

Copy link
Member

@hwillson hwillson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the test @yasharzolmajdi - this looks great to me. @brainkim @benjamn any objections to merging?

Copy link
Contributor

@brainkim brainkim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this!

@benjamn benjamn changed the title Fix default options not being set for useQuery Fix defaultOptions not being used by useQuery Jan 10, 2022
Copy link
Member

@benjamn benjamn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, and I can confirm the test fails when the changes are reverted. Thanks @yasharzolmajdi!

@benjamn benjamn merged commit fcaa208 into apollographql:main Jan 10, 2022
@benjamn benjamn changed the title Fix defaultOptions not being used by useQuery Fix defaultOptions.watchQuery.fetchPolicy not being used by useQuery Jan 10, 2022
benjamn added a commit that referenced this pull request Feb 25, 2022
Follow-up/refinement to PR #9210.

Now that the createWatchQueryOptions function no longer defaults
watchQueryOptions.fetchPolicy to "cache-first" unconditionally (instead
defaulting to defaultOptions.watchQuery.fetchPolicy if defined), we can
let the merging of defaultOptions happen as it normally does, when
client.watchQuery(watchQueryOptions) is called later.
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

useQuery doesn't seem to use defaultOptions in 3.5 defaultOptions do not work
6 participants