From dba08b779f5b4f82308163c503609975e0e40578 Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Tue, 21 Sep 2021 11:43:42 -0400 Subject: [PATCH 1/7] Default options.canonizeResults to false. --- src/cache/core/types/DataProxy.ts | 4 ++-- src/cache/inmemory/readFromStore.ts | 2 +- src/core/QueryInfo.ts | 9 ++++++--- src/core/watchQueryOptions.ts | 2 +- 4 files changed, 10 insertions(+), 7 deletions(-) diff --git a/src/cache/core/types/DataProxy.ts b/src/cache/core/types/DataProxy.ts index 8e04d01a5df..8711a771b1b 100644 --- a/src/cache/core/types/DataProxy.ts +++ b/src/cache/core/types/DataProxy.ts @@ -70,7 +70,7 @@ export namespace DataProxy { /** * Whether to canonize cache results before returning them. Canonization * takes some extra time, but it speeds up future deep equality comparisons. - * Defaults to true. + * Defaults to false. */ canonizeResults?: boolean; } @@ -91,7 +91,7 @@ export namespace DataProxy { /** * Whether to canonize cache results before returning them. Canonization * takes some extra time, but it speeds up future deep equality comparisons. - * Defaults to true. + * Defaults to false. */ canonizeResults?: boolean; } diff --git a/src/cache/inmemory/readFromStore.ts b/src/cache/inmemory/readFromStore.ts index 46b8aeb147c..42b0e98c346 100644 --- a/src/cache/inmemory/readFromStore.ts +++ b/src/cache/inmemory/readFromStore.ts @@ -231,7 +231,7 @@ export class StoreReader { rootId = 'ROOT_QUERY', variables, returnPartialData = true, - canonizeResults = true, + canonizeResults = false, }: DiffQueryAgainstStoreOptions): Cache.DiffResult { const policies = this.config.cache.policies; diff --git a/src/core/QueryInfo.ts b/src/core/QueryInfo.ts index ea23f374a79..9190b4921f2 100644 --- a/src/core/QueryInfo.ts +++ b/src/core/QueryInfo.ts @@ -192,14 +192,17 @@ export class QueryInfo { } private getDiffOptions(variables = this.variables): Cache.DiffOptions { - const oq = this.observableQuery; - return { + const options: Cache.DiffOptions = { query: this.document!, variables, returnPartialData: true, optimistic: true, - canonizeResults: !oq || oq.options.canonizeResults !== false, }; + const canonizeResults = this.observableQuery?.options.canonizeResults; + if (typeof canonizeResults === "boolean") { + options.canonizeResults = canonizeResults; + } + return options; } setDiff(diff: Cache.DiffResult | null) { diff --git a/src/core/watchQueryOptions.ts b/src/core/watchQueryOptions.ts index dbcbd4bb7dd..6293f7e4d51 100644 --- a/src/core/watchQueryOptions.ts +++ b/src/core/watchQueryOptions.ts @@ -106,7 +106,7 @@ export interface QueryOptions { /** * Whether to canonize cache results before returning them. Canonization * takes some extra time, but it speeds up future deep equality comparisons. - * Defaults to true. + * Defaults to false. */ canonizeResults?: boolean; } From 975da0ea9e470986388b104c5170345f40763b7d Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Tue, 21 Sep 2021 12:00:43 -0400 Subject: [PATCH 2/7] Allow passing default value for canonizeResults to InMemoryCache. --- src/cache/inmemory/inMemoryCache.ts | 3 +++ src/cache/inmemory/readFromStore.ts | 5 ++++- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/src/cache/inmemory/inMemoryCache.ts b/src/cache/inmemory/inMemoryCache.ts index cf058d7889e..a10a20151bb 100644 --- a/src/cache/inmemory/inMemoryCache.ts +++ b/src/cache/inmemory/inMemoryCache.ts @@ -36,6 +36,7 @@ export interface InMemoryCacheConfig extends ApolloReducerConfig { possibleTypes?: PossibleTypesMap; typePolicies?: TypePolicies; resultCacheMaxSize?: number; + canonizeResults?: boolean; } type BroadcastOptions = Pick< @@ -48,6 +49,7 @@ const defaultConfig: InMemoryCacheConfig = { dataIdFromObject: defaultDataIdFromObject, addTypename: true, resultCaching: true, + canonizeResults: false, typePolicies: {}, }; @@ -121,6 +123,7 @@ export class InMemoryCache extends ApolloCache { cache: this, addTypename: this.addTypename, resultCacheMaxSize: this.config.resultCacheMaxSize, + canonizeResults: this.config.canonizeResults, canon: resetResultIdentities ? void 0 : previousReader && previousReader.canon, diff --git a/src/cache/inmemory/readFromStore.ts b/src/cache/inmemory/readFromStore.ts index 42b0e98c346..98c5e1ac5c3 100644 --- a/src/cache/inmemory/readFromStore.ts +++ b/src/cache/inmemory/readFromStore.ts @@ -86,6 +86,7 @@ export interface StoreReaderConfig { cache: InMemoryCache, addTypename?: boolean; resultCacheMaxSize?: number; + canonizeResults?: boolean; canon?: ObjectCanon; } @@ -128,6 +129,7 @@ export class StoreReader { cache: InMemoryCache, addTypename: boolean; resultCacheMaxSize?: number; + canonizeResults: boolean; }; private knownResults = new ( @@ -143,6 +145,7 @@ export class StoreReader { this.config = { ...config, addTypename: config.addTypename !== false, + canonizeResults: !!config.canonizeResults, }; this.canon = config.canon || new ObjectCanon; @@ -231,7 +234,7 @@ export class StoreReader { rootId = 'ROOT_QUERY', variables, returnPartialData = true, - canonizeResults = false, + canonizeResults = this.config.canonizeResults, }: DiffQueryAgainstStoreOptions): Cache.DiffResult { const policies = this.config.cache.policies; From b6b898c300c6071659eb43ddae74ef3c279ae6ce Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Tue, 21 Sep 2021 11:54:25 -0400 Subject: [PATCH 3/7] Fix tests after disabling canonization by default. --- scripts/memory/tests.js | 23 ++++++++++++------- src/cache/inmemory/__tests__/cache.ts | 16 ++++++++++--- src/cache/inmemory/__tests__/entityStore.ts | 16 +++++++++---- src/cache/inmemory/__tests__/optimistic.ts | 2 ++ src/cache/inmemory/__tests__/policies.ts | 1 + src/cache/inmemory/__tests__/readFromStore.ts | 5 +++- src/core/__tests__/QueryManager/index.ts | 3 ++- src/react/hooks/__tests__/useQuery.test.tsx | 1 + 8 files changed, 49 insertions(+), 18 deletions(-) diff --git a/scripts/memory/tests.js b/scripts/memory/tests.js index 473621a98cf..b1fc6ed0b3e 100644 --- a/scripts/memory/tests.js +++ b/scripts/memory/tests.js @@ -155,13 +155,18 @@ describe("garbage collection", () => { }, }, }, + // Explicitly disable canonization to test that it can be overridden. + canonizeResults: false, }); const client = new ApolloClient({ cache }); (function () { const query = gql`query { local }`; - const obsQuery = client.watchQuery({ query }); + const obsQuery = client.watchQuery({ + query, + canonizeResults: true, + }); function register(suffix) { const reader = cache["storeReader"]; @@ -177,16 +182,18 @@ describe("garbage collection", () => { local: "hello", }); - assert.strictEqual( - cache.readQuery({ query }), - result.data, - ); + const read = () => cache.readQuery({ + query, + canonizeResults: true, + }); + + assert.strictEqual(read(), result.data); assert.deepStrictEqual(cache.gc(), []); // Nothing changes because we merely called cache.gc(). assert.strictEqual( - cache.readQuery({ query }), + read(), result.data, ); @@ -199,7 +206,7 @@ describe("garbage collection", () => { register(2); - const dataAfterResetWithSameCanon = cache.readQuery({ query }); + const dataAfterResetWithSameCanon = read(); assert.strictEqual(dataAfterResetWithSameCanon, result.data); assert.deepStrictEqual(cache.gc({ @@ -211,7 +218,7 @@ describe("garbage collection", () => { register(3); - const dataAfterFullReset = cache.readQuery({ query }); + const dataAfterFullReset = read(); assert.notStrictEqual(dataAfterFullReset, result.data); assert.deepStrictEqual(dataAfterFullReset, result.data); diff --git a/src/cache/inmemory/__tests__/cache.ts b/src/cache/inmemory/__tests__/cache.ts index 08dbb361381..abd2153e9aa 100644 --- a/src/cache/inmemory/__tests__/cache.ts +++ b/src/cache/inmemory/__tests__/cache.ts @@ -2013,6 +2013,7 @@ describe("InMemoryCache#modify", () => { it("should allow invalidation using details.INVALIDATE", () => { const cache = new InMemoryCache({ + canonizeResults: true, typePolicies: { Book: { keyFields: ["isbn"], @@ -2946,7 +2947,10 @@ describe("ReactiveVar and makeVar", () => { it("should work with resultCaching disabled (unusual)", () => { const { cache, nameVar, query } = makeCacheAndVar(false); - const result1 = cache.readQuery({ query }); + const result1 = cache.readQuery({ + query, + canonizeResults: true, + }); expect(result1).toEqual({ onCall: { __typename: "Person", @@ -2954,14 +2958,20 @@ describe("ReactiveVar and makeVar", () => { }, }); - const result2 = cache.readQuery({ query }); + const result2 = cache.readQuery({ + query, + canonizeResults: true, + }); expect(result2).toEqual(result1); expect(result2).toBe(result1); expect(nameVar()).toBe("Ben"); expect(nameVar("Hugh")).toBe("Hugh"); - const result3 = cache.readQuery({ query }); + const result3 = cache.readQuery({ + query, + canonizeResults: true, + }); expect(result3).toEqual({ onCall: { __typename: "Person", diff --git a/src/cache/inmemory/__tests__/entityStore.ts b/src/cache/inmemory/__tests__/entityStore.ts index a02febc9dfe..6707e6efd48 100644 --- a/src/cache/inmemory/__tests__/entityStore.ts +++ b/src/cache/inmemory/__tests__/entityStore.ts @@ -170,14 +170,18 @@ describe('EntityStore', () => { }, }); - const resultBeforeGC = cache.readQuery({ query }); + function read() { + return cache.readQuery({ query, canonizeResults: true }); + } + + const resultBeforeGC = read(); expect(cache.gc().sort()).toEqual([ 'Author:Ray Bradbury', 'Book:9781451673319', ]); - const resultAfterGC = cache.readQuery({ query }); + const resultAfterGC = read(); expect(resultBeforeGC).toBe(resultAfterGC); expect(cache.extract()).toEqual({ @@ -207,7 +211,7 @@ describe('EntityStore', () => { resetResultCache: true, })).toEqual([]); expect(cache["storeReader"]).not.toBe(originalReader); - const resultAfterResetResultCache = cache.readQuery({ query }); + const resultAfterResetResultCache = read(); expect(resultAfterResetResultCache).toBe(resultBeforeGC); expect(resultAfterResetResultCache).toBe(resultAfterGC); @@ -217,7 +221,7 @@ describe('EntityStore', () => { resetResultIdentities: true, })).toEqual([]); - const resultAfterFullGC = cache.readQuery({ query }); + const resultAfterFullGC = read(); expect(resultAfterFullGC).toEqual(resultBeforeGC); expect(resultAfterFullGC).toEqual(resultAfterGC); // These !== relations are triggered by passing resetResultIdentities:true @@ -225,7 +229,7 @@ describe('EntityStore', () => { expect(resultAfterFullGC).not.toBe(resultBeforeGC); expect(resultAfterFullGC).not.toBe(resultAfterGC); // Result caching immediately begins working again after the intial reset. - expect(cache.readQuery({ query })).toBe(resultAfterFullGC); + expect(read()).toBe(resultAfterFullGC); // Go back to the pre-GC snapshot. cache.restore(snapshot); @@ -1049,6 +1053,7 @@ describe('EntityStore', () => { `; const cache = new InMemoryCache({ + canonizeResults: true, typePolicies: { Query: { fields: { @@ -2391,6 +2396,7 @@ describe('EntityStore', () => { const isbnsWeHaveRead: string[] = []; const cache = new InMemoryCache({ + canonizeResults: true, typePolicies: { Query: { fields: { diff --git a/src/cache/inmemory/__tests__/optimistic.ts b/src/cache/inmemory/__tests__/optimistic.ts index 8030d43ed49..79759bff39a 100644 --- a/src/cache/inmemory/__tests__/optimistic.ts +++ b/src/cache/inmemory/__tests__/optimistic.ts @@ -6,6 +6,7 @@ describe('optimistic cache layers', () => { it('return === results for repeated reads', () => { const cache = new InMemoryCache({ resultCaching: true, + canonizeResults: true, dataIdFromObject(value: any) { switch (value && value.__typename) { case 'Book': @@ -204,6 +205,7 @@ describe('optimistic cache layers', () => { it('dirties appropriate IDs when optimistic layers are removed', () => { const cache = new InMemoryCache({ resultCaching: true, + canonizeResults: true, dataIdFromObject(value: any) { switch (value && value.__typename) { case 'Book': diff --git a/src/cache/inmemory/__tests__/policies.ts b/src/cache/inmemory/__tests__/policies.ts index dc1db912e90..5a56f1a4895 100644 --- a/src/cache/inmemory/__tests__/policies.ts +++ b/src/cache/inmemory/__tests__/policies.ts @@ -4947,6 +4947,7 @@ describe("type policies", function () { function readFirstBookResult() { return cache.readQuery<{ author: any }>({ query: firstBookQuery, + canonizeResults: true, })!; } diff --git a/src/cache/inmemory/__tests__/readFromStore.ts b/src/cache/inmemory/__tests__/readFromStore.ts index ffc2cb2de84..95f34c2c0e9 100644 --- a/src/cache/inmemory/__tests__/readFromStore.ts +++ b/src/cache/inmemory/__tests__/readFromStore.ts @@ -1377,6 +1377,7 @@ describe('reading from the store', () => { withErrorSpy(it, "propagates eviction signals to parent queries", () => { const cache = new InMemoryCache({ + canonizeResults: true, typePolicies: { Deity: { keyFields: ["name"], @@ -1866,7 +1867,9 @@ describe('reading from the store', () => { }); it("returns === results for different queries", function () { - const cache = new InMemoryCache; + const cache = new InMemoryCache({ + canonizeResults: true, + }); const aQuery: TypedDocumentNode<{ a: string[]; diff --git a/src/core/__tests__/QueryManager/index.ts b/src/core/__tests__/QueryManager/index.ts index 1342260f38f..0ab3084079e 100644 --- a/src/core/__tests__/QueryManager/index.ts +++ b/src/core/__tests__/QueryManager/index.ts @@ -834,7 +834,7 @@ describe('QueryManager', () => { }); itAsync('will return referentially equivalent data if nothing changed in a refetch', (resolve, reject) => { - const request = { + const request: WatchQueryOptions = { query: gql` { a @@ -850,6 +850,7 @@ describe('QueryManager', () => { } `, notifyOnNetworkStatusChange: false, + canonizeResults: true, }; const data1 = { diff --git a/src/react/hooks/__tests__/useQuery.test.tsx b/src/react/hooks/__tests__/useQuery.test.tsx index 5bcb6822af4..c84d46e2512 100644 --- a/src/react/hooks/__tests__/useQuery.test.tsx +++ b/src/react/hooks/__tests__/useQuery.test.tsx @@ -3015,6 +3015,7 @@ describe('useQuery Hook', () => { describe('canonical cache results', () => { it('can be disabled via useQuery options', async () => { const cache = new InMemoryCache({ + canonizeResults: true, typePolicies: { Result: { keyFields: false, From ff0e004d82d92413358613222391e122b6347a6b Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Wed, 22 Sep 2021 15:55:23 -0400 Subject: [PATCH 4/7] Make it easier to change the default options.canonizeResults value. --- src/cache/inmemory/inMemoryCache.ts | 14 +++++++++++--- src/cache/inmemory/readFromStore.ts | 4 ++-- 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/src/cache/inmemory/inMemoryCache.ts b/src/cache/inmemory/inMemoryCache.ts index a10a20151bb..9dc46c69773 100644 --- a/src/cache/inmemory/inMemoryCache.ts +++ b/src/cache/inmemory/inMemoryCache.ts @@ -45,14 +45,22 @@ type BroadcastOptions = Pick< | "onWatchUpdated" > -const defaultConfig: InMemoryCacheConfig = { +const defaultConfig = { dataIdFromObject: defaultDataIdFromObject, addTypename: true, resultCaching: true, + // Thanks to the shouldCanonizeResults helper, this should be the only line + // you have to change to reenable canonization by default in the future. canonizeResults: false, - typePolicies: {}, }; +export function shouldCanonizeResults( + config: Pick, +): boolean { + const value = config.canonizeResults; + return value === void 0 ? defaultConfig.canonizeResults : value; +} + export class InMemoryCache extends ApolloCache { private data: EntityStore; private optimisticData: EntityStore; @@ -123,7 +131,7 @@ export class InMemoryCache extends ApolloCache { cache: this, addTypename: this.addTypename, resultCacheMaxSize: this.config.resultCacheMaxSize, - canonizeResults: this.config.canonizeResults, + canonizeResults: shouldCanonizeResults(this.config), canon: resetResultIdentities ? void 0 : previousReader && previousReader.canon, diff --git a/src/cache/inmemory/readFromStore.ts b/src/cache/inmemory/readFromStore.ts index 98c5e1ac5c3..ada1a83f3a1 100644 --- a/src/cache/inmemory/readFromStore.ts +++ b/src/cache/inmemory/readFromStore.ts @@ -37,7 +37,7 @@ import { import { maybeDependOnExistenceOfEntity, supportsResultCaching } from './entityStore'; import { getTypenameFromStoreObject } from './helpers'; import { Policies } from './policies'; -import { InMemoryCache } from './inMemoryCache'; +import { shouldCanonizeResults, InMemoryCache } from './inMemoryCache'; import { MissingFieldError } from '../core/types/common'; import { canonicalStringify, ObjectCanon } from './object-canon'; @@ -145,7 +145,7 @@ export class StoreReader { this.config = { ...config, addTypename: config.addTypename !== false, - canonizeResults: !!config.canonizeResults, + canonizeResults: shouldCanonizeResults(config), }; this.canon = config.canon || new ObjectCanon; From 4ca9bf046a86436e1c6113cdadcf2e3f7b12931c Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Wed, 22 Sep 2021 15:55:53 -0400 Subject: [PATCH 5/7] Simplify processing of canonizeResults in QueryInfo#getDiffOptions. --- src/core/QueryInfo.ts | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/core/QueryInfo.ts b/src/core/QueryInfo.ts index 9190b4921f2..c76014d1ff7 100644 --- a/src/core/QueryInfo.ts +++ b/src/core/QueryInfo.ts @@ -192,17 +192,13 @@ export class QueryInfo { } private getDiffOptions(variables = this.variables): Cache.DiffOptions { - const options: Cache.DiffOptions = { + return { query: this.document!, variables, returnPartialData: true, optimistic: true, + canonizeResults: this.observableQuery?.options.canonizeResults, }; - const canonizeResults = this.observableQuery?.options.canonizeResults; - if (typeof canonizeResults === "boolean") { - options.canonizeResults = canonizeResults; - } - return options; } setDiff(diff: Cache.DiffResult | null) { From 124cd1cc48eb35549b7b3d25d658ea20c19eeb8c Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Wed, 22 Sep 2021 16:23:16 -0400 Subject: [PATCH 6/7] Use `compact` utility function to normalize `InMemoryCacheConfig`. --- src/cache/inmemory/inMemoryCache.ts | 3 ++- src/cache/inmemory/readFromStore.ts | 6 +++--- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/src/cache/inmemory/inMemoryCache.ts b/src/cache/inmemory/inMemoryCache.ts index 9dc46c69773..6838636abe0 100644 --- a/src/cache/inmemory/inMemoryCache.ts +++ b/src/cache/inmemory/inMemoryCache.ts @@ -13,6 +13,7 @@ import { StoreObject, Reference, isReference, + compact, } from '../../utilities'; import { ApolloReducerConfig, @@ -87,7 +88,7 @@ export class InMemoryCache extends ApolloCache { constructor(config: InMemoryCacheConfig = {}) { super(); - this.config = { ...defaultConfig, ...config }; + this.config = compact(defaultConfig, config); this.addTypename = !!this.config.addTypename; this.policies = new Policies({ diff --git a/src/cache/inmemory/readFromStore.ts b/src/cache/inmemory/readFromStore.ts index ada1a83f3a1..820e62ce8d1 100644 --- a/src/cache/inmemory/readFromStore.ts +++ b/src/cache/inmemory/readFromStore.ts @@ -27,6 +27,7 @@ import { maybeDeepFreeze, isNonNullObject, canUseWeakMap, + compact, } from '../../utilities'; import { Cache } from '../core/types/Cache'; import { @@ -142,11 +143,10 @@ export class StoreReader { } constructor(config: StoreReaderConfig) { - this.config = { - ...config, + this.config = compact(config, { addTypename: config.addTypename !== false, canonizeResults: shouldCanonizeResults(config), - }; + }); this.canon = config.canon || new ObjectCanon; From 2d1a439bb3c3aa0b1ad5e0ce22aea2e698531086 Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Wed, 22 Sep 2021 16:44:59 -0400 Subject: [PATCH 7/7] Mention PR #8822 in CHANGELOG.md. --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index f88439d8e61..096075b1a9e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,9 @@ ### Bug Fixes +- Disable `InMemoryCache` [result object canonization](https://github.com/apollographql/apollo-client/pull/7439) by default, to prevent unexpected memory growth and/or reuse of object references, with multiple ways to reenable it (per-cache, per-query, or a mixture of both).
+ [@benjamn](https://github.com/benjamn) in [#8822](https://github.com/apollographql/apollo-client/pull/8822) + - Clear `InMemoryCache` `watches` set when `cache.reset()` called.
[@benjamn](https://github.com/benjamn) in [#8826](https://github.com/apollographql/apollo-client/pull/8826)