Query silently fails with multiple paginated queries with differing child fields

[maxsalven]

How to reproduce the issue:
Visit https://codesandbox.io/s/admiring-dhawan-tjtz5?file=/src/client.js

Wait for query A to finish. This is a 'heavy' query, with a small limit, and a load more button. We are using the offsetLimitPagination typePolicy here to enable 'load more' to work.

Now click on B at the top. This queries the same field as A, but with a much larger limit, and only one field. You might imagine we went from some sort of infinite scroll detailed list view to a very high level but broad overview.

Now click on A at the top to go back to query A.

Query A silently fails. There is no data, and no error, and no console explanation that there is a problem somewhere.

It's very natural for separate parts of an app to use the same field with different limits and subfields. I am trying to upgrade from Apollo 2 where I was previously using updateQuery with fetchMore for this behavior.

Versions
3.4.7

[benjamn]

Hi @maxsalven! Thanks for the reproduction.

One of the changes we made in Apollo Client v3.4 was to suppress some of our noisier warning messages in development, by lowering their severity to "debug":

• Suppress Missing cache result fields... warnings unless setLogVerbosity("debug") configured #8489

These warnings happen to be useful here. To display them again (temporarily), you can call setLogVerbosity("debug"):

diff --git a/src/index.js b/src/index.js
index b999594..7cfb40f 100644
--- a/src/index.js
+++ b/src/index.js
@@ -1,11 +1,13 @@
 import { StrictMode } from "react";
 import ReactDOM from "react-dom";
-import { ApolloProvider } from "@apollo/client";
+import { ApolloProvider, setLogVerbosity } from "@apollo/client";
 import client from "./client";
 import { BrowserRouter as Router } from "react-router-dom";
 
 import App from "./App";
 
