diff --git a/src/core/QueryInfo.ts b/src/core/QueryInfo.ts index 3e63e6003ed..740a439d868 100644 --- a/src/core/QueryInfo.ts +++ b/src/core/QueryInfo.ts @@ -165,7 +165,10 @@ export class QueryInfo { this.variables = this.networkStatus = this.networkError = - this.graphQLErrors = void 0; + this.graphQLErrors = + this.lastWatch = + this.lastWrittenResult = + this.lastWrittenVars = void 0; const oq = this.observableQuery; if (oq) oq.stopPolling(); @@ -192,6 +195,9 @@ export class QueryInfo { return this; } + private lastWrittenResult?: FetchResult; + private lastWrittenVars?: WatchQueryOptions["variables"]; + public markResult( result: FetchResult, options: Pick, allowCacheWrite: boolean, ) { + this.graphQLErrors = isNonEmptyArray(result.errors) ? result.errors : []; + if (options.fetchPolicy === 'no-cache') { this.diff = { result: result.data, complete: true }; @@ -218,11 +226,57 @@ export class QueryInfo { // of writeQuery, so we can store the new diff quietly and ignore // it when we receive it redundantly from the watch callback. this.cache.performTransaction(cache => { - cache.writeQuery({ - query: this.document!, - data: result.data as T, - variables: options.variables, - }); + if (equal(result, this.lastWrittenResult) && + equal(options.variables, this.lastWrittenVars)) { + // If result is the same as the last result we received from + // the network (and the variables match too), avoid writing + // result into the cache again. The wisdom of skipping this + // cache write is far from obvious, since any cache write + // could be the one that puts the cache back into a desired + // state, fixing corruption or missing data. However, if we + // always write every network result into the cache, we enable + // feuds between queries competing to update the same data in + // incompatible ways, which can lead to an endless cycle of + // cache broadcasts and useless network requests. As with any + // feud, eventually one side must step back from the brink, + // letting the other side(s) have the last word(s). There may + // be other points where we could break this cycle, such as + // silencing the broadcast for cache.writeQuery (not a good + // idea, since it just delays the feud a bit) or somehow + // avoiding the network request that just happened (also bad, + // because the server could return useful new data). All + // options considered, skipping this cache write seems to be + // the least damaging place to break the cycle, because it + // reflects the intuition that we recently wrote this exact + // result into the cache, so the cache *should* already/still + // contain this data. If some other query has clobbered that + // data in the meantime, that's too bad, but there will be no + // winners if every query blindly reverts to its own version + // of the data. This approach also gives the network a chance + // to return new data, which will be written into the cache as + // usual, notifying only those queries that are directly + // affected by the cache updates, as usual. In the future, an + // even more sophisticated cache could perhaps prevent or + // mitigate the clobbering somehow, but that would make this + // particular cache write even less important, and thus + // skipping it would be even safer than it is today. + if (this.diff && this.diff.complete) { + // Reuse data from the last good (complete) diff that we + // received, when possible. + result.data = this.diff.result; + return; + } + // If the previous this.diff was incomplete, fall through to + // re-reading the latest data with cache.diff, below. + } else { + cache.writeQuery({ + query: this.document!, + data: result.data as T, + variables: options.variables, + }); + this.lastWrittenResult = result; + this.lastWrittenVars = options.variables; + } const diff = cache.diff({ query: this.document!, @@ -241,10 +295,11 @@ export class QueryInfo { result.data = diff.result; } }); + + } else { + this.lastWrittenResult = this.lastWrittenVars = void 0; } } - - this.graphQLErrors = isNonEmptyArray(result.errors) ? result.errors : []; } public markReady() { @@ -254,6 +309,7 @@ export class QueryInfo { public markError(error: ApolloError) { this.networkStatus = NetworkStatus.error; + this.lastWrittenResult = this.lastWrittenVars = void 0; if (error.graphQLErrors) { this.graphQLErrors = error.graphQLErrors; diff --git a/src/core/__tests__/QueryManager/index.ts b/src/core/__tests__/QueryManager/index.ts index e407fd6d700..328a311f0f0 100644 --- a/src/core/__tests__/QueryManager/index.ts +++ b/src/core/__tests__/QueryManager/index.ts @@ -36,6 +36,7 @@ import observableToPromise, { import subscribeAndCount from '../../../utilities/testing/subscribeAndCount'; import { stripSymbols } from '../../../utilities/testing/stripSymbols'; import { itAsync } from '../../../utilities/testing/itAsync'; +import { ApolloClient } from '../../../ApolloClient'; interface MockedMutation { reject: (reason: any) => any; @@ -2049,19 +2050,6 @@ describe('QueryManager', () => { networkStatus: NetworkStatus.ready, }); }, - result => { - expect(stripSymbols(result)).toEqual({ - data: { - ...data2, - author: { - ...data2.author, - id: data1.author.id, - }, - }, - loading: false, - networkStatus: NetworkStatus.ready, - }); - }, ), ]).then(resolve, reject); }); @@ -2163,6 +2151,132 @@ describe('QueryManager', () => { ]).then(resolve, reject); }); + itAsync("should not write unchanged network results to cache", (resolve, reject) => { + const cache = new InMemoryCache({ + typePolicies: { + Query: { + fields: { + info: { + merge(_, incoming) { + return incoming; + }, + }, + }, + }, + }, + }); + + const client = new ApolloClient({ + cache, + link: new ApolloLink(operation => new Observable((observer: Observer) => { + switch (operation.operationName) { + case "A": + observer.next!({ data: { info: { a: "ay" }}}); + break; + case "B": + observer.next!({ data: { info: { b: "bee" }}}); + break; + } + observer.complete!(); + })), + }); + + const queryA = gql`query A { info { a } }`; + const queryB = gql`query B { info { b } }`; + + const obsA = client.watchQuery({ + query: queryA, + returnPartialData: true, + }); + + const obsB = client.watchQuery({ + query: queryB, + returnPartialData: true, + }); + + subscribeAndCount(reject, obsA, (count, result) => { + if (count === 1) { + expect(result).toEqual({ + loading: true, + networkStatus: NetworkStatus.loading, + data: {}, + }); + } else if (count === 2) { + expect(result).toEqual({ + loading: false, + networkStatus: NetworkStatus.ready, + data: { + info: { + a: "ay", + }, + }, + }); + } else if (count === 3) { + expect(result).toEqual({ + loading: true, + networkStatus: NetworkStatus.loading, + data: { + info: {}, + }, + }); + } else if (count === 4) { + expect(result).toEqual({ + loading: false, + networkStatus: NetworkStatus.ready, + data: { + info: { + a: "ay", + }, + }, + }); + setTimeout(resolve, 100); + } else { + reject(new Error(`Unexpected ${JSON.stringify({count,result})}`)); + } + }); + + subscribeAndCount(reject, obsB, (count, result) => { + if (count === 1) { + expect(result).toEqual({ + loading: true, + networkStatus: NetworkStatus.loading, + data: {}, + }); + } else if (count === 2) { + expect(result).toEqual({ + loading: false, + networkStatus: NetworkStatus.ready, + data: { + info: { + b: "bee", + }, + }, + }); + } else if (count === 3) { + expect(result).toEqual({ + loading: true, + networkStatus: NetworkStatus.loading, + data: { + info: {}, + }, + }); + } else if (count === 4) { + expect(result).toEqual({ + loading: false, + networkStatus: NetworkStatus.ready, + data: { + info: { + b: "bee", + }, + }, + }); + setTimeout(resolve, 100); + } else { + reject(new Error(`Unexpected ${JSON.stringify({count,result})}`)); + } + }); + }); + itAsync('should not error when replacing unidentified data with a normalized ID', (resolve, reject) => { const queryWithoutId = gql` query {