From 513d733cf0162c86d2b7d56547c20bb03287a472 Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Thu, 9 Jul 2020 14:14:49 -0400 Subject: [PATCH 01/16] Eliminate ApolloCurrentQueryResult in favor of ApolloQueryResult. --- src/core/ObservableQuery.ts | 11 +++-------- src/core/QueryManager.ts | 1 + src/core/__tests__/ObservableQuery.ts | 2 ++ src/core/__tests__/QueryManager/index.ts | 4 ++++ src/core/__tests__/fetchPolicies.ts | 2 ++ src/core/index.ts | 1 - src/core/types.ts | 6 ++++++ 7 files changed, 18 insertions(+), 9 deletions(-) diff --git a/src/core/ObservableQuery.ts b/src/core/ObservableQuery.ts index b76dc96bd5a..9700adec555 100644 --- a/src/core/ObservableQuery.ts +++ b/src/core/ObservableQuery.ts @@ -23,11 +23,6 @@ import { import { QueryStoreValue } from './QueryInfo'; import { Reobserver } from './Reobserver'; -export type ApolloCurrentQueryResult = ApolloQueryResult & { - error?: ApolloError; - partial?: boolean; -}; - export interface FetchMoreOptions< TData = any, TVariables = OperationVariables @@ -134,7 +129,7 @@ export class ObservableQuery< }); } - public getCurrentResult(): ApolloCurrentQueryResult { + public getCurrentResult(): ApolloQueryResult { const { lastResult, lastError, @@ -151,8 +146,8 @@ export class ObservableQuery< isNetworkFetchPolicy ? NetworkStatus.loading : NetworkStatus.ready; - const result: ApolloCurrentQueryResult = { - data: !lastError && lastResult && lastResult.data || void 0, + const result: ApolloQueryResult = { + ...(lastError ? null : lastResult), error: lastError, loading: isNetworkRequestInFlight(networkStatus), networkStatus, diff --git a/src/core/QueryManager.ts b/src/core/QueryManager.ts index 6d042a0c600..be8522c9ee4 100644 --- a/src/core/QueryManager.ts +++ b/src/core/QueryManager.ts @@ -975,6 +975,7 @@ export class QueryManager { data, loading: isNetworkRequestInFlight(networkStatus), networkStatus, + ...(diff.complete ? null : { partial: true }), } as ApolloQueryResult); if (this.transform(query).hasForcedResolvers) { diff --git a/src/core/__tests__/ObservableQuery.ts b/src/core/__tests__/ObservableQuery.ts index 16fb4430383..4382969608c 100644 --- a/src/core/__tests__/ObservableQuery.ts +++ b/src/core/__tests__/ObservableQuery.ts @@ -1176,6 +1176,7 @@ describe('ObservableQuery', () => { }, loading: true, networkStatus: NetworkStatus.loading, + partial: true, }); } else if (handleCount === 2) { expect(result).toEqual({ @@ -1597,6 +1598,7 @@ describe('ObservableQuery', () => { data: dataOne, loading: true, networkStatus: 1, + partial: true, }); } else if (handleCount === 2) { diff --git a/src/core/__tests__/QueryManager/index.ts b/src/core/__tests__/QueryManager/index.ts index 4d62062827d..c80caf9ec75 100644 --- a/src/core/__tests__/QueryManager/index.ts +++ b/src/core/__tests__/QueryManager/index.ts @@ -2128,6 +2128,7 @@ describe('QueryManager', () => { data: {}, loading: true, networkStatus: NetworkStatus.loading, + partial: true, }); }, result => { @@ -2202,6 +2203,7 @@ describe('QueryManager', () => { loading: true, networkStatus: NetworkStatus.loading, data: {}, + partial: true, }); } else if (count === 2) { expect(result).toEqual({ @@ -2220,6 +2222,7 @@ describe('QueryManager', () => { data: { info: {}, }, + partial: true, }); } else if (count === 4) { expect(result).toEqual({ @@ -2243,6 +2246,7 @@ describe('QueryManager', () => { loading: true, networkStatus: NetworkStatus.loading, data: {}, + partial: true, }); } else if (count === 2) { expect(result).toEqual({ diff --git a/src/core/__tests__/fetchPolicies.ts b/src/core/__tests__/fetchPolicies.ts index 87390d649f2..abf78b225e1 100644 --- a/src/core/__tests__/fetchPolicies.ts +++ b/src/core/__tests__/fetchPolicies.ts @@ -494,6 +494,7 @@ describe('cache-and-network', function() { data: {}, loading: true, networkStatus: NetworkStatus.setVariables, + partial: true, }); } else if (count === 3) { expect(result).toEqual({ @@ -520,6 +521,7 @@ describe('cache-and-network', function() { data: {}, loading: true, networkStatus: NetworkStatus.setVariables, + partial: true, }); } else if (count === 7) { expect(result).toEqual({ diff --git a/src/core/index.ts b/src/core/index.ts index d53ec7f6a03..12bc59f86ba 100644 --- a/src/core/index.ts +++ b/src/core/index.ts @@ -9,7 +9,6 @@ export { ObservableQuery, FetchMoreOptions, UpdateQueryOptions, - ApolloCurrentQueryResult, } from './ObservableQuery'; export { QueryBaseOptions, diff --git a/src/core/types.ts b/src/core/types.ts index f0f82d2aab3..93036512e58 100644 --- a/src/core/types.ts +++ b/src/core/types.ts @@ -1,6 +1,7 @@ import { DocumentNode, GraphQLError } from 'graphql'; import { FetchResult } from '../link/core'; +import { ApolloError } from '../errors'; import { QueryInfo } from './QueryInfo'; import { NetworkStatus } from './networkStatus'; import { Resolver } from './LocalState'; @@ -18,8 +19,13 @@ export type PureQueryOptions = { export type ApolloQueryResult = { data?: T; errors?: ReadonlyArray; + error?: ApolloError; loading: boolean; networkStatus: NetworkStatus; + // If result.data was read from the cache with missing fields, + // result.partial will be true. Otherwise, result.partial will be falsy + // (usually because the property is absent from the result object). + partial?: boolean; }; // This is part of the public API, people write these functions in `updateQueries`. From 01a899eb776b8d50dab434d708d40e1ed4579932 Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Tue, 21 Jul 2020 18:38:53 -0400 Subject: [PATCH 02/16] Stop using getCurrentQueryResult in updateQuery. Reducing the number of calls to getCurrentQueryResult from two to one will make it easier to collapse getCurrentQueryResult into getCurrentResult. --- src/core/ObservableQuery.ts | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/src/core/ObservableQuery.ts b/src/core/ObservableQuery.ts index 9700adec555..28107c88b69 100644 --- a/src/core/ObservableQuery.ts +++ b/src/core/ObservableQuery.ts @@ -503,8 +503,15 @@ once, rather than every time you call fetchMore.`); ) => TData, ): void { const { queryManager } = this; - const previousResult = this.getCurrentQueryResult(false).data; - const newResult = mapFn(previousResult!, { + const { result } = queryManager.cache.diff({ + query: this.options.query, + variables: this.variables, + previousResult: this.lastResult?.data, + returnPartialData: true, + optimistic: false, + }); + + const newResult = mapFn(result!, { variables: (this as any).variables, }); From c08043cb777646e3cb42e02c5e264390ab3c7379 Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Tue, 21 Jul 2020 19:59:08 -0400 Subject: [PATCH 03/16] Simplify ObservableQuery#getCurrentResult. --- src/core/ObservableQuery.ts | 46 ++----------------------------------- 1 file changed, 2 insertions(+), 44 deletions(-) diff --git a/src/core/ObservableQuery.ts b/src/core/ObservableQuery.ts index 28107c88b69..1326bbc59a8 100644 --- a/src/core/ObservableQuery.ts +++ b/src/core/ObservableQuery.ts @@ -18,9 +18,7 @@ import { WatchQueryOptions, FetchMoreQueryOptions, SubscribeToMoreOptions, - ErrorPolicy, } from './watchQueryOptions'; -import { QueryStoreValue } from './QueryInfo'; import { Reobserver } from './Reobserver'; export interface FetchMoreOptions< @@ -40,14 +38,6 @@ export interface UpdateQueryOptions { variables?: TVariables; } -export const hasError = ( - storeValue: QueryStoreValue, - policy: ErrorPolicy = 'none', -) => storeValue && ( - storeValue.networkError || - (policy === 'none' && isNonEmptyArray(storeValue.graphQLErrors)) -); - let warnedAboutUpdateQuery = false; export class ObservableQuery< @@ -147,8 +137,7 @@ export class ObservableQuery< NetworkStatus.ready; const result: ApolloQueryResult = { - ...(lastError ? null : lastResult), - error: lastError, + ...(lastError ? { error: lastError } : lastResult), loading: isNetworkRequestInFlight(networkStatus), networkStatus, }; @@ -164,44 +153,13 @@ export class ObservableQuery< if (queryStoreValue) { const { networkStatus } = queryStoreValue; - if (hasError(queryStoreValue, this.options.errorPolicy)) { - return Object.assign(result, { - data: void 0, - networkStatus, - error: new ApolloError({ - graphQLErrors: queryStoreValue.graphQLErrors, - networkError: queryStoreValue.networkError, - }), - }); - } - - // Variables might have been added dynamically at query time, when - // using `@client @export(as: "varname")` for example. When this happens, - // the variables have been updated in the query store, but not updated on - // 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.options.variables, - ...(queryStoreValue.variables as TVariables), - }; - } - Object.assign(result, { loading: isNetworkRequestInFlight(networkStatus), networkStatus, }); - - if (queryStoreValue.graphQLErrors && this.options.errorPolicy === 'all') { - result.errors = queryStoreValue.graphQLErrors; - } } - if (partial) { - this.resetLastResults(); - } else { - this.updateLastResult(result); - } + this.updateLastResult(result); return result; } From 2192f897017de2a1e42a99f3e8f6c7400aa6b665 Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Wed, 22 Jul 2020 14:51:54 -0400 Subject: [PATCH 04/16] Replace QueryManager#getQueryStoreValue with more specific accessors. --- src/core/ObservableQuery.ts | 67 ++++++++------------------- src/core/QueryManager.ts | 31 ++++++++----- src/core/__tests__/ObservableQuery.ts | 12 ++--- 3 files changed, 45 insertions(+), 65 deletions(-) diff --git a/src/core/ObservableQuery.ts b/src/core/ObservableQuery.ts index 1326bbc59a8..d065bb6615b 100644 --- a/src/core/ObservableQuery.ts +++ b/src/core/ObservableQuery.ts @@ -120,22 +120,8 @@ export class ObservableQuery< } public getCurrentResult(): ApolloQueryResult { - const { - lastResult, - lastError, - options: { fetchPolicy }, - } = this; - - const isNetworkFetchPolicy = - fetchPolicy === 'network-only' || - fetchPolicy === 'no-cache'; - - const networkStatus = - lastError ? NetworkStatus.error : - lastResult ? lastResult.networkStatus : - isNetworkFetchPolicy ? NetworkStatus.loading : - NetworkStatus.ready; - + const { lastResult, lastError } = this; + const networkStatus = this.queryManager.getNetworkStatus(this.queryId); const result: ApolloQueryResult = { ...(lastError ? { error: lastError } : lastResult), loading: isNetworkRequestInFlight(networkStatus), @@ -149,16 +135,6 @@ export class ObservableQuery< const { data, partial } = this.getCurrentQueryResult(); Object.assign(result, { data, partial }); - const queryStoreValue = this.queryManager.getQueryStoreValue(this.queryId); - if (queryStoreValue) { - const { networkStatus } = queryStoreValue; - - Object.assign(result, { - loading: isNetworkRequestInFlight(networkStatus), - networkStatus, - }); - } - this.updateLastResult(result); return result; @@ -188,11 +164,7 @@ export class ObservableQuery< } public resetQueryStoreErrors() { - const queryStore = this.queryManager.getQueryStoreValue(this.queryId); - if (queryStore) { - queryStore.networkError = undefined; - queryStore.graphQLErrors = []; - } + this.queryManager.resetErrors(this.queryId); } /** @@ -260,21 +232,20 @@ export class ObservableQuery< if (combinedOptions.notifyOnNetworkStatusChange) { const currentResult = this.getCurrentResult(); - const queryInfo = this.queryManager.getQueryStoreValue(this.queryId); - if (queryInfo) { - // If we neglect to update queryInfo.networkStatus here, - // getCurrentResult may return a loading:false result while - // fetchMore is in progress, since getCurrentResult also consults - // queryInfo.networkStatus. Note: setting queryInfo.networkStatus - // to an in-flight status means that QueryInfo#shouldNotify will - // return false while fetchMore is in progress, which is why we - // call this.reobserve() explicitly in the .finally callback after - // fetchMore (below), since the cache write will not automatically - // trigger a notification, even though it does trigger a cache - // broadcast. This is a good thing, because it means we won't see - // intervening query notifications while fetchMore is pending. - queryInfo.networkStatus = NetworkStatus.fetchMore; - } + + // If we neglect to update queryInfo.networkStatus here, + // getCurrentResult may return a loading:false result while + // fetchMore is in progress, since getCurrentResult also consults + // queryInfo.networkStatus. Note: setting queryInfo.networkStatus + // to an in-flight status means that QueryInfo#shouldNotify will + // return false while fetchMore is in progress, which is why we + // call this.reobserve() explicitly in the .finally callback after + // fetchMore (below), since the cache write will not automatically + // trigger a notification, even though it does trigger a cache + // broadcast. This is a good thing, because it means we won't see + // intervening query notifications while fetchMore is pending. + this.queryManager.setNetworkStatus(this.queryId, NetworkStatus.fetchMore); + // Simulate a loading result for the original query with // networkStatus === NetworkStatus.fetchMore. this.observer.next!({ @@ -619,7 +590,9 @@ once, rather than every time you call fetchMore.`); ); }, // Avoid polling during SSR and when the query is already in flight. - !queryManager.ssrMode && (() => !queryManager.checkInFlight(queryId)), + !queryManager.ssrMode && ( + () => !isNetworkRequestInFlight( + queryManager.getNetworkStatus(queryId))), ); } diff --git a/src/core/QueryManager.ts b/src/core/QueryManager.ts index be8522c9ee4..66d5e533932 100644 --- a/src/core/QueryManager.ts +++ b/src/core/QueryManager.ts @@ -357,8 +357,25 @@ export class QueryManager { return store; } - public getQueryStoreValue(queryId: string): QueryStoreValue | undefined { - return queryId ? this.queries.get(queryId) : undefined; + public resetErrors(queryId: string) { + const queryInfo = this.queries.get(queryId); + if (queryInfo) { + queryInfo.networkError = undefined; + queryInfo.graphQLErrors = []; + } + } + + public getNetworkStatus(queryId: string): NetworkStatus { + const queryInfo = this.queries.get(queryId); + const status = queryInfo && queryInfo.networkStatus; + return status || NetworkStatus.ready; + } + + public setNetworkStatus(queryId: string, status: NetworkStatus) { + const queryInfo = this.queries.get(queryId); + if (queryInfo) { + queryInfo.networkStatus = status; + } } private transformCache = new (canUseWeakMap ? WeakMap : Map)< @@ -1066,16 +1083,6 @@ export class QueryManager { clientAwareness: this.clientAwareness, }; } - - public checkInFlight(queryId: string): boolean { - const query = this.getQueryStoreValue(queryId); - return ( - !!query && - !!query.networkStatus && - query.networkStatus !== NetworkStatus.ready && - query.networkStatus !== NetworkStatus.error - ); - } } function markMutationResult( diff --git a/src/core/__tests__/ObservableQuery.ts b/src/core/__tests__/ObservableQuery.ts index 4382969608c..9c51f4de95f 100644 --- a/src/core/__tests__/ObservableQuery.ts +++ b/src/core/__tests__/ObservableQuery.ts @@ -1887,11 +1887,11 @@ describe('ObservableQuery', () => { observable.subscribe({ error() { const { queryManager } = (observable as any); - const queryStore = queryManager.getQueryStoreValue(observable.queryId); - expect(queryStore.graphQLErrors).toEqual([graphQLError]); + const queryInfo = queryManager["queries"].get(observable.queryId); + expect(queryInfo.graphQLErrors).toEqual([graphQLError]); observable.resetQueryStoreErrors(); - expect(queryStore.graphQLErrors).toEqual([]); + expect(queryInfo.graphQLErrors).toEqual([]); resolve(); } @@ -1909,10 +1909,10 @@ describe('ObservableQuery', () => { observable.subscribe({ next() { const { queryManager } = (observable as any); - const queryStore = queryManager.getQueryStoreValue(observable.queryId); - queryStore.networkError = networkError; + const queryInfo = queryManager["queries"].get(observable.queryId); + queryInfo.networkError = networkError; observable.resetQueryStoreErrors(); - expect(queryStore.networkError).toBeUndefined(); + expect(queryInfo.networkError).toBeUndefined(); resolve(); } }); From 383405efd5d71062eafe3ab993d2f049600b31b4 Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Wed, 22 Jul 2020 18:49:54 -0400 Subject: [PATCH 05/16] Bring back QueryInfo#getDiff, and privatize updateWatch. Managing cache watching is now the exclusive responsibility of the QueryInfo class. --- src/core/QueryInfo.ts | 43 +++++++++++++++++++++++++++------------- src/core/QueryManager.ts | 9 ++------- 2 files changed, 31 insertions(+), 21 deletions(-) diff --git a/src/core/QueryInfo.ts b/src/core/QueryInfo.ts index 0fc41862ebc..a9e38ec454a 100644 --- a/src/core/QueryInfo.ts +++ b/src/core/QueryInfo.ts @@ -29,7 +29,7 @@ export type QueryStoreValue = Pick this.notify(), 0); - } - } - return this; - } - private notifyTimeout?: ReturnType; private diff: Cache.DiffResult | null = null; + getDiff(variables = this.variables): Cache.DiffResult { + if (this.diff && equal(variables, this.variables)) { + return this.diff; + } + + this.updateWatch(this.variables = variables); + + return this.diff = this.cache.diff({ + query: this.document!, + variables, + returnPartialData: true, + optimistic: true, + }); + } + setDiff(diff: Cache.DiffResult | null) { const oldDiff = this.diff; this.diff = diff; if (!this.dirty && diff?.result !== oldDiff?.result) { - this.setDirty(); + this.dirty = true; + if (!this.notifyTimeout) { + this.notifyTimeout = setTimeout(() => this.notify(), 0); + } } } @@ -181,7 +193,7 @@ export class QueryInfo { private lastWatch?: Cache.WatchOptions; - public updateWatch>(variables: TVars): this { + private updateWatch(variables = this.variables) { if (!this.lastWatch || this.lastWatch.query !== this.document || !equal(variables, this.lastWatch.variables)) { @@ -193,7 +205,6 @@ export class QueryInfo { callback: diff => this.setDiff(diff), }); } - return this; } private lastWrittenResult?: FetchResult; @@ -286,6 +297,10 @@ export class QueryInfo { optimistic: true, }); + // Any time we're about to update this.diff, we need to make + // sure we've started watching the cache. + this.updateWatch(options.variables); + // If we're allowed to write to the cache, and we can read a // complete result from the cache, update result.data to be the // result from the cache, rather than the raw network result. diff --git a/src/core/QueryManager.ts b/src/core/QueryManager.ts index 66d5e533932..ec134bba406 100644 --- a/src/core/QueryManager.ts +++ b/src/core/QueryManager.ts @@ -965,15 +965,10 @@ export class QueryManager { variables, lastRequestId: this.generateRequestId(), networkStatus, - }).updateWatch(variables); - - const readCache = () => this.cache.diff({ - query, - variables, - returnPartialData: true, - optimistic: true, }); + const readCache = () => queryInfo.getDiff(variables); + const resultsFromCache = ( diff: Cache.DiffResult, networkStatus = queryInfo.networkStatus || NetworkStatus.loading, From 022c797f4cca0d1a211191698d0b7dfdbeb4e992 Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Fri, 24 Jul 2020 10:53:34 -0400 Subject: [PATCH 06/16] Avoid watching cache for no-cache queries. Since no-cache queries never read from the cache, there's no point in making them watch the cache for changes, because the cache doesn't know anything about which fields were used by the no-cache query. --- src/core/QueryInfo.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/core/QueryInfo.ts b/src/core/QueryInfo.ts index a9e38ec454a..7012b2634d4 100644 --- a/src/core/QueryInfo.ts +++ b/src/core/QueryInfo.ts @@ -194,6 +194,10 @@ export class QueryInfo { private lastWatch?: Cache.WatchOptions; private updateWatch(variables = this.variables) { + const oq = this.observableQuery; + if (oq && oq.options.fetchPolicy === "no-cache") { + return; + } if (!this.lastWatch || this.lastWatch.query !== this.document || !equal(variables, this.lastWatch.variables)) { From af2d1e3b77d7309167550c63bec4f752ccd51ad9 Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Wed, 22 Jul 2020 20:44:29 -0400 Subject: [PATCH 07/16] Remove unused QueryManager#addQueryListener method. --- src/core/QueryManager.ts | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/core/QueryManager.ts b/src/core/QueryManager.ts index ec134bba406..43dae7c726b 100644 --- a/src/core/QueryManager.ts +++ b/src/core/QueryManager.ts @@ -33,7 +33,6 @@ import { import { ObservableQuery } from './ObservableQuery'; import { NetworkStatus, isNetworkRequestInFlight } from './networkStatus'; import { - QueryListener, ApolloQueryResult, OperationVariables, MutationQueryReducer, @@ -527,10 +526,6 @@ export class QueryManager { if (queryInfo) queryInfo.stop(); } - public addQueryListener(queryId: string, listener: QueryListener) { - this.getQuery(queryId).listeners.add(listener); - } - public clearStore(): Promise { // Before we have sent the reset action to the store, we can no longer // rely on the results returned by in-flight requests since these may From a1cd4f49c718c718cc4e44a75fd1ea131cdd61e0 Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Wed, 22 Jul 2020 19:57:26 -0400 Subject: [PATCH 08/16] Pass QueryInfo into ObservableQuery constructor. This gives the ObservableQuery direct access to its corresponding QueryInfo object, so the ObservableQuery doesn't have to keep asking the QueryManager for the QueryInfo corresponding to this.queryId. Important invariants: given ObservableQuery oq and QueryManager qm, it should always be the case that qm.queries.get(oq.queryId) === oq.queryInfo, and oq.queryInfo.observableQuery === oq. --- src/core/ObservableQuery.ts | 6 ++++++ src/core/QueryInfo.ts | 1 + src/core/QueryManager.ts | 6 +++++- 3 files changed, 12 insertions(+), 1 deletion(-) diff --git a/src/core/ObservableQuery.ts b/src/core/ObservableQuery.ts index d065bb6615b..47b75f1b46e 100644 --- a/src/core/ObservableQuery.ts +++ b/src/core/ObservableQuery.ts @@ -20,6 +20,7 @@ import { SubscribeToMoreOptions, } from './watchQueryOptions'; import { Reobserver } from './Reobserver'; +import { QueryInfo } from './QueryInfo'; export interface FetchMoreOptions< TData = any, @@ -62,12 +63,15 @@ export class ObservableQuery< private lastResult: ApolloQueryResult; private lastResultSnapshot: ApolloQueryResult; private lastError: ApolloError; + private queryInfo: QueryInfo; constructor({ queryManager, + queryInfo, options, }: { queryManager: QueryManager; + queryInfo: QueryInfo; options: WatchQueryOptions; }) { super((observer: Observer>) => @@ -86,6 +90,8 @@ export class ObservableQuery< // related classes this.queryManager = queryManager; + + this.queryInfo = queryInfo; } public result(): Promise> { diff --git a/src/core/QueryInfo.ts b/src/core/QueryInfo.ts index 7012b2634d4..2a3a3f882e9 100644 --- a/src/core/QueryInfo.ts +++ b/src/core/QueryInfo.ts @@ -133,6 +133,7 @@ export class QueryInfo { (this as any).observableQuery = oq; if (oq) { + oq["queryInfo"] = this; this.listeners.add(this.oqListener = () => oq.reobserve()); } else { delete this.oqListener; diff --git a/src/core/QueryManager.ts b/src/core/QueryManager.ts index 43dae7c726b..a6d591c53d0 100644 --- a/src/core/QueryManager.ts +++ b/src/core/QueryManager.ts @@ -456,12 +456,16 @@ export class QueryManager { options.notifyOnNetworkStatusChange = false; } + const queryInfo = new QueryInfo(this.cache); const observable = new ObservableQuery({ queryManager: this, + queryInfo, options, }); - this.getQuery(observable.queryId).init({ + this.queries.set(observable.queryId, queryInfo); + + queryInfo.init({ document: options.query, observableQuery: observable, variables: options.variables, From 06a1dc2e03d805e084e52e5e8be51d667038eeed Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Wed, 22 Jul 2020 21:02:57 -0400 Subject: [PATCH 09/16] Avoid unnecessarily discarding QueryInfo fields on stop. Since ObservableQuery objects have a direct reference to their QueryInfo objects, it's possible for an ObservableQuery to find use for these fields even after the QueryManager has stopped the query and removed the QueryInfo object from queryManager.queries. --- src/core/QueryInfo.ts | 8 -------- 1 file changed, 8 deletions(-) diff --git a/src/core/QueryInfo.ts b/src/core/QueryInfo.ts index 2a3a3f882e9..8f42d1d28b7 100644 --- a/src/core/QueryInfo.ts +++ b/src/core/QueryInfo.ts @@ -176,14 +176,6 @@ export class QueryInfo { // QueryInfo.prototype. delete this.cancel; - this.variables = - this.networkStatus = - this.networkError = - this.graphQLErrors = - this.lastWatch = - this.lastWrittenResult = - this.lastWrittenVars = void 0; - const oq = this.observableQuery; if (oq) oq.stopPolling(); } From 0cf0cbdb57a3269d8d14f0ee7d9301e84f129eee Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Wed, 22 Jul 2020 20:03:43 -0400 Subject: [PATCH 10/16] Use this.queryInfo directly, obviating new methods. --- src/core/ObservableQuery.ts | 7 +++---- src/core/QueryManager.ts | 13 ------------- 2 files changed, 3 insertions(+), 17 deletions(-) diff --git a/src/core/ObservableQuery.ts b/src/core/ObservableQuery.ts index 47b75f1b46e..931888a94b0 100644 --- a/src/core/ObservableQuery.ts +++ b/src/core/ObservableQuery.ts @@ -127,7 +127,7 @@ export class ObservableQuery< public getCurrentResult(): ApolloQueryResult { const { lastResult, lastError } = this; - const networkStatus = this.queryManager.getNetworkStatus(this.queryId); + const networkStatus = this.queryInfo.networkStatus || NetworkStatus.ready; const result: ApolloQueryResult = { ...(lastError ? { error: lastError } : lastResult), loading: isNetworkRequestInFlight(networkStatus), @@ -250,7 +250,7 @@ export class ObservableQuery< // trigger a notification, even though it does trigger a cache // broadcast. This is a good thing, because it means we won't see // intervening query notifications while fetchMore is pending. - this.queryManager.setNetworkStatus(this.queryId, NetworkStatus.fetchMore); + this.queryInfo.networkStatus = NetworkStatus.fetchMore; // Simulate a loading result for the original query with // networkStatus === NetworkStatus.fetchMore. @@ -597,8 +597,7 @@ once, rather than every time you call fetchMore.`); }, // Avoid polling during SSR and when the query is already in flight. !queryManager.ssrMode && ( - () => !isNetworkRequestInFlight( - queryManager.getNetworkStatus(queryId))), + () => !isNetworkRequestInFlight(this.queryInfo.networkStatus)) ); } diff --git a/src/core/QueryManager.ts b/src/core/QueryManager.ts index a6d591c53d0..a0ac0444786 100644 --- a/src/core/QueryManager.ts +++ b/src/core/QueryManager.ts @@ -364,19 +364,6 @@ export class QueryManager { } } - public getNetworkStatus(queryId: string): NetworkStatus { - const queryInfo = this.queries.get(queryId); - const status = queryInfo && queryInfo.networkStatus; - return status || NetworkStatus.ready; - } - - public setNetworkStatus(queryId: string, status: NetworkStatus) { - const queryInfo = this.queries.get(queryId); - if (queryInfo) { - queryInfo.networkStatus = status; - } - } - private transformCache = new (canUseWeakMap ? WeakMap : Map)< DocumentNode, Readonly<{ From 61e0e62ca5547b6f48926b67d7bcb057e02d82b1 Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Wed, 22 Jul 2020 19:58:08 -0400 Subject: [PATCH 11/16] Use QueryInfo#getDiff in getCurrentQueryResult. --- src/core/ObservableQuery.ts | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/src/core/ObservableQuery.ts b/src/core/ObservableQuery.ts index 931888a94b0..60f977c1a43 100644 --- a/src/core/ObservableQuery.ts +++ b/src/core/ObservableQuery.ts @@ -477,13 +477,7 @@ once, rather than every time you call fetchMore.`); }; } - let { result, complete } = this.queryManager.cache.diff({ - query: this.options.query, - variables: this.variables, - previousResult: this.lastResult?.data, - returnPartialData: true, - optimistic, - }); + let { result, complete } = this.queryInfo.getDiff(); if (lastData && !this.lastError && From 7df6ec04c6257a8278fd02e89e80f1b0435073a3 Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Wed, 22 Jul 2020 21:24:59 -0400 Subject: [PATCH 12/16] Inline getCurrentQueryResult and simplify its logic. --- src/core/ObservableQuery.ts | 61 +++++++++--------------- src/core/__tests__/QueryManager/index.ts | 11 ++++- 2 files changed, 31 insertions(+), 41 deletions(-) diff --git a/src/core/ObservableQuery.ts b/src/core/ObservableQuery.ts index 60f977c1a43..c645d195dd8 100644 --- a/src/core/ObservableQuery.ts +++ b/src/core/ObservableQuery.ts @@ -138,8 +138,28 @@ export class ObservableQuery< return result; } - const { data, partial } = this.getCurrentQueryResult(); - Object.assign(result, { data, partial }); + const { fetchPolicy } = this.options; + if (fetchPolicy === 'no-cache' || + fetchPolicy === 'network-only') { + result.partial = false; + } else if ( + !result.data || + // If this.options.query has @client(always: true) fields, we cannot + // trust result.data, since it was read from the cache without + // running local resolvers (and it's too late to run resolvers now, + // since we must return a result synchronously). TODO In the future + // (after Apollo Client 3.0), we should find a way to trust + // this.lastResult in more cases, and read from the cache only in + // cases when no result has been received yet. + !this.queryManager.transform(this.options.query).hasForcedResolvers + ) { + const diff = this.queryInfo.getDiff(); + result.partial = !diff.complete; + result.data = ( + diff.complete || + this.options.returnPartialData + ) ? diff.result : void 0; + } this.updateLastResult(result); @@ -461,43 +481,6 @@ once, rather than every time you call fetchMore.`); } } - private getCurrentQueryResult( - optimistic: boolean = true, - ): { - data?: TData; - partial: boolean; - } { - const { fetchPolicy } = this.options; - const lastData = this.lastResult?.data; - if (fetchPolicy === 'no-cache' || - fetchPolicy === 'network-only') { - return { - data: lastData, - partial: false, - }; - } - - let { result, complete } = this.queryInfo.getDiff(); - - if (lastData && - !this.lastError && - // If this.options.query has @client(always: true) fields, we - // cannot trust result, since it was read from the cache without - // running local resolvers (and it's too late to run resolvers - // now, since we must return a result synchronously). TODO In the - // future (after Apollo Client 3.0), we should find a way to trust - // this.lastResult in more cases, and read from the cache only in - // cases when no result has been received yet. - this.queryManager.transform(this.options.query).hasForcedResolvers) { - result = lastData; - } - - return { - data: (complete || this.options.returnPartialData) ? result : void 0, - partial: !complete, - }; - } - public startPolling(pollInterval: number) { this.getReobserver().updateOptions({ pollInterval }); } diff --git a/src/core/__tests__/QueryManager/index.ts b/src/core/__tests__/QueryManager/index.ts index c80caf9ec75..6f927080509 100644 --- a/src/core/__tests__/QueryManager/index.ts +++ b/src/core/__tests__/QueryManager/index.ts @@ -191,8 +191,15 @@ describe('QueryManager', () => { function getCurrentQueryResult( observableQuery: ObservableQuery, - ): ReturnType["getCurrentQueryResult"]> { - return (observableQuery as any).getCurrentQueryResult(); + ): { + data?: TData; + partial: boolean; + } { + const result = observableQuery.getCurrentResult(); + return { + data: result.data, + partial: !!result.partial, + }; } itAsync('handles GraphQL errors', (resolve, reject) => { From 8be38008c335ec48c0be57c8c16728229588c1cc Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Mon, 27 Jul 2020 12:13:35 -0400 Subject: [PATCH 13/16] Move cache-first fallback logic after first network request. PR #6353 explains the rationale for switching to a cache-first FetchPolicy after an initial cache-and-network or network-only policy. When #6365 was implemented, options.fetchPolicy was examined only once, at the beginning of fetchQueryObservable, so the timing of changing options.fetchPolicy did not matter as much. However, fixing #6659 involves checking the current options.fetchPolicy whenever the QueryData class calls this.currentObservable.getCurrentResult(), so it's now more important to delay changing options.fetchPolicy until after the first network request has completed. --- src/core/QueryManager.ts | 40 ++++++++++--------- src/react/hoc/__tests__/queries/skip.test.tsx | 25 +++++++----- 2 files changed, 35 insertions(+), 30 deletions(-) diff --git a/src/core/QueryManager.ts b/src/core/QueryManager.ts index a0ac0444786..da6fc4b80a6 100644 --- a/src/core/QueryManager.ts +++ b/src/core/QueryManager.ts @@ -836,24 +836,6 @@ export class QueryManager { context = {}, } = options; - if (fetchPolicy === "cache-and-network" || - fetchPolicy === "network-only") { - // When someone chooses cache-and-network or network-only as their - // initial FetchPolicy, they almost certainly do not want future cache - // updates to trigger unconditional network requests, which is what - // repeatedly applying the cache-and-network or network-only policies - // would seem to require. Instead, when the cache reports an update - // after the initial network request, subsequent network requests should - // be triggered only if the cache result is incomplete. This behavior - // corresponds exactly to switching to a cache-first FetchPolicy, so we - // modify options.fetchPolicy here for the next fetchQueryObservable - // call, using the same options object that the Reobserver always passes - // to fetchQueryObservable. Note: if these FetchPolicy transitions get - // much more complicated, we might consider using some sort of state - // machine to capture the transition rules. - options.fetchPolicy = "cache-first"; - } - const mightUseNetwork = fetchPolicy === "cache-first" || fetchPolicy === "cache-and-network" || @@ -924,7 +906,27 @@ export class QueryManager { : fromVariables(normalized.variables!) ); - concast.cleanup(() => this.fetchCancelFns.delete(queryId)); + concast.cleanup(() => { + this.fetchCancelFns.delete(queryId); + + if (fetchPolicy === "cache-and-network" || + fetchPolicy === "network-only") { + // When someone chooses cache-and-network or network-only as their + // initial FetchPolicy, they almost certainly do not want future cache + // updates to trigger unconditional network requests, which is what + // repeatedly applying the cache-and-network or network-only policies + // would seem to require. Instead, when the cache reports an update + // after the initial network request, subsequent network requests should + // be triggered only if the cache result is incomplete. This behavior + // corresponds exactly to switching to a cache-first FetchPolicy, so we + // modify options.fetchPolicy here for the next fetchQueryObservable + // call, using the same options object that the Reobserver always passes + // to fetchQueryObservable. Note: if these FetchPolicy transitions get + // much more complicated, we might consider using some sort of state + // machine to capture the transition rules. + options.fetchPolicy = "cache-first"; + } + }); return concast; } diff --git a/src/react/hoc/__tests__/queries/skip.test.tsx b/src/react/hoc/__tests__/queries/skip.test.tsx index 19d43677fe2..e80c842c046 100644 --- a/src/react/hoc/__tests__/queries/skip.test.tsx +++ b/src/react/hoc/__tests__/queries/skip.test.tsx @@ -619,32 +619,35 @@ describe('[queries] skip', () => { })( class extends React.Component { render() { - switch (count) { - case 0: - expect(this.props.data.loading).toBeTruthy(); + switch (++count) { + case 1: + expect(this.props.data.loading).toBe(true); expect(ranQuery).toBe(1); break; - case 1: - expect(this.props.data.loading).toBeFalsy(); + case 2: + expect(this.props.data.loading).toBe(false); expect(ranQuery).toBe(1); setTimeout(() => { this.props.setSkip(true); }); break; - case 2: + case 3: expect(this.props.data).toBeUndefined(); expect(ranQuery).toBe(1); setTimeout(() => { this.props.setSkip(false); }); break; - case 3: - expect(this.props.data!.loading).toBeFalsy(); - expect(ranQuery).toBe(2); + case 4: + expect(this.props.data!.loading).toBe(true); + expect(ranQuery).toBe(3); + break; + case 5: + expect(this.props.data!.loading).toBe(false); + expect(ranQuery).toBe(3); break; default: } - count += 1; return null; } } @@ -669,7 +672,7 @@ describe('[queries] skip', () => { ); await wait(() => { - expect(count).toEqual(4); + expect(count).toEqual(5); }); }); From 5fe8feed50033eadbdd67241f791a61a7f57217f Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Mon, 27 Jul 2020 12:23:26 -0400 Subject: [PATCH 14/16] Avoid loading:true results for warm cache reads when possible. Fixes #6659 and #6686. --- src/core/ObservableQuery.ts | 12 +++++++++++- src/core/__tests__/ObservableQuery.ts | 4 ++-- .../components/__tests__/client/Query.test.tsx | 5 +---- .../__tests__/mutations/recycled-queries.test.tsx | 14 ++------------ .../hoc/__tests__/queries/observableQuery.test.tsx | 3 --- 5 files changed, 16 insertions(+), 22 deletions(-) diff --git a/src/core/ObservableQuery.ts b/src/core/ObservableQuery.ts index c645d195dd8..7bff619a1ae 100644 --- a/src/core/ObservableQuery.ts +++ b/src/core/ObservableQuery.ts @@ -138,7 +138,7 @@ export class ObservableQuery< return result; } - const { fetchPolicy } = this.options; + const { fetchPolicy = 'cache-first' } = this.options; if (fetchPolicy === 'no-cache' || fetchPolicy === 'network-only') { result.partial = false; @@ -159,6 +159,16 @@ export class ObservableQuery< diff.complete || this.options.returnPartialData ) ? diff.result : void 0; + // If the cache diff is complete, and we're using a FetchPolicy that + // 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')) { + result.networkStatus = NetworkStatus.ready; + result.loading = false; + } } this.updateLastResult(result); diff --git a/src/core/__tests__/ObservableQuery.ts b/src/core/__tests__/ObservableQuery.ts index 9c51f4de95f..4ecca7bf4da 100644 --- a/src/core/__tests__/ObservableQuery.ts +++ b/src/core/__tests__/ObservableQuery.ts @@ -1396,8 +1396,8 @@ describe('ObservableQuery', () => { }); expect(stripSymbols(observable.getCurrentResult())).toEqual({ data: dataOne, - loading: true, - networkStatus: NetworkStatus.loading, + loading: false, + networkStatus: NetworkStatus.ready, partial: false, }); }).then(resolve, reject); diff --git a/src/react/components/__tests__/client/Query.test.tsx b/src/react/components/__tests__/client/Query.test.tsx index 5d2814476aa..a95d18e5ad9 100644 --- a/src/react/components/__tests__/client/Query.test.tsx +++ b/src/react/components/__tests__/client/Query.test.tsx @@ -1674,9 +1674,6 @@ describe('Query component', () => { }); break; case 4: - expect(props.loading).toBeTruthy(); - break; - case 5: // Good result should be received without any errors. expect(props.error).toBeFalsy(); expect(props.data.allPeople).toBeTruthy(); @@ -1698,7 +1695,7 @@ describe('Query component', () => { ); - return wait(() => expect(count).toBe(6)).then(resolve, reject); + return wait(() => expect(count).toBe(5)).then(resolve, reject); } ); diff --git a/src/react/hoc/__tests__/mutations/recycled-queries.test.tsx b/src/react/hoc/__tests__/mutations/recycled-queries.test.tsx index 40bcbb731d9..53147256128 100644 --- a/src/react/hoc/__tests__/mutations/recycled-queries.test.tsx +++ b/src/react/hoc/__tests__/mutations/recycled-queries.test.tsx @@ -162,9 +162,6 @@ describe('graphql(mutation) update queries', () => { }); break; case 3: - expect(this.props.data!.loading).toBeTruthy(); - break; - case 4: expect(stripSymbols(this.props.data!.todo_list)).toEqual({ id: '123', title: 'how to apollo', @@ -228,7 +225,6 @@ describe('graphql(mutation) update queries', () => { expect(todoUpdateQueryCount).toBe(2); expect(queryMountCount).toBe(2); expect(queryUnmountCount).toBe(2); - expect(queryRenderCount).toBe(5); }, 5); }, 5); }, 5); @@ -236,7 +232,7 @@ describe('graphql(mutation) update queries', () => { }, 5); return wait(() => { - expect(queryRenderCount).toBe(5); + expect(queryRenderCount).toBe(4); }); }); @@ -359,12 +355,6 @@ describe('graphql(mutation) update queries', () => { ); break; case 3: - expect(this.props.data!.loading).toBeTruthy(); - expect(stripSymbols(this.props.data!.todo_list)).toEqual( - updatedData.todo_list - ); - break; - case 4: expect(this.props.data!.loading).toBeFalsy(); expect(stripSymbols(this.props.data!.todo_list)).toEqual( updatedData.todo_list @@ -405,7 +395,7 @@ describe('graphql(mutation) update queries', () => { }); return wait(() => { - expect(queryRenderCount).toBe(5); + expect(queryRenderCount).toBe(4); }); }); }); diff --git a/src/react/hoc/__tests__/queries/observableQuery.test.tsx b/src/react/hoc/__tests__/queries/observableQuery.test.tsx index 7f856749ac8..940e74fe824 100644 --- a/src/react/hoc/__tests__/queries/observableQuery.test.tsx +++ b/src/react/hoc/__tests__/queries/observableQuery.test.tsx @@ -321,9 +321,6 @@ describe('[queries] observableQuery', () => { expect(stripSymbols(allPeople)).toEqual(data.allPeople); break; case 3: - expect(loading).toBe(true); - break; - case 4: expect(loading).toBe(false); expect(stripSymbols(allPeople)).toEqual(data.allPeople); break; From fb3c913a6e30ab0cf05d081c39e4c163fcc2ae39 Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Mon, 27 Jul 2020 14:37:10 -0400 Subject: [PATCH 15/16] Mention PR #6710 in CHANGELOG.md. --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4bd9308c691..c84dee83f65 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,9 @@ [@igaloly](https://github.com/igaloly) in [#6261](https://github.com/apollographql/apollo-client/pull/6261) and [@Kujawadl](https://github.com/Kujawadl) in [#6526](https://github.com/apollographql/apollo-client/pull/6526) +- Refactor `ObservableQuery#getCurrentResult` to reenable immediate delivery of warm cache results.
+ [@benjamn](https://github.com/benjamn) in [#6710](https://github.com/apollographql/apollo-client/pull/6710) + ## Improvements - Errors of the form `Invariant Violation: 42` thrown in production can now be looked up much more easily, by consulting the auto-generated `@apollo/client/invariantErrorCodes.js` file specific to your `@apollo/client` version.
From fd94a3d5f0862681de9fca5d2527d838fec7fe40 Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Mon, 27 Jul 2020 14:39:14 -0400 Subject: [PATCH 16/16] Mention removal of ApolloCurrentQueryResult type in CHANGELOG.md. --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c84dee83f65..f83e9f55977 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,7 +13,7 @@ [@igaloly](https://github.com/igaloly) in [#6261](https://github.com/apollographql/apollo-client/pull/6261) and [@Kujawadl](https://github.com/Kujawadl) in [#6526](https://github.com/apollographql/apollo-client/pull/6526) -- Refactor `ObservableQuery#getCurrentResult` to reenable immediate delivery of warm cache results.
+- Refactor `ObservableQuery#getCurrentResult` to reenable immediate delivery of warm cache results. As part of this refactoring, the `ApolloCurrentQueryResult` type was eliminated in favor of `ApolloQueryResult`.
[@benjamn](https://github.com/benjamn) in [#6710](https://github.com/apollographql/apollo-client/pull/6710) ## Improvements