From 2f593943dc539709cd662d9441a4b5a697d33ea7 Mon Sep 17 00:00:00 2001 From: Alessia Bellisario Date: Thu, 27 Oct 2022 14:35:13 -0400 Subject: [PATCH] fix(regression): avoid calling `useQuery` `onCompleted` for cache writes (#10229) --- .changeset/early-pens-retire.md | 5 + CHANGELOG.md | 7 + src/react/hooks/__tests__/useQuery.test.tsx | 194 +++++++++++++++++++- src/react/hooks/useQuery.ts | 13 +- 4 files changed, 214 insertions(+), 5 deletions(-) create mode 100644 .changeset/early-pens-retire.md diff --git a/.changeset/early-pens-retire.md b/.changeset/early-pens-retire.md new file mode 100644 index 00000000000..1eb7fe3d95b --- /dev/null +++ b/.changeset/early-pens-retire.md @@ -0,0 +1,5 @@ +--- +"@apollo/client": patch +--- + +Avoid calling `useQuery` `onCompleted` for cache writes diff --git a/CHANGELOG.md b/CHANGELOG.md index 0fad22ff6aa..5ad783aaf99 100644 --- a/CHANGELOG.md +++ b/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.
+ [@alessbell](https://github.com/alessbell) in [#10229](https://github.com/apollographql/apollo-client/pull/10229) + ## Apollo Client 3.7.2 (2022-12-06) ### Improvements diff --git a/src/react/hooks/__tests__/useQuery.test.tsx b/src/react/hooks/__tests__/useQuery.test.tsx index 1b023b058ec..c0a53235473 100644 --- a/src/react/hooks/__tests__/useQuery.test.tsx +++ b/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 }) => ( + + {children} + + ), + }, + ); + + 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 ( +
+ Data: {data?.hello} + + +
+ ); + }; + + const ParentComponent: React.FC = () => { + return ( + +
+ +
+ + ); + }; + + render(); + + 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 ( +
+ Data: {data?.hello} + + +
+ ); + }; + + const ParentComponent: React.FC = () => { + return ( + +
+ +
+ + ); + }; + + render(); + + 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', () => { diff --git a/src/react/hooks/useQuery.ts b/src/react/hooks/useQuery.ts index 2bf72a83329..0758499c380 100644 --- a/src/react/hooks/useQuery.ts +++ b/src/react/hooks/useQuery.ts @@ -498,16 +498,23 @@ class InternalState { // 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) { + private handleErrorOrCompleted( + result: ApolloQueryResult, + previousResult?: ApolloQueryResult + ) { 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 => {