From 01376a3ad09e1a806d7517cad44502b4355456af Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Tue, 22 Oct 2019 14:12:58 -0400 Subject: [PATCH] Stop reading from cache in ObservableQuery#getCurrentResult. Since getCurrentResult must return its result synchronously, reading from the cache in this method prevents us from making cache reads asynchronous in the future. This may be a breaking change for code that attempts to read cache data from an ObservableQuery immediately after calling watchQuery, but that's a behavior that should never have been enforced by our test suite in the first place. --- src/core/ObservableQuery.ts | 73 +++++++----------- src/core/QueryManager.ts | 2 +- src/core/__tests__/ObservableQuery.ts | 83 ++++++++++----------- src/react/hooks/__tests__/useQuery.test.tsx | 21 +++--- 4 files changed, 74 insertions(+), 105 deletions(-) diff --git a/src/core/ObservableQuery.ts b/src/core/ObservableQuery.ts index 4fb85a35429..0d6ddccac4b 100644 --- a/src/core/ObservableQuery.ts +++ b/src/core/ObservableQuery.ts @@ -137,46 +137,44 @@ export class ObservableQuery< }); } - /** - * Return the result of the query from the local cache as well as some fetching status - * `loading` and `networkStatus` allow to know if a request is in flight - * `partial` lets you know if the result from the local cache is complete or partial - * @return {data: Object, error: ApolloError, loading: boolean, networkStatus: number, partial: boolean} - */ public getCurrentResult(): ApolloCurrentQueryResult { - if (this.isTornDown) { - const { lastResult } = this; - return { - data: !this.lastError && lastResult && lastResult.data || void 0, - error: this.lastError, - loading: false, - networkStatus: NetworkStatus.error, - }; - } - - const { data, partial } = this.queryManager.getCurrentQueryResult(this); - const queryStoreValue = this.queryManager.queryStore.get(this.queryId); - let result: ApolloQueryResult; - + const { lastResult, lastError } = this; const { fetchPolicy } = this.options; - const isNetworkFetchPolicy = fetchPolicy === 'network-only' || fetchPolicy === 'no-cache'; + const networkStatus = + lastError ? NetworkStatus.error : + lastResult ? lastResult.networkStatus : + isNetworkFetchPolicy ? NetworkStatus.loading : + NetworkStatus.ready; + + const result: ApolloCurrentQueryResult = { + data: !lastError && lastResult && lastResult.data || void 0, + error: this.lastError, + loading: isNetworkRequestInFlight(networkStatus), + networkStatus, + partial: false, + }; + + if (this.isTornDown) { + return result; + } + + const queryStoreValue = this.queryManager.queryStore.get(this.queryId); if (queryStoreValue) { const { networkStatus } = queryStoreValue; if (hasError(queryStoreValue, this.options.errorPolicy)) { - return { + return Object.assign(result, { data: void 0, - loading: false, networkStatus, error: new ApolloError({ graphQLErrors: queryStoreValue.graphQLErrors, networkError: queryStoreValue.networkError, }), - }; + }); } // Variables might have been added dynamically at query time, when @@ -192,38 +190,19 @@ export class ObservableQuery< this.variables = this.options.variables; } - result = { - data, + Object.assign(result, { loading: isNetworkRequestInFlight(networkStatus), networkStatus, - } as ApolloQueryResult; + }); if (queryStoreValue.graphQLErrors && this.options.errorPolicy === 'all') { result.errors = queryStoreValue.graphQLErrors; } - - } else { - // We need to be careful about the loading state we show to the user, to try - // and be vaguely in line with what the user would have seen from .subscribe() - // but to still provide useful information synchronously when the query - // will not end up hitting the server. - // See more: https://github.com/apollostack/apollo-client/issues/707 - // Basically: is there a query in flight right now (modolo the next tick)? - const loading = isNetworkFetchPolicy || - (partial && fetchPolicy !== 'cache-only'); - - result = { - data, - loading, - networkStatus: loading ? NetworkStatus.loading : NetworkStatus.ready, - } as ApolloQueryResult; } - if (!partial) { - this.updateLastResult({ ...result, stale: false }); - } + this.updateLastResult({ ...result, stale: false }); - return { ...result, partial }; + return result; } // Compares newResult to the snapshot we took of this.lastResult when it was diff --git a/src/core/QueryManager.ts b/src/core/QueryManager.ts index 29a4c674c48..c725346d172 100644 --- a/src/core/QueryManager.ts +++ b/src/core/QueryManager.ts @@ -1047,7 +1047,7 @@ export class QueryManager { this.queries.delete(queryId); } - public getCurrentQueryResult( + private getCurrentQueryResult( observableQuery: ObservableQuery, optimistic: boolean = true, ): { diff --git a/src/core/__tests__/ObservableQuery.ts b/src/core/__tests__/ObservableQuery.ts index b096c0be705..cbe578bb063 100644 --- a/src/core/__tests__/ObservableQuery.ts +++ b/src/core/__tests__/ObservableQuery.ts @@ -306,11 +306,8 @@ describe('ObservableQuery', () => { } else if (handleCount === 3) { expect(stripSymbols(result.data)).toEqual(data2); // go back to first set of variables - await observable.setOptions({ variables }); - const current = observable.getCurrentResult(); + const current = await observable.setOptions({ variables }); expect(stripSymbols(current.data)).toEqual(data); - const secondCurrent = observable.getCurrentResult(); - expect(current.data).toEqual(secondCurrent.data); resolve(); } }); @@ -1461,7 +1458,7 @@ describe('ObservableQuery', () => { loading: true, data: undefined, networkStatus: 1, - partial: true, + partial: false, }); setTimeout( @@ -1470,39 +1467,13 @@ describe('ObservableQuery', () => { loading: true, data: undefined, networkStatus: 1, - partial: true, + partial: false, }); }), 0, ); }); - itAsync('returns results from the store immediately', (resolve, reject) => { - const queryManager = mockQueryManager(reject, { - request: { query, variables }, - result: { data: dataOne }, - }); - - return queryManager.query({ query, variables }).then((result: any) => { - expect(stripSymbols(result)).toEqual({ - data: dataOne, - loading: false, - networkStatus: 7, - stale: false, - }); - const observable = queryManager.watchQuery({ - query, - variables, - }); - expect(stripSymbols(observable.getCurrentResult())).toEqual({ - data: dataOne, - loading: true, - networkStatus: NetworkStatus.loading, - partial: false, - }); - }).then(resolve, reject); - }); - itAsync('returns errors from the store immediately', (resolve, reject) => { const queryManager = mockQueryManager(reject, { request: { query, variables }, @@ -1592,7 +1563,7 @@ describe('ObservableQuery', () => { }).then(resolve, reject); }); - itAsync('returns partial data from the store immediately', (resolve, reject) => { + itAsync('returns partial data from the store', (resolve, reject) => { const superQuery = gql` query superQuery($id: ID!) { people_one(id: $id) { @@ -1629,10 +1600,10 @@ describe('ObservableQuery', () => { }); expect(observable.getCurrentResult()).toEqual({ - data: dataOne, + data: void 0, loading: true, networkStatus: 1, - partial: true, + partial: false, }); // we can use this to trigger the query @@ -1647,19 +1618,28 @@ describe('ObservableQuery', () => { if (handleCount === 1) { expect(subResult).toEqual({ - data: dataOne, + data: void 0, loading: true, networkStatus: 1, stale: false, }); } else if (handleCount === 2) { + expect(subResult).toEqual({ + data: dataOne, + loading: false, + networkStatus: 7, + stale: false, + }); + + } else if (handleCount === 3) { expect(subResult).toEqual({ data: superDataOne, loading: false, networkStatus: 7, stale: false, }); + resolve(); } }); @@ -1698,6 +1678,7 @@ describe('ObservableQuery', () => { loading, networkStatus, } = observable.getCurrentResult(); + expect(subResult).toEqual({ data, loading, @@ -1705,13 +1686,22 @@ describe('ObservableQuery', () => { stale: false, }); - if (handleCount === 2) { + if (handleCount === 1) { + expect(stripSymbols(subResult)).toEqual({ + data: void 0, + loading: true, + networkStatus: NetworkStatus.loading, + stale: false, + }); + + } else if (handleCount === 2) { expect(stripSymbols(subResult)).toEqual({ data: dataTwo, loading: false, - networkStatus: 7, + networkStatus: NetworkStatus.ready, stale: false, }); + resolve(); } }); @@ -1737,6 +1727,7 @@ describe('ObservableQuery', () => { variables, fetchPolicy: 'no-cache', }); + expect(stripSymbols(observable.getCurrentResult())).toEqual({ data: undefined, loading: true, @@ -1750,14 +1741,16 @@ describe('ObservableQuery', () => { loading, networkStatus, } = observable.getCurrentResult(); - expect(subResult).toEqual({ - data, - loading, - networkStatus, - stale: false, - }); - if (handleCount === 2) { + if (handleCount === 1) { + expect(subResult).toEqual({ + data, + loading, + networkStatus, + partial: false, + stale: false, + }); + } else if (handleCount === 2) { expect(stripSymbols(subResult)).toEqual({ data: dataTwo, loading: false, diff --git a/src/react/hooks/__tests__/useQuery.test.tsx b/src/react/hooks/__tests__/useQuery.test.tsx index 5404b11cfad..e39653eb471 100644 --- a/src/react/hooks/__tests__/useQuery.test.tsx +++ b/src/react/hooks/__tests__/useQuery.test.tsx @@ -382,42 +382,39 @@ describe('useQuery Hook', () => { cache: new InMemoryCache() }); - const onErrorMock = jest.fn(); + let onError; + const onErrorPromise = new Promise(resolve => onError = resolve); let renderCount = 0; const Component = () => { const { loading, error, refetch, data, networkStatus } = useQuery( query, { - onError: onErrorMock, + onError, notifyOnNetworkStatusChange: true } ); - switch (renderCount) { - case 0: + switch (++renderCount) { + case 1: expect(loading).toBeTruthy(); break; - case 1: + case 2: expect(loading).toBeFalsy(); expect(error).toBeDefined(); expect(error!.message).toEqual('Network error: Oh no!'); - setTimeout(() => { - expect(onErrorMock.mock.calls.length).toBe(1); - refetch(); - }); + onErrorPromise.then(() => refetch()); break; - case 2: + case 3: expect(loading).toBeTruthy(); break; - case 3: + case 4: expect(loading).toBeFalsy(); expect(data).toEqual(resultData); break; default: // Do nothing } - renderCount += 1; return null; };