Repository: apollographql/apollo-client

Problem statement:
"Loading" stays true when the same result is returned twice in Apollo Client 3.6.9
This has been a long-standing issue that I've seen numerous people bring up, but I may have found the cause.

Here is the problem:

```js
const request = {
  query: gql`
    query test($foo: String) {
      test(foo: $foo) {
        value
      }
    }
  `,
  variables: { foo: "bar" },
  notifyOnNetworkStatusChange: true
};

const observer = client.watchQuery(request);

observer.subscribe({
  next: ({ loading }) => {
    console.log(loading);
  }
});
```

When the GraphQL query returns the same result successfully and subsequently, even if the variables have changed, the `loading` flag remains `true` even after the second request returns successfully. It does not return back to `false` until the result returns something different than it did the last time.

The expected outcome is that `loading` flips to `true` when the request is in flight and then flips back to `false` after the request completes, even if the result is the same between the two requests.

I believe there's a good chance this bug is due to [this bit of code](https://github.com/apollographql/apollo-client/blob/d0da9d396a749e90b008305ecfd37b17d39e3d38/src/core/ObservableQuery.ts#L874-L880):

```js
  private reportResult(
    result: ApolloQueryResult<TData>,
    variables: TVariables | undefined,
  ) {
    const lastError = this.getLastError();
    if (lastError || this.isDifferentFromLastResult(result)) {
      if (lastError || !result.partial || this.options.returnPartialData) {
        this.updateLastResult(result, variables);
      }

      iterateObserversSafely(this.observers, 'next', result);
    }
  }
```

I believe that maybe it was _supposed_ to be:

```js
  private reportResult(
    result: ApolloQueryResult<TData>,
    variables: TVariables | undefined,
  ) {
    const lastError = this.getLastError();
    if (lastError || !result.partial || this.options.returnPartialData || this.isDifferentFromLastResult(result)) {
      this.updateLastResult(result, variables);
      iterateObserversSafely(this.observers, 'next', result);
    }
  }
```

In the former, the observer will never iterate if the request is successful and the next result is the same as the previous result.

Maybe this was just a minor coding oversight. But it would be nice to get feedback from the Apollo Client team to make sure this aligns with their thinking.

The way I was able to confirm this was the issue is that, when I added a timestamp of the request to the GraphQL API result, the problem was completely gone. So when the result is different with each request, it works as intended. But I shouldn't have to do this in a production setting.

Note I am running `@apollo/client` 3.6.9 and `graphql` 16.6.0, both the newest versions. I am also _not_ using React. So if a fix for this was previously introduced for React, I'm afraid it wouldn't have fixed the root of the problem in Apollo Client itself.

Many tickets about this have been opened by others, all with suggestions of different workarounds, like changing `networkPolicy` to `network-only`, `no-cache`, or `cache-and-network`. Or even setting `pollInterval` to `0`. But none of these worked for me, presumably because the root problem seems to be related to the conditions under which the observer will iterate. But it is interesting that the cache is still checked even when using a `networkPolicy` of `no-cache`.

Here are some of the related issues I found on this topic:

https://github.com/apollographql/apollo-client/issues/6334
https://github.com/apollographql/apollo-client/issues/9845
https://github.com/apollographql/apollo-client/pull/9844 (another attempted fix, but I wonder if this one was overkill)
https://github.com/apollographql/apollo-client/pull/6417 (a merged PR from two years ago attempting to fix this, but not successfully)
https://github.com/apollographql/apollo-client/issues/9689 (maybe related)
https://github.com/apollographql/apollo-client/issues/9668 (also maybe related)

Would greatly appreciate any insight on this. I currently don't have any other workarounds other than to add a timestamp to our API, but since we have a public-facing API, this would not be ideal.

Thank you!


Hints:
Sorry to hear this is a problem @michaelcbrook - any chance you can provide a small runnable reproduction (repro [template](https://github.com/apollographql/react-apollo-error-template), repro [codesandbox](https://codesandbox.io/s/github/apollographql/react-apollo-error-template)) that demonstrates the problem? This will help expedite a fix.
Hey @hwillson,

This one gave me a run for my money to cut down to a reproduction, but I finally got it:

https://codesandbox.io/s/competent-worker-7qu3p1?file=/src/index.jsx

Turns out there are some more specific criteria that trigger this bug. In working on the reproduction, I found this error only happens under these conditions:

1) Variables must contain non-primitive values (I confirmed an array of objects creates the issue)
2) The non-primitive value must change between requests
3) The next result must be the same as the previous result, even though the variables have changed

Of note, the array of objects is being passed as a JSON GraphQL type via the scalars provided by this package: https://www.npmjs.com/package/graphql-scalars

...but that GraphQLJSON definition is set on the server, not the client...

I have not yet tested whether or not this issue still happens with a natively provided data type instead of a custom scalar, but seeing as though Apollo Client _should_ be naive to that type (I think), that may not have any bearing on the problem, at least in my mind.

But I wanted to get your feedback and see whether that's consistent with your thinking or not.

As for my proposed solution, I'm less certain now in light of what I found after creating this reproduction. But that's beyond my knowledge.

Thanks!
In case this can help, in our team we were able to replicate this issue by making two parallel async calls with the same params, as follow:
```
 LOG  filter: NEARBY
 LOG  input: {"coordinates": {"userLat": 25, "userLng": -80.1}, "first": 20}
 LOG  filter: FAVORITE
 LOG  input: {"coordinates": {"userLat": 25, "userLng": -80.1}, "first": 20}
 ```
 
 The GraphQL query looks something like:
 ```
 gql`
    query GetPlaces($input: PlacesInput) {
  places(input: $input) {
    pageInfo {
      hasNextPage
      endCursor
    }
    totalCount
    nodes {
      ...PlaceNodeFragment
    }
  }
}
    ${PlaceNodeFragmentDoc}`;
 ```


