fix(regression): avoid calling useQuery onCompleted for cache wri…

…tes (#10229)

.changeset/early-pens-retire.md

@@ -0,0 +1,5 @@
+---
+"@apollo/client": patch
+---
+
+Avoid calling `useQuery` `onCompleted` for cache writes

CHANGELOG.md

@@ -1,3 +1,10 @@
+## Apollo Client 3.8.0
+
+### Bug fixes
+
+- Avoid calling `useQuery` `onCompleted` callback after cache writes, only after the originating query's network request(s) complete. <br/>
+  [@alessbell](https://github.com/alessbell) in [#10229](https://github.com/apollographql/apollo-client/pull/10229)
+
 ## Apollo Client 3.7.2 (2022-12-06)
 
 ### Improvements

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

@@ -3,7 +3,8 @@ import React, { Fragment, useEffect, useState } from 'react';
 import { DocumentNode, GraphQLError } from 'graphql';
 import gql from 'graphql-tag';
 import { act } from 'react-dom/test-utils';
-import { render, waitFor } from '@testing-library/react';
+import { render, screen, waitFor } from '@testing-library/react';
+import userEvent from '@testing-library/user-event';
 import { renderHook } from '@testing-library/react-hooks';
 import {
   ApolloClient,
@@ -3069,7 +3070,7 @@ describe('useQuery Hook', () => {
       expect(onCompleted).toHaveBeenCalledTimes(1);
     });
 
-    it('onCompleted should work with polling', async () => {
+    it('onCompleted should not fire for polling queries without notifyOnNetworkStatusChange: true', async () => {
       const query = gql`{ hello }`;
       const mocks = [
         {
@@ -3112,6 +3113,68 @@ describe('useQuery Hook', () => {
       await waitForNextUpdate();
       expect(result.current.loading).toBe(false);
       expect(result.current.data).toEqual({ hello: 'world 2' });
+      expect(onCompleted).toHaveBeenCalledTimes(1);
+
+      await waitForNextUpdate();
+      expect(result.current.loading).toBe(false);
+      expect(result.current.data).toEqual({ hello: 'world 3' });
+      expect(onCompleted).toHaveBeenCalledTimes(1);
+    });
+
+    it('onCompleted should fire when polling with notifyOnNetworkStatusChange: true', async () => {
+      const query = gql`{ hello }`;
+      const mocks = [
+        {
+          request: { query },
+          result: { data: { hello: 'world 1' } },
+        },
+        {
+          request: { query },
+          result: { data: { hello: 'world 2' } },
+        },
+        {
+          request: { query },
+          result: { data: { hello: 'world 3' } },
+        },
+      ];
+
+      const cache = new InMemoryCache();
+      const onCompleted = jest.fn();
+      const { result, waitForNextUpdate } = renderHook(
+        () => useQuery(query, {
+          onCompleted,
+          notifyOnNetworkStatusChange: true,
+          pollInterval: 10,
+        }),
+        {
+          wrapper: ({ children }) => (
+            <MockedProvider mocks={mocks} cache={cache}>
+              {children}
+            </MockedProvider>
+          ),
+        },
+      );
+
+      expect(result.current.loading).toBe(true);
+
+      await waitForNextUpdate();
+      expect(result.current.loading).toBe(false);
+      expect(result.current.data).toEqual({ hello: 'world 1' });
+      expect(onCompleted).toHaveBeenCalledTimes(1);
+
+      await waitForNextUpdate();
+      expect(result.current.loading).toBe(true);
+      expect(result.current.data).toEqual({ hello: 'world 1' });
+      expect(onCompleted).toHaveBeenCalledTimes(1);
+
+      await waitForNextUpdate();
+      expect(result.current.loading).toBe(false);
+      expect(result.current.data).toEqual({ hello: 'world 2' });
+      expect(onCompleted).toHaveBeenCalledTimes(2);
+
+      await waitForNextUpdate();
+      expect(result.current.loading).toBe(true);
+      expect(result.current.data).toEqual({ hello: 'world 2' });
       expect(onCompleted).toHaveBeenCalledTimes(2);
 
       await waitForNextUpdate();
@@ -3165,6 +3228,133 @@ describe('useQuery Hook', () => {
       expect(errorSpy).not.toHaveBeenCalled();
       errorSpy.mockRestore();
     });
+
+    it("onCompleted should not execute on cache writes after initial query execution", async () => {
+      const query = gql`
+        {
+          hello
+        }
+      `;
+      const mocks = [
+        {
+          request: { query },
+          result: { data: { hello: "foo" } },
+        },
+        {
+          request: { query },
+          result: { data: { hello: "bar" } },
+        },
+      ];
+      const link = new MockLink(mocks);
+      const cache = new InMemoryCache();
+      const onCompleted = jest.fn();
+
+      const ChildComponent: React.FC = () => {
+        const { data, client } = useQuery(query, { onCompleted });
+        function refetchQueries() {
+          client.refetchQueries({ include: 'active' })
+        }
+        function writeQuery() {
+          client.writeQuery({ query, data: { hello: 'baz'}})
+        }
+        return (
+          <div>
+            <span>Data: {data?.hello}</span>
+            <button onClick={() => refetchQueries()}>Refetch queries</button>
+            <button onClick={() => writeQuery()}>Update word</button>
+          </div>
+        );
+      };
+
+      const ParentComponent: React.FC = () => {
+        return (
+          <MockedProvider link={link} cache={cache}>
+            <div>
+              <ChildComponent />
+            </div>
+          </MockedProvider>
+        );
+      };
+
+      render(<ParentComponent />);
+
+      await screen.findByText("Data: foo");
+      await userEvent.click(screen.getByRole('button', { name: /refetch queries/i }));
+      expect(onCompleted).toBeCalledTimes(1);
+      await screen.findByText("Data: bar");
+      await userEvent.click(screen.getByRole('button', { name: /update word/i }));
+      expect(onCompleted).toBeCalledTimes(1);
+      await screen.findByText("Data: baz");
+      expect(onCompleted).toBeCalledTimes(1);
+    });
+
+    it("onCompleted should execute on cache writes after initial query execution with notifyOnNetworkStatusChange: true", async () => {
+      const query = gql`
+        {
+          hello
+        }
+      `;
+      const mocks = [
+        {
+          request: { query },
+          result: { data: { hello: "foo" } },
+        },
+        {
+          request: { query },
+          result: { data: { hello: "bar" } },
+        },
+      ];
+      const link = new MockLink(mocks);
+      const cache = new InMemoryCache();
+      const onCompleted = jest.fn();
+
+      const ChildComponent: React.FC = () => {
+        const { data, client } = useQuery(
+          query,
+          {
+            onCompleted,
+            notifyOnNetworkStatusChange: true
+          }
+        );
+        function refetchQueries() {
+          client.refetchQueries({ include: 'active' })
+        }
+        function writeQuery() {
+          client.writeQuery({ query, data: { hello: 'baz'}})
+        }
+        return (
+          <div>
+            <span>Data: {data?.hello}</span>
+            <button onClick={() => refetchQueries()}>Refetch queries</button>
+            <button onClick={() => writeQuery()}>Update word</button>
+          </div>
+        );
+      };
+
+      const ParentComponent: React.FC = () => {
+        return (
+          <MockedProvider link={link} cache={cache}>
+            <div>
+              <ChildComponent />
+            </div>
+          </MockedProvider>
+        );
+      };
+
+      render(<ParentComponent />);
+
+      await screen.findByText("Data: foo");
+      expect(onCompleted).toBeCalledTimes(1);
+      await userEvent.click(screen.getByRole('button', { name: /refetch queries/i }));
+      // onCompleted increments when refetch occurs since we're hitting the network...
+      expect(onCompleted).toBeCalledTimes(2);
+      await screen.findByText("Data: bar");
+      await userEvent.click(screen.getByRole('button', { name: /update word/i }));
+      // but not on direct cache write, since there's no network request to complete
+      expect(onCompleted).toBeCalledTimes(2);
+      await screen.findByText("Data: baz");
+      expect(onCompleted).toBeCalledTimes(2);
+    });
   });
 
   describe('Optimistic data', () => {

src/react/hooks/useQuery.ts

@@ -498,16 +498,23 @@ class InternalState<TData, TVariables> {
     // Calling state.setResult always triggers an update, though some call sites
     // perform additional equality checks before committing to an update.
     this.forceUpdate();
-    this.handleErrorOrCompleted(nextResult);
+    this.handleErrorOrCompleted(nextResult, previousResult);
   }
 
-  private handleErrorOrCompleted(result: ApolloQueryResult<TData>) {
+  private handleErrorOrCompleted(
+    result: ApolloQueryResult<TData>,
+    previousResult?: ApolloQueryResult<TData>
+  ) {
     if (!result.loading) {
       // wait a tick in case we are in the middle of rendering a component
       Promise.resolve().then(() => {
         if (result.error) {
           this.onError(result.error);
-        } else if (result.data) {
+        } else if (
+          result.data &&
+          previousResult?.networkStatus !== result.networkStatus &&
+          result.networkStatus === NetworkStatus.ready
+        ) {
           this.onCompleted(result.data);
         }
       }).catch(error => {