fetchMore and fetchPolicy: cache-and-network does not work together

[alex-golubtsov]

Intended outcome:
In case of fetchMore additional request should not be executed or should use last variables (passed to fetchMore)

Actual outcome:
The request executes with wrong variables

How to reproduce the issue:

function MyComponent() {
  const q = useQuery(query, { variables: { offset: 0 }, fetchPolicy: 'cache-and-network' });
  ...
  <button id="button" onClick={() => q.fetchMore({ variables: { offset: 10 } })}>Click me</button>
}

That's what is happening:

1. <MyComponent /> is rendered
   => request to the server is executed with { offset: 0 }
2. Button #button clicked
   => request to the server is executed with { offset: 10 }
   => request to the server is executed with { offset: 0 }

In other words the request, which fetches fresh data from the server uses variables passed to the hook, but to to fetchMore

Versions

  System:
    OS: Windows 10 10.0.19041
  Binaries:
    Node: 12.18.0
    Yarn: 1.22.4
    npm: 6.14.4
  Browsers:
    Chrome: 84.0.4147.125
  npmPackages:
    @apollo/client: ~3.1.2 => 3.1.3
    apollo: ~2.30.2 => 2.30.2

[DanielMarkiel]

@alex-golubtsov I believe you're describing a related issue to the one mentioned here -> #6907

Addition from myself:
The same issue goes for the network-only policy. The only one that is working properly is cache-first.
useQuery / useLazyQuery -> same effect.

[khitrenovich]

See also #6327 and #6833, especially this comment.

Looks like the proper policy combination in 2021 for paging+cache is

    fetchPolicy: 'cache-and-network',
    nextFetchPolicy: 'cache-first',

[fracmak]

Any reason why that isn’t the default setting?

[khitrenovich]

My guess would be that the system is too flexible for "one size fits all" defaults.
You can make it default for your own setup using the recipe from this comment.

[Akryum]

Having the same issue in Apollo Client 3.3.13
Using fetchPolicy: 'network-only' and notifyOnNetworkStatusChange: true, calling fetchMore will trigger an unwanted additionnal network fetch which will mess up with the type policy merge.

I can't use nexFetchPolicy since I'm using network-only. Please send help 🏳️

[Akryum]

Reproduction: https://codesandbox.io/s/apolloclient-beta-fetchmore-bug-forked-8mplu?file=/src/App.js

Click multiple times on the load more button:

Indeed apollo client does a "phantom" request after the fetch more one:

[Akryum]

Note: still an issue in Apollo Client 3.4.0-beta.19

[ivands]

Any update on people trying to fix this???

[Akryum]

I tried again on the codesandbox with 3.4.0-rc.23. The issue is kinda still here on the network side: the second phantom/unnecessary request with first: 1 is still made, but it doesn't call the merge (so it doesn't break pagination).
So the bug halfway-fixed! 🔧

[Akryum]

Seems it's still not working on my real project with 3.4.0-rc.23. Still having two merges with the second one being the initial query before the fetchMore. Apollo still "reverts" the pagination. 😭

[Akryum]

Digging into the source code...

The unwanted fetch seems to be caused by this line:

apollo-client/src/core/ObservableQuery.ts

Line 333 in 336337d

this.reobserve();

Which in turn calls this with the initial variables/options:

apollo-client/src/core/Reobserver.ts

Line 52 in 336337d

const concast = this.fetch(this.options, newNetworkStatus);

On 3.4 the lines are all in ObservableQuery.ts but the logic is the same:

apollo-client/src/core/ObservableQuery.ts

Line 383 in 7856c18

this.reobserve();

And the fetch:

apollo-client/src/core/ObservableQuery.ts

Line 658 in 7856c18

const concast = this.fetch(options, newNetworkStatus);

Not sure what a concast is though.

[Akryum]

If I remove the reobserve from fetchMore, there is another one called as a side-effect of this writeQuery:

apollo-client/src/core/ObservableQuery.ts

Line 372 in 7856c18

