fix: caching issue in #11560 (reverts #11202)

.changeset/quick-pears-give.md

@@ -0,0 +1,5 @@
+---
+"@apollo/client": patch
+---
+
+Revert PR [#11202](https://github.com/apollographql/apollo-client/pull/11202) to fix caching bug reported in [#11560](https://github.com/apollographql/apollo-client/issues/11560)

src/__tests__/client.ts

@@ -2949,10 +2949,7 @@ describe("client", () => {
       return client
         .query({ query })
         .then(({ data }) => {
-          const { price, ...todoWithoutPrice } = data.todos[0];
-          expect(data).toEqual({
-            todos: [todoWithoutPrice],
-          });
+          expect(data).toEqual(result.data);
         })
         .then(resolve, reject);
     });

src/core/QueryInfo.ts

@@ -364,8 +364,7 @@ export class QueryInfo {
       "variables" | "fetchPolicy" | "errorPolicy"
     >,
     cacheWriteBehavior: CacheWriteBehavior
-  ): typeof result {
-    result = { ...result };
+  ) {
     const merger = new DeepMerger();
     const graphQLErrors =
       isNonEmptyArray(result.errors) ? result.errors.slice(0) : [];
@@ -411,10 +410,7 @@ export class QueryInfo {
             });
 
             this.lastWrite = {
-              // Make a shallow defensive copy of the result object, in case we
-              // later later modify result.data in place, since we don't want
-              // that mutation affecting the saved lastWrite.result.data.
-              result: { ...result },
+              result,
               variables: options.variables,
               dmCount: destructiveMethodCounts.get(this.cache),
             };
@@ -476,19 +472,20 @@ export class QueryInfo {
             this.updateWatch(options.variables);
           }
 
-          // If we're allowed to write to the cache, update result.data to be
-          // the result as re-read from the cache, rather than the raw network
-          // result. Set without setDiff to avoid triggering a notify call,
-          // since we have other ways of notifying for this result.
+          // If we're allowed to write to the cache, and we can read a
+          // complete result from the cache, update result.data to be the
+          // result from the cache, rather than the raw network result.
+          // Set without setDiff to avoid triggering a notify call, since
+          // we have other ways of notifying for this result.
           this.updateLastDiff(diff, diffOptions);
-          result.data = diff.result;
+          if (diff.complete) {
+            result.data = diff.result;
+          }
         });
       } else {
         this.lastWrite = void 0;
       }
     }
-
-    return result;
   }
 
   public markReady() {

src/core/QueryManager.ts

@@ -1194,7 +1194,7 @@ export class QueryManager<TStore> {
           // Use linkDocument rather than queryInfo.document so the
           // operation/fragments used to write the result are the same as the
           // ones used to obtain it from the link.
-          result = queryInfo.markResult(
+          queryInfo.markResult(
             result,
             linkDocument,
             options,

src/core/__tests__/QueryManager/index.ts

@@ -67,6 +67,14 @@ export function resetStore(qm: QueryManager<any>) {
 }
 
 describe("QueryManager", () => {
+  // Standard "get id from object" method.
+  const dataIdFromObject = (object: any) => {
+    if (object.__typename && object.id) {
+      return object.__typename + "__" + object.id;
+    }
+    return undefined;
+  };
+
   // Helper method that serves as the constructor method for
   // QueryManager but has defaults that make sense for these
   // tests.
@@ -2217,6 +2225,107 @@ describe("QueryManager", () => {
     }
   );
 
+  itAsync(
+    "should not return stale data when we orphan a real-id node in the store with a real-id node",
+    (resolve, reject) => {
+      const query1 = gql`
+        query {
+          author {
+            name {
+              firstName
+              lastName
+            }
+            age
+            id
+            __typename
+          }
+        }
+      `;
+      const query2 = gql`
+        query {
+          author {
+            name {
+              firstName
+            }
+            id
+            __typename
+          }
+        }
+      `;
+      const data1 = {
+        author: {
+          name: {
+            firstName: "John",
+            lastName: "Smith",
+          },
+          age: 18,
+          id: "187",
+          __typename: "Author",
+        },
+      };
+      const data2 = {
+        author: {
+          name: {
+            firstName: "John",
+          },
+          id: "197",
+          __typename: "Author",
+        },
+      };
+      const reducerConfig = { dataIdFromObject };
+      const queryManager = createQueryManager({
+        link: mockSingleLink(
+          {
+            request: { query: query1 },
+            result: { data: data1 },
+          },
+          {
+            request: { query: query2 },
+            result: { data: data2 },
+          },
+          {
+            request: { query: query1 },
+            result: { data: data1 },
+          }
+        ).setOnError(reject),
+        config: reducerConfig,
+      });
+
+      const observable1 = queryManager.watchQuery<any>({ query: query1 });
+      const observable2 = queryManager.watchQuery<any>({ query: query2 });
+
+      // I'm not sure the waiting 60 here really is required, but the test used to do it
+      return Promise.all([
+        observableToPromise(
+          {
+            observable: observable1,
+            wait: 60,
+          },
+          (result) => {
+            expect(result).toEqual({
+              data: data1,
+              loading: false,
+              networkStatus: NetworkStatus.ready,
+            });
+          }
+        ),
+        observableToPromise(
+          {
+            observable: observable2,
+            wait: 60,
+          },
+          (result) => {
+            expect(result).toEqual({
+              data: data2,
+              loading: false,
+              networkStatus: NetworkStatus.ready,
+            });
+          }
+        ),
+      ]).then(resolve, reject);
+    }
+  );
+
   itAsync(
     "should return partial data when configured when we orphan a real-id node in the store with a real-id node",
     (resolve, reject) => {
@@ -2411,7 +2520,9 @@ describe("QueryManager", () => {
             loading: false,
             networkStatus: NetworkStatus.ready,
             data: {
-              info: {},
+              info: {
+                a: "ay",
+              },
             },
           });
           setTimeout(resolve, 100);

src/react/hooks/__tests__/useQuery.test.tsx

@@ -6017,10 +6017,7 @@ describe("useQuery Hook", () => {
         },
         { interval: 1 }
       );
-      const { vine, ...carDataWithoutVine } = carData.cars[0];
-      expect(result.current.data).toEqual({
-        cars: [carDataWithoutVine],
-      });
+      expect(result.current.data).toEqual(carData);
       expect(result.current.error).toBeUndefined();
 
       expect(consoleSpy.error).toHaveBeenCalled();