From 46dd60835e8fbb1c7e437e2f9b5deccb08cd992d Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Mon, 10 Aug 2020 14:51:08 -0400 Subject: [PATCH] Disable feud-stopping logic after any cache eviction. (#6817) When application code evicts an object or its fields from the cache, and those evictions trigger network requests, the network data is often vital for repairing the damage done by the eviction, even if the network data look exactly the same as the previously-written data. In other words, after any cache eviction, we should disable the logic introduced in PR #6448 to stop query feuds. This exception is reasonable because feuding queries only write additional data into the cache, rather than evicting anything, so we can safely assume evictions do not contribute to a cycle of competing cache updates. I freely admit the method of counting evictions that I've implemented here is a bit of a hack, but it works for any ApolloCache implementation, without requiring the public ApolloCache API to support any new functionality. If we identify any other potential consumers of this information, we might consider a more official API. --- CHANGELOG.md | 3 ++ src/core/QueryInfo.ts | 79 ++++++++++++++++++++++++++++++++++--------- 2 files changed, 66 insertions(+), 16 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 59b0701f73e..71b64989978 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,9 @@ - Substantially improve type inference for data and variables types using `TypedDocumentNode` type instead of `DocumentNode`.
[@dotansimha](https://github.com/dotansimha) in [#6720](https://github.com/apollographql/apollo-client/pull/6720) +- Disable feud-stopping logic after any cache eviction.
+ [@benjamn](https://github.com/benjamn) in [#6817](https://github.com/apollographql/apollo-client/pull/6817) + ## Apollo Client 3.1.3 ## Bug Fixes diff --git a/src/core/QueryInfo.ts b/src/core/QueryInfo.ts index 333a22fbce6..6c2fd8ce2f1 100644 --- a/src/core/QueryInfo.ts +++ b/src/core/QueryInfo.ts @@ -10,6 +10,7 @@ import { ObservableSubscription, isNonEmptyArray, graphQLResultHasError, + canUseWeakMap, } from '../utilities'; import { NetworkStatus, @@ -24,6 +25,10 @@ export type QueryStoreValue = Pick; +const cacheEvictCounts = new ( + canUseWeakMap ? WeakMap : Map +), number>(); + // A QueryInfo object represents a single query managed by the // QueryManager, which tracks all QueryInfo objects by queryId in its // this.queries Map. QueryInfo objects store the latest results and errors @@ -46,7 +51,28 @@ export class QueryInfo { networkError?: Error | null; graphQLErrors?: ReadonlyArray; - constructor(private cache: ApolloCache) {} + constructor(private cache: ApolloCache) { + // Track how often cache.evict is called, since we want eviction to + // override the feud-stopping logic in the markResult method, by + // causing shouldWrite to return true. Wrapping the cache.evict method + // is a bit of a hack, but it saves us from having to make eviction + // counting an official part of the ApolloCache API. + if (!cacheEvictCounts.has(cache) && cache.evict) { + cacheEvictCounts.set(cache, 0); + const originalEvict = cache.evict; + cache.evict = function evict() { + cacheEvictCounts.set( + cache, + // The %1e15 allows the count to wrap around to 0 safely every + // quadrillion evictions, so there's no risk of overflow. To be + // clear, this is more of a pedantic principle than something + // that matters in any conceivable practical scenario. + (cacheEvictCounts.get(cache)! + 1) % 1e15, + ); + return originalEvict.apply(this, arguments); + }; + } + } public init(query: { document: DocumentNode; @@ -204,8 +230,27 @@ export class QueryInfo { } } - private lastWrittenResult?: FetchResult; - private lastWrittenVars?: WatchQueryOptions["variables"]; + private lastWrite?: { + result: FetchResult; + variables: WatchQueryOptions["variables"]; + evictCount: number | undefined; + }; + + private shouldWrite( + result: FetchResult, + variables: WatchQueryOptions["variables"], + ) { + const { lastWrite } = this; + return !( + lastWrite && + // If cache.evict has been called since the last time we wrote this + // data into the cache, there's a chance writing this result into + // the cache will repair what was evicted. + lastWrite.evictCount === cacheEvictCounts.get(this.cache) && + equal(variables, lastWrite.variables) && + equal(result.data, lastWrite.result.data) + ); + } public markResult( result: FetchResult, @@ -235,9 +280,19 @@ 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 => { - if (this.lastWrittenResult && - equal(result.data, this.lastWrittenResult.data) && - equal(options.variables, this.lastWrittenVars)) { + if (this.shouldWrite(result, options.variables)) { + cache.writeQuery({ + query: this.document!, + data: result.data as T, + variables: options.variables, + }); + + this.lastWrite = { + result, + variables: options.variables, + evictCount: cacheEvictCounts.get(this.cache), + }; + } else { // 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 @@ -278,14 +333,6 @@ export class QueryInfo { } // 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({ @@ -311,7 +358,7 @@ export class QueryInfo { }); } else { - this.lastWrittenResult = this.lastWrittenVars = void 0; + this.lastWrite = void 0; } } } @@ -323,7 +370,7 @@ export class QueryInfo { public markError(error: ApolloError) { this.networkStatus = NetworkStatus.error; - this.lastWrittenResult = this.lastWrittenVars = void 0; + this.lastWrite = void 0; if (error.graphQLErrors) { this.graphQLErrors = error.graphQLErrors;