this.queryManager.cache.writeQuery({

This writeQuery call will lead to setDiff being called as well, but more importantly notify:

apollo-client/src/core/QueryInfo.ts

Line 205 in 7856c18

this.notifyTimeout = setTimeout(() => this.notify(), 0);

This then calls a reobserve too, causing a new request with the initial variables/options to be sent:

apollo-client/src/core/QueryInfo.ts

Line 233 in 7856c18

oq.reobserve();

Then the result from fetchMore is reverted by the new fetch.

[Akryum]

There is probably an architectural issue if a notify() causes a full new fetch?

[brainkim]

Maybe related? #8541

[rcbevans]

I've just spent ~3.5h debugging my code, then Apollo, investigating what appears to be the same issue as described, in a lazy loading list view.

Fetch more is correctly fetching and merging the next page of data, but a second request without the pagination token was then being sent and replacing the data in merge, causing the page content to reset and reset the scrollbar to the end, tiggering a refetch of the second page again, repeating infinitely

@khitrenovich suggestion to set

    fetchPolicy: 'cache-and-network',
    nextFetchPolicy: 'cache-first',

appears to suppress the second call and things now seem to work as expected.

What's frustrating is this query is only used in one place, so I'm not sure what exactly is subscribed to be notified which could be triggering this behavior.

[Akryum]

@khitrenovich suggestion to set

    fetchPolicy: 'cache-and-network',
    nextFetchPolicy: 'cache-first',

appears to suppress the second call and things now seem to work as expected.

Unfortunately this trick doesn't work with fetchPolicy: 'network-only' 😭

[lrojas94]

I can also confirm this behavior, but I can't use the workaround. I really need cache-and-network to trigger as soon as my variables change (every time the variables change). It seems the fetchPolicy + nextFetchPolicy doesn't really do it for me :/ Is this planned at all? seems like it's been more than a year

[benjamn]

After thinking more about this, I agree with @Akryum the extra network fetch after fetchMore is not a reasonable consequence of using network-only or cache-and-network fetch policies.

Yes, you may be able to work around the problem with nextFetchPolicy: "cache-first", but I no longer think using nextFetchPolicy should be necessary in this case. As long as the cache read triggered by fetchMore is complete (in the diff.complete sense), I think we should deliver that cache result immediately, and skip the network. In other words, reacting to a fetchMore request is not one of the situations where we want/need to reapply the original query's fetchPolicy literally, so I think it should be safe to ignore the fetchPolicy here, and just deliver the result.

I'm still investigating the consequences of these changes, but I think/hope we can remove the this.reobserve() from the finally after fetchMore, and change the QueryInfo code as follows:

commit 5c378d1d07a33ab4de0c14d2f6d68d48297ef9ec
Author: Ben Newman <ben@apollographql.com>
Date:   Tue Aug 17 16:00:56 2021 -0400

    Avoid full reobservation in QueryInfo listener when diff.complete.

diff --git a/src/core/QueryInfo.ts b/src/core/QueryInfo.ts
index ea23f374a..7f5b6ce5d 100644
--- a/src/core/QueryInfo.ts
+++ b/src/core/QueryInfo.ts
@@ -235,7 +235,8 @@ export class QueryInfo {
         // full reobservation, since oq.reobserve might make a network
         // request, and we don't want to trigger network requests for
         // optimistic updates.
-        if (this.getDiff().fromOptimisticTransaction) {
+        const diff = this.getDiff();
+        if (diff.complete || diff.fromOptimisticTransaction) {
           oq["observe"]();
         } else {
           oq.reobserve();

The observe method of ObservableQuery delivers the current result only, whereas reobserve reapplies the fetchPolicy, potentially delivering multiple results.

I hope to have more answers soon. Thanks to @Akryum for sharing your findings!

[lrojas94]

So what's the verdict here? Is it reasonable to expect/say this is a valid solution and no harm comes from it - hence it'd make it onto the next release <3? (Wishful thinking - I know, but hey, what's there if there's no hope?)

[kamilmsoprasteria]

Hi @benjamn
any chance that this fix you posted in #6916 (comment) will go into some near version? 3.5?

[justinhandley]

Just following this - setting the fetchPolicy as recommended does not seem to stop my app from doing a whole query when a new subscription item is received and therefore duplicating the entire cache. I'm using the incredibly ugly hack

return incoming.length > 3 ? [...incoming] : [...existing, ...incoming]

All subscriptions return all values PLUS the new one, which is always more than 3 (the number we get on fetchMore). So if this is a fetchmore we merge, and otherwise we just write the incoming - since the subscription query on the front end is already merging in the new data correctly, the incoming in that case is what we need.

[videni]

what is the solution at the moment ? still have this issue with @apollo/client": "^3.5.8, I have disabled local cache.

 const { loading, error, data, fetchMore} = useQuery(GET_CUSTOMER_TICKETS, {
    variables: {
      first: DEFAULT_PAGE_SIZE,
      last: DEFAULT_PAGE_SIZE
    },
    fetchPolicy: 'no-cache',
    nextFetchPolicy: 'no-cache',
  });

[alamothe]

@videni The whole world is pretty much waiting on this and other similar issues. Hopefully it will make it to 3.6 release.

The last good version of Apollo Client was 2.6.10 and we are still on this version.

EDIT: The last package versions that work correctly are:

    "@apollo/react-hooks": "^3.1.5",
    "apollo-cache-inmemory": "^1.6.6",
    "apollo-client": "^2.6.10",
    "apollo-link": "^1.2.14",
    "apollo-link-context": "^1.0.20",
    "apollo-link-http": "^1.5.17",
    "apollo-link-schema": "^1.2.5",

[jairmedeiros]

Any update? At this moment I can't use fetch more or create an infinite scroll 😭.

There are some workaround to make this fetchMore?

[jonathan-guerrero-milli]

@jairmedeiros
Based on this comment it seems like a workaround is to useState for the variables and modify them instead of callingfetchMore or refetch.

useQuery should requery based on changed variables, see the linked comment for how they did it with useLazyQuery.

It probably requires a refactor though to handle the loading/error states (our code used to wait for/catch the fetchMore promise to show a loader/message in the footer of our SectionList)

[benjamn]

Some good news, at long last: using @Akryum's reproduction and @apollo/client@3.6.0-beta.10, I see no extra network requests after the fetchMore calls (thanks to #9504). These changes have just landed (today) on the release-3.6 branch, so we are actively looking for help testing them.

While this change seems like the right behavior for most use cases, there's a chance it will be disruptive for code that was previously relying on fetchMore triggering extra network requests (hard to imagine, but certainly possible). If you're reading this, please help us test/validate these changes by updating your projects with npm i @apollo/client@beta when you have the chance! Your help will get v3.6 released sooner (in April).

[nateql]

Was closed on the team board so I am closing it here.

[alamothe]

How about github.com//issues/7485 and #6760, will those be addressed as well?

[zakton5]

For any poor soul who couldn't figure this out like me. My problem was unique in that I had set custom keyFields for the specific type I was querying for.

keyFields: ["id", "target", ["entityId"]]

The problem for me specifically was that target could sometimes be null. I think Apollo got upset when this happened because it was trying to access target.entityId and couldn't. So maybe there was an internal error (no logs about it) and it just retried the query with the default variables/arguments?

My fix was to just remove entityId entirely. target was sufficient.

keyFields: ["id", "target"]

[hatched-kade]

I had a similar issue to @zakton5. I originally though it was a fetchPolicy issue, but what was actually causing the behavior was a custom merge function for a field that originally looked like

merge: (existing, incoming) => incoming || existing

That field could be null for some of my data while I was doing a fetchMore, so existing=undefined and incoming=null. My merge function would return undefined, which must've triggered some silent error causing apollo to retry the original query, with the network status going from 3 -> 1.

Changing my merge to be:

merge: (existing, incoming) => {
  if (!incoming && existing) {
    return existing;
  }
  return incoming;
}

caused the issue to stop happening.