From ecfb48f3cb48eaf85d5d464d8e1a20a186e1ef26 Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Tue, 14 Jan 2020 13:54:30 -0500 Subject: [PATCH 1/3] Read from cache again in ObservableQuery#getCurrentResult. This partially reverts two commits from PR #5565, preserving related improvements that have happened in the meantime: * Revert "Remove partial field from ApolloCurrentQueryResult type." This reverts commit ca2cef659210dc77b21f251d97d69eefcc8f18e0. * Revert "Stop reading from cache in ObservableQuery#getCurrentResult." This reverts commit 01376a3ad09e1a806d7517cad44502b4355456af. One of the motivations behind the original changes was to lay the foundations for eventually supporting asynchronous (Promise-based) cache reads, but that form of asynchrony is no longer a goal of Apollo Client 3.0 or any planned future version, so there's not as much benefit to weakening getCurrentResult in this way. On the other hand, allowing getCurrentResult to read synchronously from the cache ensures that custom field read functions can return new results immediately, which helps with issues like #5782. Immediate cache reads have also been the behavior of Apollo Client for much longer than #5565, which makes this change appealing from an ease-of-upgrading perspective. --- src/core/ObservableQuery.ts | 22 ++++++--- src/core/QueryManager.ts | 2 +- src/core/__tests__/ObservableQuery.ts | 70 ++++++++++++++++----------- src/react/data/QueryData.ts | 3 +- 4 files changed, 62 insertions(+), 35 deletions(-) diff --git a/src/core/ObservableQuery.ts b/src/core/ObservableQuery.ts index 7db43eaeceb..49b8a791d3e 100644 --- a/src/core/ObservableQuery.ts +++ b/src/core/ObservableQuery.ts @@ -20,6 +20,7 @@ import { isNonEmptyArray } from '../utilities/common/arrays'; export type ApolloCurrentQueryResult = ApolloQueryResult & { error?: ApolloError; + partial?: boolean; }; export interface FetchMoreOptions< @@ -131,8 +132,12 @@ export class ObservableQuery< } public getCurrentResult(): ApolloCurrentQueryResult { - const { lastResult, lastError } = this; - const { fetchPolicy } = this.options; + const { + lastResult, + lastError, + options: { fetchPolicy }, + } = this; + const isNetworkFetchPolicy = fetchPolicy === 'network-only' || fetchPolicy === 'no-cache'; @@ -145,7 +150,7 @@ export class ObservableQuery< const result: ApolloCurrentQueryResult = { data: !lastError && lastResult && lastResult.data || void 0, - error: this.lastError, + error: lastError, loading: isNetworkRequestInFlight(networkStatus), networkStatus, stale: lastResult ? lastResult.stale : false, @@ -155,6 +160,9 @@ export class ObservableQuery< return result; } + const { data, partial } = this.queryManager.getCurrentQueryResult(this); + Object.assign(result, { data, partial }); + const queryStoreValue = this.queryManager.queryStore.get(this.queryId); if (queryStoreValue) { const { networkStatus } = queryStoreValue; @@ -176,11 +184,10 @@ export class ObservableQuery< // the original `ObservableQuery`. We'll update the observable query // variables here to match, so retrieving from the cache doesn't fail. if (queryStoreValue.variables) { - this.options.variables = { + this.variables = this.options.variables = { ...this.options.variables, ...(queryStoreValue.variables as TVariables), }; - this.variables = this.options.variables; } Object.assign(result, { @@ -193,7 +200,10 @@ export class ObservableQuery< } } - this.updateLastResult(result); + if (!partial) { + result.stale = false; + this.updateLastResult(result); + } return result; } diff --git a/src/core/QueryManager.ts b/src/core/QueryManager.ts index f6cb7a57702..391476e2dc2 100644 --- a/src/core/QueryManager.ts +++ b/src/core/QueryManager.ts @@ -1052,7 +1052,7 @@ export class QueryManager { this.queries.delete(queryId); } - private getCurrentQueryResult( + public getCurrentQueryResult( observableQuery: ObservableQuery, optimistic: boolean = true, ): { diff --git a/src/core/__tests__/ObservableQuery.ts b/src/core/__tests__/ObservableQuery.ts index 07dfe0dbfcd..5c8e3dbe9e1 100644 --- a/src/core/__tests__/ObservableQuery.ts +++ b/src/core/__tests__/ObservableQuery.ts @@ -1450,6 +1450,7 @@ describe('ObservableQuery', () => { loading: false, networkStatus: 7, stale: false, + partial: false, }); resolve(); }); @@ -1459,6 +1460,7 @@ describe('ObservableQuery', () => { data: undefined, networkStatus: 1, stale: false, + partial: true, }); setTimeout( @@ -1468,12 +1470,40 @@ describe('ObservableQuery', () => { data: undefined, networkStatus: 1, stale: false, + partial: true, }); }), 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, + stale: false, + partial: false, + }); + }).then(resolve, reject); + }); + itAsync('returns errors from the store immediately', (resolve, reject) => { const queryManager = mockQueryManager(reject, { request: { query, variables }, @@ -1563,7 +1593,7 @@ describe('ObservableQuery', () => { }).then(resolve, reject); }); - itAsync('returns partial data from the store', (resolve, reject) => { + itAsync('returns partial data from the store immediately', (resolve, reject) => { const superQuery = gql` query superQuery($id: ID!) { people_one(id: $id) { @@ -1600,10 +1630,11 @@ describe('ObservableQuery', () => { }); expect(observable.getCurrentResult()).toEqual({ - data: void 0, + data: dataOne, loading: true, networkStatus: 1, stale: false, + partial: true, }); // we can use this to trigger the query @@ -1618,28 +1649,19 @@ describe('ObservableQuery', () => { if (handleCount === 1) { expect(subResult).toEqual({ - data: void 0, + data: dataOne, 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(); } }); @@ -1670,6 +1692,7 @@ describe('ObservableQuery', () => { loading: true, networkStatus: 1, stale: false, + partial: false, }); subscribeAndCount(reject, observable, (handleCount, subResult) => { @@ -1679,29 +1702,21 @@ describe('ObservableQuery', () => { networkStatus, } = observable.getCurrentResult(); - expect(subResult).toEqual({ - data, - loading, - networkStatus, - stale: false, - }); - if (handleCount === 1) { - expect(stripSymbols(subResult)).toEqual({ - data: void 0, - loading: true, - networkStatus: NetworkStatus.loading, + expect(subResult).toEqual({ + data, + loading, + networkStatus, stale: false, + partial: false, }); - } else if (handleCount === 2) { expect(stripSymbols(subResult)).toEqual({ data: dataTwo, loading: false, - networkStatus: NetworkStatus.ready, + networkStatus: 7, stale: false, }); - resolve(); } }); @@ -1727,12 +1742,12 @@ describe('ObservableQuery', () => { variables, fetchPolicy: 'no-cache', }); - expect(stripSymbols(observable.getCurrentResult())).toEqual({ data: undefined, loading: true, networkStatus: 1, stale: false, + partial: false, }); subscribeAndCount(reject, observable, (handleCount, subResult) => { @@ -1748,6 +1763,7 @@ describe('ObservableQuery', () => { loading, networkStatus, stale: false, + partial: false, }); } else if (handleCount === 2) { expect(stripSymbols(subResult)).toEqual({ diff --git a/src/react/data/QueryData.ts b/src/react/data/QueryData.ts index fe56dc4f3ef..a1ed59e5c06 100644 --- a/src/react/data/QueryData.ts +++ b/src/react/data/QueryData.ts @@ -353,7 +353,7 @@ export class QueryData extends OperationData { } else { // Fetch the current result (if any) from the store. const currentResult = this.currentObservable.query!.getCurrentResult(); - const { loading, networkStatus, errors } = currentResult; + const { loading, partial, networkStatus, errors } = currentResult; let { error, data } = currentResult; // Until a set naming convention for networkError and graphQLErrors is @@ -390,6 +390,7 @@ export class QueryData extends OperationData { const { partialRefetch } = options; if ( partialRefetch && + partial && (!data || Object.keys(data).length === 0) && fetchPolicy !== 'cache-only' ) { From a891b362c486b9bd04424532c077f33cde50e676 Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Tue, 14 Jan 2020 18:31:47 -0500 Subject: [PATCH 2/3] Increase bundlesize limit to 24.05kB. --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index e0f261b2b15..2423a843df1 100644 --- a/package.json +++ b/package.json @@ -48,7 +48,7 @@ { "name": "apollo-client", "path": "./dist/apollo-client.cjs.min.js", - "maxSize": "24 kB" + "maxSize": "24.05 kB" } ], "peerDependencies": { From b4d8b03c8e7abd3a67b409227a24ad57300e2edc Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Tue, 14 Jan 2020 18:34:53 -0500 Subject: [PATCH 3/3] Remove note about PR #5565 from CHANGELOG.md. --- CHANGELOG.md | 3 --- 1 file changed, 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c0325c15976..f2160d1fa9b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -54,9 +54,6 @@ - `ApolloClient` is now only available as a named export. The default `ApolloClient` export has been removed.
[@hwillson](https://github.com/hwillson) in [#5425](https://github.com/apollographql/apollo-client/pull/5425) -- The `ObservableQuery#getCurrentResult` method no longer falls back to reading from the cache, so calling it immediately after `client.watchQuery` will consistently return a `loading: true` result. When the `fetchPolicy` permits cached results, those results will be delivered via the `next` method of the `ObservableQuery`, and can be obtained by `getCurrentResult` after they have been delivered. This change prevents race conditions where the initial behavior of one query could depend on the timing of cache writes associated with other queries.
- [@benjamn](https://github.com/benjamn) in [#5565](https://github.com/apollographql/apollo-client/pull/5565) - - The `QueryOptions`, `MutationOptions`, and `SubscriptionOptions` React Apollo interfaces have been renamed to `QueryDataOptions`, `MutationDataOptions`, and `SubscriptionDataOptions` (to avoid conflicting with similarly named and exported Apollo Client interfaces). - `InMemoryCache` will no longer merge the fields of written objects unless the objects are known to have the same identity, and the values of fields with the same name will not be recursively merged unless a custom `merge` function is defined by a field policy for that field, within a type policy associated with the `__typename` of the parent object.