Patch:
diff --git a/src/core/ObservableQuery.ts b/src/core/ObservableQuery.ts
--- a/src/core/ObservableQuery.ts
+++ b/src/core/ObservableQuery.ts
@@ -303,8 +303,15 @@ export class ObservableQuery<
 
   // Compares newResult to the snapshot we took of this.lastResult when it was
   // first received.
-  public isDifferentFromLastResult(newResult: ApolloQueryResult<TData>) {
-    return !this.last || !equal(this.last.result, newResult);
+  public isDifferentFromLastResult(
+    newResult: ApolloQueryResult<TData>,
+    variables?: TVariables
+  ) {
+    return (
+      !this.last ||
+      !equal(this.last.result, newResult) ||
+      (variables && !equal(this.last.variables, variables))
+    );
   }
 
   private getLast<K extends keyof Last<TData, TVariables>>(
@@ -872,7 +879,7 @@ Did you mean to call refetch(variables) instead of refetch({ variables })?`);
     variables: TVariables | undefined,
   ) {
     const lastError = this.getLastError();
-    if (lastError || this.isDifferentFromLastResult(result)) {
+    if (lastError || this.isDifferentFromLastResult(result, variables)) {
       if (lastError || !result.partial || this.options.returnPartialData) {
         this.updateLastResult(result, variables);
       }


Test patch:
diff --git a/src/__tests__/local-state/general.ts b/src/__tests__/local-state/general.ts
--- a/src/__tests__/local-state/general.ts
+++ b/src/__tests__/local-state/general.ts
@@ -1,5 +1,16 @@
 import gql from 'graphql-tag';
-import { DocumentNode, GraphQLError, getIntrospectionQuery } from 'graphql';
+import {
+  graphql,
+  GraphQLInt,
+  print,
+  DocumentNode,
+  GraphQLError,
+  getIntrospectionQuery,
+  GraphQLSchema,
+  GraphQLObjectType,
+  GraphQLID,
+  GraphQLString
+} from 'graphql';
 
 import { Observable } from '../../utilities';
 import { ApolloLink } from '../../link/core';
@@ -819,6 +830,102 @@ describe('Combining client and server state/operations', () => {
     }, 10);
   });
 
+  itAsync('query resolves with loading: false if subsequent responses contain the same data', (resolve, reject) => {
+    const request = {
+      query: gql`
+        query people($id: Int) {
+          people(id: $id) {
+            id
+            name
+          }
+        }
+      `,
+      variables: {
+        id: 1,
+      },
+      notifyOnNetworkStatusChange: true
+    };
+
+    const PersonType = new GraphQLObjectType({
+      name: "Person",
+      fields: {
+        id: { type: GraphQLID },
+        name: { type: GraphQLString }
+      }
+    });
+
+    const peopleData = [
+      { id: 1, name: "John Smith" },
+      { id: 2, name: "Sara Smith" },
+      { id: 3, name: "Budd Deey" }
+    ];
+
+    const QueryType = new GraphQLObjectType({
+      name: "Query",
+      fields: {
+        people: {
+          type: PersonType,
+          args: {
+            id: {
+              type: GraphQLInt
+            }
+          },
+          resolve: (_, { id }) => {
+            return peopleData;
+          }
+        }
+      }
+    });
+
+    const schema = new GraphQLSchema({ query: QueryType });
+
+    const link = new ApolloLink(operation => {
+      // @ts-ignore
+      return new Observable(async observer => {
+        const { query, operationName, variables } = operation;
+        try {
+          const result = await graphql({
+            schema,
+            source: print(query),
+            variableValues: variables,
+            operationName,
+          });
+          observer.next(result);
+          observer.complete();
+        } catch (err) {
+          observer.error(err);
+        }
+      });
+    });
+
+    const client = new ApolloClient({
+      cache: new InMemoryCache(),
+      link,
+    });
+
+    const observer = client.watchQuery(request);
+
+    let count = 0;
+    observer.subscribe({
+      next: ({ loading, data }) => {
+        if (count === 0) expect(loading).toBe(false);
+        if (count === 1) expect(loading).toBe(true);
+        if (count === 2) {
+          expect(loading).toBe(false)
+          resolve();
+        };
+        count++;
+      },
+      error: reject,
+    });
+
+    setTimeout(() => {
+      observer.refetch({
+        id: 2
+      });
+    }, 1);
+  });
+
   itAsync('should correctly propagate an error from a client resolver', async (resolve, reject) => {
     const data = {
       list: {
diff --git a/src/core/__tests__/fetchPolicies.ts b/src/core/__tests__/fetchPolicies.ts
--- a/src/core/__tests__/fetchPolicies.ts
+++ b/src/core/__tests__/fetchPolicies.ts
@@ -1163,7 +1163,17 @@ describe("nextFetchPolicy", () => {
       // resets the fetchPolicy to context.initialPolicy), so cache-first is
       // still what we see here.
       expect(observable.options.fetchPolicy).toBe("cache-first");
+    } else if (count === 3) {
+      expect(result.loading).toBe(false);
+      expect(result.data).toEqual({
+        linkCounter: 2,
+        opName: "EchoQuery",
+        opVars: {
+          refetching: true,
+        },
+      });
 
+      expect(observable.options.fetchPolicy).toBe("cache-first");
       setTimeout(resolve, 20);
     } else {
       reject(`Too many results (${count})`);