+setLogVerbosity("debug");
+
 const rootElement = document.getElementById("root");
 ReactDOM.render(
   <StrictMode>

Note that you may need to adjust your browser console logging level to include debug/verbose messages. With this logging reenabled, switching back to query A in your reproduction dumps a bunch of similar warnings to the console:

If it's not already apparent, the common thread between these errors is the absence of data for the Launch.rocket fields, which are required by your EXTENDED_QUERY but not fetched by your SLIM_QUERY.

One quick way to fix (or help diagnose) this problem is to relax the expectations of EXTENDED_QUERY by passing returnPartialData: true to useQuery:

diff --git a/src/App.js b/src/App.js
index 6718e6e..5f25e77 100644
--- a/src/App.js
+++ b/src/App.js
@@ -30,7 +30,8 @@ const EXTENDED_QUERY = gql`
 
 const A = () => {
   const { error, data, fetchMore } = useQuery(EXTENDED_QUERY, {
-    variables: { offset: 0, limit: 5 }
+    variables: { offset: 0, limit: 5 },
+    returnPartialData: true,
   });
 
   return (

This will allow you to display the A launches with their extended data and the B launches with the slim data that's available, even though it's not complete.

Another way to fix the problem would be to provide some default value for the Launch.rocket field (such as null), using a field policy:

diff --git a/src/client.js b/src/client.js
index 356bd42..66338f3 100644
--- a/src/client.js
+++ b/src/client.js
@@ -4,13 +4,20 @@ import { offsetLimitPagination } from "@apollo/client/utilities";
 const cache = new InMemoryCache({
   typePolicies: {
     Query: {
       fields: {
         launchesPast: offsetLimitPagination()
       }
-    }
+    },
+    Launch: {
+      fields: {
+        rocket(rocket = null) {
+          return rocket;
+        },
+      },
+    },
   }
 });
 
 const client = new ApolloClient({
   cache: cache,
   uri: "https://api.spacex.land/graphql/"

This has a similar effect to returnPartialData: true, except the Launch.rocket fields will be null instead of absent.

I'll stop there, but there are a few other possible solutions here, if neither of these ideas works for you.

[maxsalven]

Thanks for the rapid response. I appreciate all the effort that has gone into Apollo, it's a critical part of our software, and we're very grateful for it.

The setLogVerbosity command is helpful, thank you.

However I'm still surprised by this behavior. Adding a valid query to a completely unrelated part of an app can cause a query elsewhere to fail silently (i.e. break), and only if the user takes a specific path (e.g. hits query B before query A). This is a heavy maintenance burden, any time you add a query you now need to be worried that you accidentally broke another, unrelated, part of your app, and possibly one that you've never even personally worked on. Presumably you need to check the field resolvers for every field in the query you just added (and it's not unusual for us to have 50-100 fields), and if there are pagination style rules for them, then you need to look for any other query that shares those fields. And if they are shared, you now need to start modifying an unrelated component that was working correctly before, in a way that's only testable by navigating through the app in a very specific order.

Unfortunately both of your proposed solutions have a bit of a 'band aid' feel to them rather than addressing the underlying issue. My screen for query A is built with query A in mind; it expects the fields in the query to be there, and expects e.g. rocket to never be null (and indeed in the hypothetical gql schema rocket will be non null, and the auto-generated typescript typings will also state it's never null or absent). To now tell the developer of component A that 'the rocket field might not be there, or if it is, it might be null' even though it's in the query, and the server guarantees it is not null, seems off. And in my real world scenario, we have close to a hundred fields under the equivalent of the Launch object in this specific query that I'm encountering this issue for.

You mention some other solutions, I'd be very keen to hear them. Specifically, it seems like if query A can't resolve via the cache, then it should resolve via the network. Or ideally, perhaps we can segregate the data in the read policy based on whether it has all the required fields. In no situation would I expect it to not return anything, i.e. neither data nor an error.

Is partialRefetch a solution here? It appears to have been deprecated, but it does seem to fix the behavior. However it causes query A to fire twice when you navigate back to it in the code sandbox.

[benjamn]

@maxsalven Thank you for drawing the discussion back to the surprising part (query A silently failing to display any results)! I agree there's something unusual/buggy going on here, not addressed by my previous comment.

One question before I continue investigating and propose more solutions: are you thinking of these two Query.launchesPast lists as two separate lists, where fetchMore appends additional items to one list or the other, but not both? Or are you thinking of the queries as two different ways of fetching data into a single combined list (so fetchMore always appends to that list)?

[maxsalven]

Thanks @benjamn

are you thinking of these two Query.launchesPast lists as two separate lists, where fetchMore appends additional items to one list or the other, but not both? Or are you thinking of the queries as two different ways of fetching data into a single combined list (so fetchMore always appends to that list)?

In my specific use case here, they are two separate lists.

[maxsalven]

Hi @benjamn I noticed you released 3.4.8 which includes #8642

I've created a new codesandbox with 3.4.8 here:
https://codesandbox.io/s/inspiring-rgb-y1m2z?file=/src/App.js

The original bug I reported appears to be fixed. However I note a new bug:

1. Click 'load more' on the A screen. We successfully fetch more records, and the screen updates from A - 5 to A - 10
2. Navigate to B, and wait for the query to load
3. Navigate back to A. This is back to A - 5 now (not 100% ideal, but much better than displaying nothing as before). However if you now click 'load more', the network request fires, but the component does not update. It stays stuck on A - 5, i.e. fetchMore is no longer functioning.

[benjamn]

@maxsalven I think I finally have a good/complete answer for you!

Based on my debugging, I would say the central remaining problem in your reproduction is that the offetLimitPagination() helper defaults to keyArgs = false, which means the Query.launchesPast field will always be stored in the cache using just its field name, launchesPast, with no additional distinguishing information. This is ultimately why your "slim" results end up mixed together in the same list as your "extended" results. If you could just keep those lists separate in the cache, you'd be able to use fetchMore with no problem.

Normally, I would recommend finding a way of distinguishing the two different Query.launchesPast fields in your field policy functions (read and/or merge). Unfortunately, since you're using the same arguments (offset and limit) for the field in both queries, it's tricky/ill-advised to use those arguments to distinguish the fields.

However, you could try passing a new client-only extended: Boolean argument, like so:

query Extended($offset: Int!, $limit: Int!) {
  # Almost works, as long as your server knows about the extended argument.
  launchesPast(extended: true, offset: $offset, limit: $limit) {...}
}

query Slim($offset: Int!, $limit: Int!) {
  launchesPast(extended: false, offset: $offset, limit: $limit) {...}
}

and then you could configure offsetLimitPagination(["extended"]) instead of just offsetLimitPagination() to take this made-up extended argument into account. Though this approach can work, it runs the risk of confusing your GraphQL server, which might not be so happy about unexpected new arguments.

A more common/recommended solution is to disambiguate the fields in the two queries using a @connection(key:...) directive. Unfortunately, while information about directives is included in the field key when you don't provide a keyArgs configuration for the Query.launchesPast, it gets lost when you switch to keyArgs: [...], forcing you to implement a more complicated keyArgs(args, context) {...} function that takes context.field.directives into consideration.

This limitation of keyArgs: [...] array notation was noted recently in #8659, which gave me an idea for a way to generalize keyArgs to include selective information from field directives and variables, as well as the field's arguments:

• Allow @directive and $variable strings in field policy keyArgs: ["arg", "@dir", "$var"] arrays #8678

Once PR #8678 is shipped, the following changes should make your reproduction work perfectly:

click to expand

diff --git a/package.json b/package.json
index 8eb2ff8..91e6394 100644
--- a/package.json
+++ b/package.json
@@ -8,7 +8,7 @@
   ],
   "main": "src/index.js",
   "dependencies": {
-    "@apollo/client": "3.4.7",
+    "@apollo/client": "beta",
     "graphql": "15.5.1",
     "lodash": "4.17.21",
     "react": "17.0.2",
diff --git a/src/App.js b/src/App.js
index 07ec023..5bcd1ab 100644
--- a/src/App.js
+++ b/src/App.js
@@ -3,7 +3,7 @@ import { Switch, Route, Link } from "react-router-dom";
 
 const EXTENDED_QUERY = gql`
   query Extended($offset: Int!, $limit: Int!) {
-    launchesPast(offset: $offset, limit: $limit) {
+    launchesPast(offset: $offset, limit: $limit) @connection(key: "extended") {
       id
       rocket {
         rocket_name
@@ -58,7 +58,7 @@ const A = () => {
 
 const SLIM_QUERY = gql`
   query Slim($offset: Int!, $limit: Int!) {
-    launchesPast(offset: $offset, limit: $limit) {
+    launchesPast(offset: $offset, limit: $limit) @connection(key: "slim") {
       id
       mission_name
     }
diff --git a/src/client.js b/src/client.js
index 356bd42..20c6541 100644
--- a/src/client.js
+++ b/src/client.js
@@ -5,7 +5,7 @@ const cache = new InMemoryCache({
   typePolicies: {
     Query: {
       fields: {
-        launchesPast: offsetLimitPagination()
+        launchesPast: offsetLimitPagination(["@connection", ["key"]])
       }
     }
   }

I originally hoped keyArgs would make @connection obsolete, and with these changes I think that's closer than ever. Yes, I'm recommending you use @connection to solve this problem, but there's nothing special or hard-coded about it, as far as ApolloClient or InMemoryCache are concerned. The following code would work equally well for your reproduction, if your server understood the @extended and @slim directives:

click to expand

diff --git a/src/App.js b/src/App.js
index 07ec023..839069c 100644
--- a/src/App.js
+++ b/src/App.js
@@ -3,7 +3,7 @@ import { Switch, Route, Link } from "react-router-dom";
 
 const EXTENDED_QUERY = gql`
   query Extended($offset: Int!, $limit: Int!) {
-    launchesPast(offset: $offset, limit: $limit) {
+    launchesPast(offset: $offset, limit: $limit) @extended {
       id
       rocket {
         rocket_name
@@ -58,7 +58,7 @@ const A = () => {
 
 const SLIM_QUERY = gql`
   query Slim($offset: Int!, $limit: Int!) {
-    launchesPast(offset: $offset, limit: $limit) {
+    launchesPast(offset: $offset, limit: $limit) @slim {
       id
       mission_name
     }
diff --git a/src/client.js b/src/client.js
index 356bd42..4fca18a 100644
--- a/src/client.js
+++ b/src/client.js
@@ -5,7 +5,7 @@ const cache = new InMemoryCache({
   typePolicies: {
     Query: {
       fields: {
-        launchesPast: offsetLimitPagination()
+        launchesPast: offsetLimitPagination(["@extended", "@slim"])
       }
     }
   }

In other words, I think the only remaining advantage of @connection(key: ...) over keyArgs + custom directives is that most GraphQL servers already understand/ignore @connection and won't complain about it.

Hope that all makes (some) sense! Thanks again for the reproduction and your patience.

[hwillson]

This should now be resolved in @apollo/client@3.5.0. Let us know if you notice any issues. Thanks!