From 78d0d78c3b2ce419da76c31e98346ecf5112b3bc Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Tue, 24 Aug 2021 17:41:05 -0400 Subject: [PATCH 1/6] Make `cache.batch` return the result of calling `options.update`. --- src/cache/core/__tests__/cache.ts | 1 + src/cache/core/cache.ts | 9 ++- src/cache/core/types/Cache.ts | 6 +- src/cache/inmemory/__tests__/cache.ts | 100 +++++++++++++++++++++++++- src/cache/inmemory/inMemoryCache.ts | 9 ++- 5 files changed, 115 insertions(+), 10 deletions(-) diff --git a/src/cache/core/__tests__/cache.ts b/src/cache/core/__tests__/cache.ts index baa090c1af4..5f07bb28e97 100644 --- a/src/cache/core/__tests__/cache.ts +++ b/src/cache/core/__tests__/cache.ts @@ -21,6 +21,7 @@ class TestCache extends ApolloCache { } public performTransaction(transaction: (c: ApolloCache) => void): void { + transaction(this); } public read(query: Cache.ReadOptions): T | null { diff --git a/src/cache/core/cache.ts b/src/cache/core/cache.ts index f15a142f8f8..8f65cbd26a1 100644 --- a/src/cache/core/cache.ts +++ b/src/cache/core/cache.ts @@ -59,11 +59,16 @@ export abstract class ApolloCache implements DataProxy { // provide a default batch implementation that's just another way of calling // performTransaction. Subclasses of ApolloCache (such as InMemoryCache) can // override the batch method to do more interesting things with its options. - public batch(options: Cache.BatchOptions) { + public batch(options: Cache.BatchOptions): U { const optimisticId = typeof options.optimistic === "string" ? options.optimistic : options.optimistic === false ? null : void 0; - this.performTransaction(options.update, optimisticId); + let updateResult: U; + this.performTransaction( + () => updateResult = options.update(this), + optimisticId, + ); + return updateResult!; } public abstract performTransaction( diff --git a/src/cache/core/types/Cache.ts b/src/cache/core/types/Cache.ts index fe9b9d83bbb..9464d330c2b 100644 --- a/src/cache/core/types/Cache.ts +++ b/src/cache/core/types/Cache.ts @@ -54,10 +54,10 @@ export namespace Cache { broadcast?: boolean; } - export interface BatchOptions> { + export interface BatchOptions, U = void> { // Same as the first parameter of performTransaction, except the cache // argument will have the subclass type rather than ApolloCache. - update(cache: C): void; + update(cache: C): U; // Passing a string for this option creates a new optimistic layer, with the // given string as its layer.id, just like passing a string for the @@ -66,7 +66,7 @@ export namespace Cache { // against the current top layer of the cache), and passing false is the // same as passing null (running the operation against root/non-optimistic // cache data). - optimistic: string | boolean; + optimistic?: string | boolean; // If you specify the ID of an optimistic layer using this option, that // layer will be removed as part of the batch transaction, triggering at diff --git a/src/cache/inmemory/__tests__/cache.ts b/src/cache/inmemory/__tests__/cache.ts index 7794721ecae..cae97479d98 100644 --- a/src/cache/inmemory/__tests__/cache.ts +++ b/src/cache/inmemory/__tests__/cache.ts @@ -1377,7 +1377,7 @@ describe('Cache', () => { const dirtied = new Map>(); - cache.batch({ + const aUpdateResult = cache.batch({ update(cache) { cache.writeQuery({ query: aQuery, @@ -1385,12 +1385,14 @@ describe('Cache', () => { a: "ay", }, }); + return "aQuery updated"; }, optimistic: true, onWatchUpdated(w, diff) { dirtied.set(w, diff); }, }); + expect(aUpdateResult).toBe("aQuery updated"); expect(dirtied.size).toBe(2); expect(dirtied.has(aInfo.watch)).toBe(true); @@ -1418,7 +1420,7 @@ describe('Cache', () => { dirtied.clear(); - cache.batch({ + const bUpdateResult = cache.batch({ update(cache) { cache.writeQuery({ query: bQuery, @@ -1426,12 +1428,14 @@ describe('Cache', () => { b: "bee", }, }); + // Not returning anything, so beUpdateResult will be undefined. }, optimistic: true, onWatchUpdated(w, diff) { dirtied.set(w, diff); }, }); + expect(bUpdateResult).toBeUndefined(); expect(dirtied.size).toBe(2); expect(dirtied.has(aInfo.watch)).toBe(false); @@ -1654,6 +1658,98 @@ describe('Cache', () => { abInfo.cancel(); bInfo.cancel(); }); + + it("returns options.update result for optimistic and non-optimistic batches", () => { + const cache = new InMemoryCache; + const expected = Symbol.for("expected"); + + expect(cache.batch({ + optimistic: false, + update(c) { + c.writeQuery({ + query: gql`query { value }`, + data: { value: 12345 }, + }); + return expected; + }, + })).toBe(expected); + + expect(cache.batch({ + optimistic: false, + update(c) { + c.reset(); + return expected; + }, + })).toBe(expected); + + expect(cache.batch({ + optimistic: false, + update(c) { + c.writeQuery({ + query: gql`query { optimistic }`, + data: { optimistic: false }, + }); + return expected; + }, + onWatchUpdated() { + throw new Error("onWatchUpdated should not have been called"); + }, + })).toBe(expected); + + expect(cache.batch({ + optimistic: true, + update(c) { + return expected; + }, + })).toBe(expected); + + expect(cache.batch({ + optimistic: true, + update(c) { + c.writeQuery({ + query: gql`query { optimistic }`, + data: { optimistic: true }, + }); + return expected; + }, + onWatchUpdated() { + throw new Error("onWatchUpdated should not have been called"); + }, + })).toBe(expected); + + expect(cache.batch({ + // The optimistic option defaults to true. + // optimistic: true, + update(c) { + return expected; + }, + })).toBe(expected); + + expect(cache.batch({ + optimistic: "some optimistic ID", + update(c) { + expect(c.readQuery({ + query: gql`query { __typename }`, + })).toEqual({ __typename: "Query" }); + return expected; + }, + })).toBe(expected); + + const optimisticId = "some optimistic ID"; + expect(cache.batch({ + optimistic: optimisticId, + update(c) { + c.writeQuery({ + query: gql`query { optimistic }`, + data: { optimistic: optimisticId }, + }); + return expected; + }, + onWatchUpdated() { + throw new Error("onWatchUpdated should not have been called"); + }, + })).toBe(expected); + }); }); describe('performTransaction', () => { diff --git a/src/cache/inmemory/inMemoryCache.ts b/src/cache/inmemory/inMemoryCache.ts index c9b83d89b7a..45cd5527811 100644 --- a/src/cache/inmemory/inMemoryCache.ts +++ b/src/cache/inmemory/inMemoryCache.ts @@ -384,7 +384,7 @@ export class InMemoryCache extends ApolloCache { private txCount = 0; - public batch(options: Cache.BatchOptions) { + public batch(options: Cache.BatchOptions): U { const { update, optimistic = true, @@ -392,14 +392,15 @@ export class InMemoryCache extends ApolloCache { onWatchUpdated, } = options; - const perform = (layer?: EntityStore) => { + let updateResult: U; + const perform = (layer?: EntityStore): U => { const { data, optimisticData } = this; ++this.txCount; if (layer) { this.data = this.optimisticData = layer; } try { - update(this); + return updateResult = update(this); } finally { --this.txCount; this.data = data; @@ -478,6 +479,8 @@ export class InMemoryCache extends ApolloCache { // options.onWatchUpdated. this.broadcastWatches(options); } + + return updateResult!; } public performTransaction( From 8dd371be5ea81cd6017c81a5ed7cf7efa9b4e37b Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Thu, 26 Aug 2021 18:37:01 -0400 Subject: [PATCH 2/6] Use `cache.batch` in `cache.update{Query,Fragment}`. --- src/cache/core/cache.ts | 28 ++++++++++++++++++---------- 1 file changed, 18 insertions(+), 10 deletions(-) diff --git a/src/cache/core/cache.ts b/src/cache/core/cache.ts index 8f65cbd26a1..eef9d6a2ba0 100644 --- a/src/cache/core/cache.ts +++ b/src/cache/core/cache.ts @@ -176,21 +176,29 @@ export abstract class ApolloCache implements DataProxy { options: Cache.UpdateQueryOptions, update: (data: TData | null) => TData | null | void, ): TData | null { - const value = this.readQuery(options); - const data = update(value); - if (data === void 0 || data === null) return value; - this.writeQuery({ ...options, data }); - return data; + return this.batch({ + update(cache) { + const value = cache.readQuery(options); + const data = update(value); + if (data === void 0 || data === null) return value; + cache.writeQuery({ ...options, data }); + return data; + }, + }); } public updateFragment( options: Cache.UpdateFragmentOptions, update: (data: TData | null) => TData | null | void, ): TData | null { - const value = this.readFragment(options); - const data = update(value); - if (data === void 0 || data === null) return value; - this.writeFragment({ ...options, data }); - return data; + return this.batch({ + update(cache) { + const value = cache.readFragment(options); + const data = update(value); + if (data === void 0 || data === null) return value; + cache.writeFragment({ ...options, data }); + return data; + }, + }); } } From 0a6dfa55ecb98e875a22a19e01feb1ab25420092 Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Tue, 24 Aug 2021 18:54:12 -0400 Subject: [PATCH 3/6] Mention PR #8696 in CHANGELOG.md. --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index be703edf985..3d64446f8b7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -36,6 +36,9 @@ - A new nested entry point called `@apollo/client/testing/core` has been created. Importing from this entry point instead of `@apollo/client/testing` excludes any React-related dependencies.
[@wassim-k](https://github.com/wassim-k) in [#8687](https://github.com/apollographql/apollo-client/pull/8687) +- Make `cache.batch` return the result of calling the `options.update` function.
+ [@benjamn](https://github.com/benjamn) in [#8696](https://github.com/apollographql/apollo-client/pull/8696) + ### React Refactoring #### Bug Fixes (due to [@brainkim](https://github.com/brainkim) in [#8596](https://github.com/apollographql/apollo-client/pull/8596)): From bb1bda129f2bb8e16476ba1f3f1d7b63b3cc725f Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Thu, 26 Aug 2021 19:33:33 -0400 Subject: [PATCH 4/6] Improve generic type inference for cache.watch. --- src/cache/core/cache.ts | 14 ++++++++------ src/cache/core/types/Cache.ts | 22 +++++++++++++--------- src/cache/inmemory/inMemoryCache.ts | 8 ++++++-- src/core/QueryInfo.ts | 4 ++-- 4 files changed, 29 insertions(+), 19 deletions(-) diff --git a/src/cache/core/cache.ts b/src/cache/core/cache.ts index eef9d6a2ba0..4ce39cf9621 100644 --- a/src/cache/core/cache.ts +++ b/src/cache/core/cache.ts @@ -14,14 +14,16 @@ export type Transaction = (c: ApolloCache) => void; export abstract class ApolloCache implements DataProxy { // required to implement // core API - public abstract read( - query: Cache.ReadOptions, - ): T | null; - public abstract write( - write: Cache.WriteOptions, + public abstract read( + query: Cache.ReadOptions, + ): TData | null; + public abstract write( + write: Cache.WriteOptions, ): Reference | undefined; public abstract diff(query: Cache.DiffOptions): Cache.DiffResult; - public abstract watch(watch: Cache.WatchOptions): () => void; + public abstract watch( + watch: Cache.WatchOptions, + ): () => void; public abstract reset(): Promise; // Remove whole objects from the cache by passing just options.id, or diff --git a/src/cache/core/types/Cache.ts b/src/cache/core/types/Cache.ts index 9464d330c2b..4b3054aa705 100644 --- a/src/cache/core/types/Cache.ts +++ b/src/cache/core/types/Cache.ts @@ -3,9 +3,9 @@ import { Modifier, Modifiers } from './common'; import { ApolloCache } from '../cache'; export namespace Cache { - export type WatchCallback = ( - diff: Cache.DiffResult, - lastDiff?: Cache.DiffResult, + export type WatchCallback = ( + diff: Cache.DiffResult, + lastDiff?: Cache.DiffResult, ) => void; export interface ReadOptions @@ -25,19 +25,23 @@ export namespace Cache { result: TResult; } - export interface DiffOptions extends ReadOptions { + export interface DiffOptions< + TData = any, + TVariables = any, + > extends ReadOptions { // The DiffOptions interface is currently just an alias for // ReadOptions, though DiffOptions used to be responsible for // declaring the returnPartialData option. } export interface WatchOptions< - Watcher extends object = Record - > extends ReadOptions { - watcher?: Watcher; + TData = any, + TVariables = any, + > extends ReadOptions { + watcher?: object; immediate?: boolean; - callback: WatchCallback; - lastDiff?: DiffResult; + callback: WatchCallback; + lastDiff?: DiffResult; } export interface EvictOptions { diff --git a/src/cache/inmemory/inMemoryCache.ts b/src/cache/inmemory/inMemoryCache.ts index 45cd5527811..f36b125a369 100644 --- a/src/cache/inmemory/inMemoryCache.ts +++ b/src/cache/inmemory/inMemoryCache.ts @@ -247,7 +247,9 @@ export class InMemoryCache extends ApolloCache { } } - public diff(options: Cache.DiffOptions): Cache.DiffResult { + public diff( + options: Cache.DiffOptions, + ): Cache.DiffResult { return this.storeReader.diffQueryAgainstStore({ ...options, store: options.optimistic ? this.optimisticData : this.data, @@ -256,7 +258,9 @@ export class InMemoryCache extends ApolloCache { }); } - public watch(watch: Cache.WatchOptions): () => void { + public watch( + watch: Cache.WatchOptions, + ): () => void { if (!this.watches.size) { // In case we previously called forgetCache(this) because // this.watches became empty (see below), reattach this cache to any diff --git a/src/core/QueryInfo.ts b/src/core/QueryInfo.ts index ea23f374a79..797437c1a62 100644 --- a/src/core/QueryInfo.ts +++ b/src/core/QueryInfo.ts @@ -296,7 +296,7 @@ export class QueryInfo { // updateWatch method. private cancel() {} - private lastWatch?: Cache.WatchOptions; + private lastWatch?: Cache.WatchOptions; private updateWatch(variables = this.variables) { const oq = this.observableQuery; @@ -304,7 +304,7 @@ export class QueryInfo { return; } - const watchOptions: Cache.WatchOptions = { + const watchOptions: Cache.WatchOptions = { // Although this.getDiffOptions returns Cache.DiffOptions instead of // Cache.WatchOptions, all the overlapping options should be the same, so // we can reuse getDiffOptions here, for consistency. From cfb2d88bc2cb6511f5b67a48698e0b01cb129f28 Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Thu, 26 Aug 2021 19:39:42 -0400 Subject: [PATCH 5/6] Test that `cache.update{Query,Fragment}` are broadcast-batched. --- src/cache/inmemory/__tests__/cache.ts | 160 ++++++++++++++++++++++++++ 1 file changed, 160 insertions(+) diff --git a/src/cache/inmemory/__tests__/cache.ts b/src/cache/inmemory/__tests__/cache.ts index cae97479d98..ff40c38edff 100644 --- a/src/cache/inmemory/__tests__/cache.ts +++ b/src/cache/inmemory/__tests__/cache.ts @@ -1296,6 +1296,166 @@ describe('Cache', () => { ); }); + describe("cache.updateQuery and cache.updateFragment", () => { + it('should be batched', () => { + const cache = new InMemoryCache({ + typePolicies: { + Person: { + keyFields: ["name"], + }, + }, + }); + + type QueryData = { + me: { + __typename: string; + name: string; + }, + }; + + const query: TypedDocumentNode = gql`query { me { name } }`; + const results: QueryData[] = []; + + const cancel = cache.watch({ + query, + optimistic: true, + callback(diff) { + results.push(diff.result!); + }, + }); + + cache.updateQuery({ query }, data => { + expect(data).toBe(null); + + cache.writeQuery({ + query, + data: { + me: { + __typename: "Person", + name: "Ben", + }, + }, + }); + + return { + me: { + __typename: "Person", + name: "Ben Newman", + }, + }; + }); + + expect(results).toEqual([ + { me: { __typename: "Person", name: "Ben Newman" }}, + ]); + + expect(cache.extract()).toEqual({ + 'Person:{"name":"Ben Newman"}': { + __typename: "Person", + name: "Ben Newman", + }, + 'Person:{"name":"Ben"}': { + __typename: "Person", + name: "Ben", + }, + ROOT_QUERY: { + __typename: "Query", + me: { + __ref: 'Person:{"name":"Ben Newman"}', + }, + }, + }); + + const usernameFragment = gql` + fragment UsernameFragment on Person { + username + } + `; + + const bnId = cache.identify({ + __typename: "Person", + name: "Ben Newman", + }); + + cache.updateFragment({ + id: bnId, + fragment: usernameFragment, + returnPartialData: true, + }, data => { + expect(data).toEqual({ + __typename: "Person", + }); + + cache.writeQuery({ + query, + data: { + me: { + __typename: "Person", + name: "Brian Kim", + }, + }, + }); + + cache.writeFragment({ + id: cache.identify({ + __typename: "Person", + name: "Brian Kim", + }), + fragment: usernameFragment, + data: { + username: "brainkim", + }, + }); + + expect(results.length).toBe(1); + + return { + ...data, + name: "Ben Newman", + username: "benjamn", + }; + }); + + // Still just two results, thanks to cache.update{Query,Fragment} using + // cache.batch behind the scenes. + expect(results).toEqual([ + { me: { __typename: "Person", name: "Ben Newman" }}, + { me: { __typename: "Person", name: "Brian Kim" }}, + ]); + + expect(cache.extract()).toEqual({ + 'Person:{"name":"Ben"}': { + __typename: "Person", + name: "Ben", + }, + 'Person:{"name":"Ben Newman"}': { + __typename: "Person", + name: "Ben Newman", + username: "benjamn", + }, + 'Person:{"name":"Brian Kim"}': { + __typename: "Person", + name: "Brian Kim", + username: "brainkim", + }, + ROOT_QUERY: { + __typename: "Query", + me: { + __ref: 'Person:{"name":"Brian Kim"}', + }, + }, + __META: { + extraRootIds: [ + 'Person:{"name":"Ben Newman"}', + 'Person:{"name":"Brian Kim"}', + ], + }, + }); + + cancel(); + }); + }); + describe('cache.restore', () => { it('replaces cache.{store{Reader,Writer},maybeBroadcastWatch}', () => { const cache = new InMemoryCache; From 6bf12cb4f13da8c7241ab514adab3d597c3a43dc Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Fri, 17 Sep 2021 14:20:11 -0400 Subject: [PATCH 6/6] Use more descriptive type parameter names for cache.batch. https://github.com/apollographql/apollo-client/pull/8696#pullrequestreview-757733932 --- src/cache/core/types/Cache.ts | 9 ++++++--- src/cache/inmemory/inMemoryCache.ts | 8 +++++--- 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/src/cache/core/types/Cache.ts b/src/cache/core/types/Cache.ts index 4b3054aa705..11efe315af8 100644 --- a/src/cache/core/types/Cache.ts +++ b/src/cache/core/types/Cache.ts @@ -58,10 +58,13 @@ export namespace Cache { broadcast?: boolean; } - export interface BatchOptions, U = void> { + export interface BatchOptions< + TCache extends ApolloCache, + TUpdateResult = void, + > { // Same as the first parameter of performTransaction, except the cache // argument will have the subclass type rather than ApolloCache. - update(cache: C): U; + update(cache: TCache): TUpdateResult; // Passing a string for this option creates a new optimistic layer, with the // given string as its layer.id, just like passing a string for the @@ -84,7 +87,7 @@ export namespace Cache { // this batch operation, pass this optional callback function. Returning // false from the callback will prevent broadcasting this result. onWatchUpdated?: ( - this: C, + this: TCache, watch: Cache.WatchOptions, diff: Cache.DiffResult, lastDiff: Cache.DiffResult | undefined, diff --git a/src/cache/inmemory/inMemoryCache.ts b/src/cache/inmemory/inMemoryCache.ts index f36b125a369..b5f1fc6e862 100644 --- a/src/cache/inmemory/inMemoryCache.ts +++ b/src/cache/inmemory/inMemoryCache.ts @@ -388,7 +388,9 @@ export class InMemoryCache extends ApolloCache { private txCount = 0; - public batch(options: Cache.BatchOptions): U { + public batch( + options: Cache.BatchOptions, + ): TUpdateResult { const { update, optimistic = true, @@ -396,8 +398,8 @@ export class InMemoryCache extends ApolloCache { onWatchUpdated, } = options; - let updateResult: U; - const perform = (layer?: EntityStore): U => { + let updateResult: TUpdateResult; + const perform = (layer?: EntityStore): TUpdateResult => { const { data, optimisticData } = this; ++this.txCount; if (layer) {