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

Revert PR #6353 and support options.nextFetchPolicy instead. #6712

Merged
merged 2 commits into from
Jul 27, 2020

Conversation

benjamn
Copy link
Member

@benjamn benjamn commented Jul 27, 2020

PR #6353 seemed like a clever zero-configuration way to improve the default behavior of the cache-and-network and network-only fetch policies. Even though the word "network" is right there in the policy strings, it can be surprising (see #6305) to see network requests happening when you didn't expect them.

However, the wisdom of #6353 was contingent on this new behavior of falling back to cache-first not creating problems for anyone, and unfortunately that hope appears to have been overly optimistic/naive: #6677 (comment)

To keep everyone happy, I think we need to revert #6353 while providing an easy way to achieve the same effect, when that's what you want. The new options.nextFetchPolicy option allows updating options.fetchPolicy after the initial network request, without having to call observableQuery.setOptions. Specifically, passing { nextFetchPolicy: "cache-first" } for network-only or cache-and-network queries should restore the behavior of #6353.

This could be considered a breaking change, but it's also a regression from Apollo Client 2.x that needs fixing. We are confident options.nextFetchPolicy will enable the #6353 functionality where desired, and we are much more comfortable requiring the explicit use of options.nextFetchPolicy going forward.

PR #6353 seemed like a clever zero-configuration way to improve the
default behavior of the cache-and-network and network-only fetch policies.
Even though the word "network" is right there in the policy strings, it
can be surprising (see #6305) to see network requests happening when you
didn't expect them.

However, the wisdom of #6353 was contingent on this new behavior of
falling back to cache-first not creating problems for anyone, and
unfortunately that hope appears to have been overly optimistic/naive:
#6677 (comment)

To keep everyone happy, I think we need to revert #6353 while providing an
easy way to achieve the same effect, when that's what you want. The new
options.nextFetchPolicy option allows updating options.fetchPolicy after
the intial network request, without calling observableQuery.setOptions.

This could be considered a breaking change, but it's also a regression
from Apollo Client 2.x that needs fixing. We are confident
options.nextFetchPolicy will restore the #6353 functionality where
desired, and we are much more comfortable requiring the explicit use of
options.nextFetchPolicy in the future.
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.

Looks great @benjamn! Adding nextFetchPolicy to https://github.com/apollographql/apollo-client/blob/master/docs/shared/query-options.mdx would be helpful as well. Thanks!

@pleunv
Copy link
Contributor

pleunv commented Aug 9, 2020

Would it make sense to support nextFetchPolicy in the defaultOptions as well in order to easily get the old cache-and-network behavior back?

@sanketjo96
Copy link

sanketjo96 commented Aug 12, 2020

@benjamn Is there any other way to get the old cache-and-network behavior by setting global option ?

@benjamn
Copy link
Member Author

benjamn commented Aug 17, 2020

@sanketjo96 @pleunv There are some details to iron out (like #6839), but here's my basic plan for that: #6833 (comment)

@benjamn
Copy link
Member Author

benjamn commented Aug 25, 2020

@pleunv @sanketjo96 With @apollo/[email protected] (which includes #6893), it should be possible to configure nextFetchPolicy globally, using a function instead of just a string:

new ApolloClient({
  link,
  cache,
  defaultOptions: {
    watchQuery: {
      nextFetchPolicy(lastFetchPolicy) {
        if (lastFetchPolicy === "cache-and-network" ||
            lastFetchPolicy === "network-only") {
          return "cache-first";
        }
        return lastFetchPolicy;
      },
    },
  },
})

It's important to use a function here, so you're only modifying cache-and-network and network-only, not any other fetch policies. It would be weird/wrong to change cache-only to cache-first, for example.

Hope that helps!

@pleunv
Copy link
Contributor

pleunv commented Aug 25, 2020

@benjamn Awesome! Definitely does!

@aquelehugo
Copy link

@pleunv @sanketjo96 With @apollo/[email protected] (which includes #6893), it should be possible to configure nextFetchPolicy globally, using a function instead of just a string:

new ApolloClient({
  link,
  cache,
  defaultOptions: {
    watchQuery: {
      nextFetchPolicy(lastFetchPolicy) {
        if (lastFetchPolicy === "cache-and-network" ||
            lastFetchPolicy === "network-only") {
          return "cache-first";
        }
        return lastFetchPolicy;
      },
    },
  },
})

It's important to use a function here, so you're only modifying cache-and-network and network-only, not any other fetch policies. It would be weird/wrong to change cache-only to cache-first, for example.

Hope that helps!

@benjamn
I am trying to use it for defaultOptions.query and it is not available, defaultOptions.watchQuery seems to enable it anyway. Using version 3.2.7. Is it intended?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants