diff --git a/src/core/ObservableQuery.ts b/src/core/ObservableQuery.ts index 1bc872da4c6..cc682d0c6c8 100644 --- a/src/core/ObservableQuery.ts +++ b/src/core/ObservableQuery.ts @@ -270,7 +270,6 @@ export class ObservableQuery< // terminates after a complete cache read, we can assume the next result // we receive will have NetworkStatus.ready and !loading. if ( - diff.complete && result.networkStatus === NetworkStatus.loading && (fetchPolicy === 'cache-first' || fetchPolicy === 'cache-only') diff --git a/src/react/hoc/__tests__/queries/loading.test.tsx b/src/react/hoc/__tests__/queries/loading.test.tsx index cf28381e83c..3dca1b14b62 100644 --- a/src/react/hoc/__tests__/queries/loading.test.tsx +++ b/src/react/hoc/__tests__/queries/loading.test.tsx @@ -419,7 +419,7 @@ describe('[queries] loading', () => { "network-only", "cache-first", ]); - expect(count).toBe(6); + expect(count).toBe(5); }).then(resolve, reject); }); diff --git a/src/react/hooks/__tests__/useQuery.test.tsx b/src/react/hooks/__tests__/useQuery.test.tsx index 1b023b058ec..b8d7f9dcd40 100644 --- a/src/react/hooks/__tests__/useQuery.test.tsx +++ b/src/react/hooks/__tests__/useQuery.test.tsx @@ -1,5 +1,5 @@ import { RenderResult } from "@testing-library/react-hooks/src/types"; -import React, { Fragment, useEffect, useState } from 'react'; +import React, { Fragment, ReactNode, useEffect, useState } from 'react'; import { DocumentNode, GraphQLError } from 'graphql'; import gql from 'graphql-tag'; import { act } from 'react-dom/test-utils'; @@ -4925,6 +4925,200 @@ describe('useQuery Hook', () => { }); }); + // https://github.com/apollographql/apollo-client/issues/10222 + describe('regression test issue #10222', () => { + it('maintains initial fetch policy when component unmounts and remounts', async () => { + let helloCount = 1; + const query = gql`{ hello }`; + const link = new ApolloLink(() => { + return new Observable(observer => { + const timer = setTimeout(() => { + console.log('test observer.next', helloCount) + observer.next({ data: { hello: `hello ${helloCount++}` } }); + observer.complete(); + }, 50); + + return () => { + clearTimeout(timer); + } + }) + }) + + const cache = new InMemoryCache(); + + const client = new ApolloClient({ + link, + cache + }); + + let setShow: Function + const Toggler = ({ children }: { children: ReactNode }) => { + const [show, _setShow] = useState(true); + setShow = _setShow; + + return show ? <>{children} : null; + } + + const counts = { mount: 0, unmount: 0 }; + + const { result, waitForNextUpdate } = renderHook( + () => { + useEffect(() => { + counts.mount++; + + return () => { + counts.unmount++; + } + }, []); + + const result = useQuery(query, { + fetchPolicy: 'network-only', + nextFetchPolicy: 'cache-first' + }); + + return result + }, + { + wrapper: ({ children }) => ( + + {children} + + ), + }, + ); + + expect(counts).toEqual({ mount: 1, unmount: 0 }); + expect(result.current.loading).toBe(true); + expect(result.current.data).toBeUndefined(); + + await waitForNextUpdate(); + + expect(result.current.loading).toBe(false); + expect(result.current.data).toEqual({ hello: 'hello 1' }); + expect(cache.readQuery({ query })).toEqual({ hello: 'hello 1' }) + + act(() => { + setShow(false); + }); + + expect(counts).toEqual({ mount: 1, unmount: 1 }); + + act(() => { + setShow(true); + }); + + expect(counts).toEqual({ mount: 2, unmount: 1 }); + + expect(result.current.loading).toBe(true); + expect(result.current.data).toBeUndefined(); + + await waitForNextUpdate(); + + expect(result.current.loading).toBe(false); + expect(result.current.data).toEqual({ hello: 'hello 2' }); + expect(cache.readQuery({ query })).toEqual({ hello: 'hello 2' }) + }); + + it('can handle nextFetchPolicy as a function', async () => { + let helloCount = 1; + const query = gql`{ hello }`; + const link = new ApolloLink(() => { + return new Observable(observer => { + const timer = setTimeout(() => { + console.log('test observer.next', helloCount) + observer.next({ data: { hello: `hello ${helloCount++}` } }); + observer.complete(); + }, 50); + + return () => { + clearTimeout(timer); + } + }) + }) + + const cache = new InMemoryCache(); + + const client = new ApolloClient({ + link, + cache + }); + + let setShow: Function + const Toggler = ({ children }: { children: ReactNode }) => { + const [show, _setShow] = useState(true); + setShow = _setShow; + + return show ? <>{children} : null; + } + + const counts = { mount: 0, unmount: 0 }; + + const { result, waitForNextUpdate } = renderHook( + () => { + useEffect(() => { + counts.mount++; + + return () => { + counts.unmount++; + } + }, []); + + const result = useQuery(query, { + fetchPolicy: 'network-only', + nextFetchPolicy: (currentFetchPolicy, { reason }) => { + if (reason === 'after-fetch') { + return 'cache-first' + } + + return currentFetchPolicy; + } + }); + + return result + }, + { + wrapper: ({ children }) => ( + + {children} + + ), + }, + ); + + expect(counts).toEqual({ mount: 1, unmount: 0 }); + expect(result.current.loading).toBe(true); + expect(result.current.data).toBeUndefined(); + + await waitForNextUpdate(); + + expect(result.current.loading).toBe(false); + expect(result.current.data).toEqual({ hello: 'hello 1' }); + expect(cache.readQuery({ query })).toEqual({ hello: 'hello 1' }) + + act(() => { + setShow(false); + }); + + expect(counts).toEqual({ mount: 1, unmount: 1 }); + + act(() => { + setShow(true); + }); + + expect(counts).toEqual({ mount: 2, unmount: 1 }); + + expect(result.current.loading).toBe(true); + expect(result.current.data).toBeUndefined(); + + await waitForNextUpdate(); + + expect(result.current.loading).toBe(false); + expect(result.current.data).toEqual({ hello: 'hello 2' }); + expect(cache.readQuery({ query })).toEqual({ hello: 'hello 2' }) + }); + }); + + describe('defer', () => { it('should handle deferred queries', async () => { const query = gql` @@ -5542,6 +5736,7 @@ describe('useQuery Hook', () => { expect(result.current.loading).toBe(true); expect(result.current.data).toBe(undefined); + expect(result.current.networkStatus).toBe(NetworkStatus.loading); setTimeout(() => { link.simulateResult({ result: { @@ -5621,6 +5816,7 @@ describe('useQuery Hook', () => { expect(result.current.label).toBe(undefined); // @ts-ignore expect(result.current.extensions).toBe(undefined); + expect(result.current.networkStatus).toBe(NetworkStatus.error); expect(result.current.error).toBeInstanceOf(ApolloError); expect(result.current.error!.message).toBe('homeWorld for character with ID 1000 could not be fetched.'); diff --git a/src/react/hooks/useQuery.ts b/src/react/hooks/useQuery.ts index 2bf72a83329..8200087cbec 100644 --- a/src/react/hooks/useQuery.ts +++ b/src/react/hooks/useQuery.ts @@ -27,6 +27,8 @@ import { QueryResult, ObservableQueryFields, } from '../types/types'; +import { isNetworkRequestInFlight } from '../../core/networkStatus'; +import { logMissingFieldErrors } from '../../core/ObservableQuery'; import { DocumentType, verifyDocumentType } from '../parser'; import { useApolloClient } from './useApolloClient'; @@ -140,13 +142,85 @@ class InternalState { return () => {}; } - const onNext = () => { + const onNext = (result: ApolloQueryResult) => { const previousResult = this.result; - // We use `getCurrentResult()` instead of the onNext argument because - // the values differ slightly. Specifically, loading results will have - // an empty object for data instead of `undefined` for some reason. - const result = obsQuery.getCurrentResult(); - // Make sure we're not attempting to re-render similar results + const diff = obsQuery['queryInfo'].getDiff(); + + // Unfortunately the result passed into `onNext` doesn't seem to have + // canonization applied, whereas the `diff.result` does. Theoretically + // it should as result.data when read from the cache also uses the + // diff (https://github.com/apollographql/apollo-client/blob/6875d3ced43162557cbd507558dfbdbc37512a69/src/core/QueryManager.ts#L1392) + // but I can't seem to figure out why it isn't applied. This patches + // the behavior to ensure we retain it. + if (obsQuery.options.canonizeResults) { + result.data = diff.result; + } + + // For some reason, we can't always trust the result passed to onNext + // when the result is part of an optimistic transaction, so we need + // to rely on the diff result which has the correct data. This is + // difficult to track down why onNext is called with the wrong value. + if (diff.fromOptimisticTransaction) { + result.data = diff.result; + } + + // Previously, this code called `obsQuery.getCurrentResult()` to get + // the result returned in `useQuery` rather than relying on the + // argument passed to `onNext`. Unfortunately the `result` passed as + // the argument doesn't always resemble the result returned from + // `obsQuery.getCurrentResult()`. Because of this, we have to patch + // some of that behavior here to ensure the result maintains the same + // information. + // + // Why can't we use `obsQuery.getCurrentResult()`? #9823 fixed an + // issue with `skip` and the fetch policy. In that fix, the + // `nextFetchPolicy` was changed to be synchronously set as soon as we + // kick off the query. Unfortunately this has the side effect that + // `obsQuery.getCurrentResult()` uses the new fetch policy too early. + // In some cases, this meant we'd return cached data when we didn't + // mean to, such as when a component is unmounted, then mounted again + // (see #10222). + // + // We should really look to refactor this code out of here for AC v4. + // This behavior should really be patched in QueryManager, but I was + // afraid that developers might rely on the existing behavior + // (such as returning empty objects instead of setting those as + // undefined). As such, I decided to patch it only in useQuery. + if (!diff.complete) { + if (!hasOwnProperty.call(result, 'partial')) { + result.partial = true; + } + + if ( + // Deferred queries by nature return partial results, so we want + // to ensure we return that data, even if `returnPartialData` is + // set to false. Therefore we only reset `data` to `undefined` + // when the request is in flight. + isNetworkRequestInFlight(result.networkStatus) && + hasOwnProperty.call(result, 'data') && + !this.getObsQueryOptions().returnPartialData + ) { + result.data = void 0 as any; + } + + // Retain logging used in `obsQuery.getCurrentResult()` when this + // was switched over to read the result from the argument. + if ( + __DEV__ && + !obsQuery.options.partialRefetch && + !result.loading && + !result.data && + !result.error + ) { + logMissingFieldErrors(diff.missing); + } + } + + if (equal(result.data, {})) { + result.data = void 0 as any; + } + + // make sure we're not attempting to re-render similar results if ( previousResult && previousResult.loading === result.loading &&