From c0ccb1b0dfff280bc6fb59a9d78a7484288b429b Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Tue, 29 Sep 2020 16:03:12 -0400 Subject: [PATCH 1/5] Ensure InMemoryCache#read{,Query,Fragment} always return T|null. Closes https://github.com/apollographql/apollo-feature-requests/issues/1, by making cache.read no longer throw under any circumstances. This is a long-standing feature request that I partially addressed in #5930, but this commit goes all the way. Since the current behavior (sometimes returning T, sometimes null, and sometimes throwing) has been in place for so long, we do not make this change lightly, and we should state precisely what is changing: in every case where cache.read would previously throw an exception, it will now return null instead. Since these null results have the effect of hiding which fields were missing, you may wish to switch from cache.readQuery to cache.diff to gain more insight into why cache.read returned null. In fact, cache.diff (along with cache.watch) are the primary ApolloCache methods that Apollo Client uses internally, because the cache.diff API provides so much more useful information than cache.read provides. If you would prefer for cache.read to return partial data rather than null, you can now pass returnPartialData:true to cache.readQuery and cache.readFragment, though the default must remain false instead of true, for the reasons explained in the code comments. In the positive column, null should be easier to handle in update functions that use the readQuery/writeQuery pattern for updating the cache. Not only can you avoid wrapping cache.readQuery with a try-catch block, but you can safely spread ...null into an object literal, which is often something that happens between readQuery and writeQuery. If you were relying on the exception-throwing behavior, and you have application code that is not prepared to handle null, please leave a comment describing your use case. We will let these changes bake in AC3.3 betas until we are confident they have been adequately tested. --- src/cache/core/types/Cache.ts | 5 +- src/cache/inmemory/__tests__/entityStore.ts | 24 ++++++++-- src/cache/inmemory/__tests__/policies.ts | 51 +++++++++++++++------ src/cache/inmemory/inMemoryCache.ts | 34 ++++++++++---- 4 files changed, 85 insertions(+), 29 deletions(-) diff --git a/src/cache/core/types/Cache.ts b/src/cache/core/types/Cache.ts index 8a9af7c3ff1..8d2412629ad 100644 --- a/src/cache/core/types/Cache.ts +++ b/src/cache/core/types/Cache.ts @@ -9,6 +9,7 @@ export namespace Cache { rootId?: string; previousResult?: any; optimistic: boolean; + returnPartialData?: boolean; } export interface WriteOptions @@ -19,7 +20,9 @@ export namespace Cache { } export interface DiffOptions extends ReadOptions { - returnPartialData?: boolean; + // The DiffOptions interface is currently just an alias for + // ReadOptions, though DiffOptions used to be responsible for + // declaring the returnPartialData option. } export interface WatchOptions extends ReadOptions { diff --git a/src/cache/inmemory/__tests__/entityStore.ts b/src/cache/inmemory/__tests__/entityStore.ts index 052c05218ca..d1815c5b08b 100644 --- a/src/cache/inmemory/__tests__/entityStore.ts +++ b/src/cache/inmemory/__tests__/entityStore.ts @@ -1588,13 +1588,29 @@ describe('EntityStore', () => { }, }); - expect(() => cache.readQuery({ + function diff(query: DocumentNode) { + return cache.diff({ + query, + optimistic: true, + returnPartialData: false, + }); + } + + expect(cache.readQuery({ query: queryWithAliases, - })).toThrow(/Dangling reference to missing ABCs:.* object/); + })).toBe(null); - expect(() => cache.readQuery({ + expect(() => diff(queryWithAliases)).toThrow( + /Dangling reference to missing ABCs:.* object/, + ); + + expect(cache.readQuery({ query: queryWithoutAliases, - })).toThrow(/Dangling reference to missing ABCs:.* object/); + })).toBe(null); + + expect(() => diff(queryWithoutAliases)).toThrow( + /Dangling reference to missing ABCs:.* object/, + ); }); it("gracefully handles eviction amid optimistic updates", () => { diff --git a/src/cache/inmemory/__tests__/policies.ts b/src/cache/inmemory/__tests__/policies.ts index 4d00064d3ec..6db285d11e6 100644 --- a/src/cache/inmemory/__tests__/policies.ts +++ b/src/cache/inmemory/__tests__/policies.ts @@ -2193,17 +2193,27 @@ describe("type policies", function () { expect(secretReadAttempted).toBe(false); - expect(() => { - cache.readQuery({ - query: gql` - query { - me { - secret - } + expect(cache.readQuery({ + query: gql` + query { + me { + secret } - ` - }); - }).toThrowError("Can't find field 'secret' "); + } + `, + })).toBe(null); + + expect(() => cache.diff({ + optimistic: true, + returnPartialData: false, + query: gql` + query { + me { + secret + } + } + `, + })).toThrowError("Can't find field 'secret' "); expect(secretReadAttempted).toBe(true); }); @@ -3517,6 +3527,15 @@ describe("type policies", function () { }); } + function diff(isbn = "156858217X") { + return cache.diff({ + query, + variables: { isbn }, + returnPartialData: false, + optimistic: true, + }); + } + expect(read()).toBe(null); cache.writeQuery({ @@ -3532,8 +3551,10 @@ describe("type policies", function () { }, }); - expect(read).toThrow( - /Dangling reference to missing Book:{"isbn":"156858217X"} object/ + expect(read()).toBe(null); + + expect(diff).toThrow( + /Dangling reference to missing Book:{"isbn":"156858217X"} object/, ); const stealThisData = { @@ -3664,11 +3685,13 @@ describe("type policies", function () { }, }); - expect(() => read("0393354326")).toThrow( + expect(read("0393354326")).toBe(null); + expect(() => diff("0393354326")).toThrow( /Dangling reference to missing Book:{"isbn":"0393354326"} object/ ); - expect(() => read("156858217X")).toThrow( + expect(read("156858217X")).toBe(null); + expect(() => diff("156858217X")).toThrow( /Dangling reference to missing Book:{"isbn":"156858217X"} object/ ); }); diff --git a/src/cache/inmemory/inMemoryCache.ts b/src/cache/inmemory/inMemoryCache.ts index aa988941e1d..d64473dec7e 100644 --- a/src/cache/inmemory/inMemoryCache.ts +++ b/src/cache/inmemory/inMemoryCache.ts @@ -106,18 +106,32 @@ export class InMemoryCache extends ApolloCache { } public read(options: Cache.ReadOptions): T | null { - const store = options.optimistic ? this.optimisticData : this.data; - if (typeof options.rootId === 'string' && !store.has(options.rootId)) { + const { + // Since read returns data or null, without any additional metadata + // about whether/where there might have been missing fields, the + // default behavior cannot be returnPartialData = true (like it is + // for the diff method), since defaulting to true would violate the + // integrity of the T in the return type. However, partial data may + // be useful in some cases, so returnPartialData:true may be + // specified explicitly. + returnPartialData = false, + } = options; + try { + return this.storeReader.diffQueryAgainstStore({ + store: options.optimistic ? this.optimisticData : this.data, + query: options.query, + variables: options.variables, + rootId: options.rootId, + config: this.config, + returnPartialData, + }).result || null; + } catch { + // Intentionally swallow exceptions and return null, so that callers + // never need to worry about catching exceptions. If you need more + // information about what was incomplete, use cache.diff instead, + // and examine diffResult.missing. return null; } - return this.storeReader.diffQueryAgainstStore({ - store, - query: options.query, - variables: options.variables, - rootId: options.rootId, - config: this.config, - returnPartialData: false, - }).result || null; } public write(options: Cache.WriteOptions): Reference | undefined { From 5b4fc75d3631cc27c7f2b6598d98bfb3d85546b2 Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Wed, 30 Sep 2020 11:59:00 -0400 Subject: [PATCH 2/5] Swallow only MissingFieldError exceptions in cache.read. --- src/cache/inmemory/inMemoryCache.ts | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/src/cache/inmemory/inMemoryCache.ts b/src/cache/inmemory/inMemoryCache.ts index d64473dec7e..38c33e28812 100644 --- a/src/cache/inmemory/inMemoryCache.ts +++ b/src/cache/inmemory/inMemoryCache.ts @@ -6,6 +6,7 @@ import { dep, wrap } from 'optimism'; import { ApolloCache } from '../core/cache'; import { Cache } from '../core/types/Cache'; +import { MissingFieldError } from '../core/types/common'; import { addTypenameToDocument, StoreObject, @@ -125,12 +126,16 @@ export class InMemoryCache extends ApolloCache { config: this.config, returnPartialData, }).result || null; - } catch { - // Intentionally swallow exceptions and return null, so that callers - // never need to worry about catching exceptions. If you need more - // information about what was incomplete, use cache.diff instead, - // and examine diffResult.missing. - return null; + } catch (e) { + if (e instanceof MissingFieldError) { + // Swallow MissingFieldError and return null, so callers do not + // need to worry about catching "normal" exceptions resulting from + // incomplete cache data. Unexpected errors will be re-thrown. If + // you need more information about which fields were missing, use + // cache.diff instead, and examine diffResult.missing. + return null; + } + throw e; } } From c97fbd36aa968cf5022abb9972e1c0e8339e2406 Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Wed, 30 Sep 2020 14:14:46 -0400 Subject: [PATCH 3/5] Update docs about readQuery and readFragment null/exception behavior. --- docs/source/caching/cache-interaction.md | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/docs/source/caching/cache-interaction.md b/docs/source/caching/cache-interaction.md index 22b1151282d..46e6b46bd56 100644 --- a/docs/source/caching/cache-interaction.md +++ b/docs/source/caching/cache-interaction.md @@ -19,9 +19,11 @@ All code samples below assume that you have initialized an instance of `ApolloC The `readQuery` method enables you to run a GraphQL query directly on your cache. -* If your cache contains all of the data necessary to fulfill a specified query, `readQuery` returns a data object in the shape of that query, just like a GraphQL server does. +* If your cache contains all of the data necessary to fulfill the specified query, `readQuery` returns a data object in the shape of that query, just like a GraphQL server does. -* If your cache _doesn't_ contain all of the data necessary to fulfill a specified query, `readQuery` throws an error. It _never_ attempts to fetch data from a remote server. +* If your cache does not contain all of the data necessary to fulfill the specified query, `readQuery` returns `null`, without attempting to fetch data from a remote server. + +> Prior to Apollo Client 3.3, `readQuery` would throw `MissingFieldError` exceptions to report missing fields. Beginning with Apollo Client 3.3, `readQuery` always returns `null` to indicate fields were missing. Pass `readQuery` a GraphQL query string like so: @@ -81,12 +83,9 @@ const todo = client.readFragment({ The first argument, `id`, is the value of the unique identifier for the object you want to read from the cache. By default, this is the value of the object's `id` field, but you can [customize this behavior](./cache-configuration/#generating-unique-identifiers). -In the example above: +In the example above, `readFragment` returns `null` if no `Todo` object with an `id` of `5` exists in the cache, or if the object exists but is missing the `text` or `completed` fields. -* If a `Todo` object with an `id` of `5` is _not_ in the cache, -`readFragment` returns `null`. -* If the `Todo` object _is_ in the cache but it's -missing either a `text` or `completed` field, `readFragment` throws an error. +> Prior to Apollo Client 3.3, `readFragment` would throw `MissingFieldError` exceptions to report missing fields, and return `null` only when reading a fragment from a nonexistent ID. Beginning with Apollo Client 3.3, `readFragment` always returns `null` to indicate insufficient data (missing ID or missing fields), instead of throwing a `MissingFieldError`. ## `writeQuery` and `writeFragment` From 5c96bf0297c2fd70ea5e97882953ab80a51c7a50 Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Wed, 30 Sep 2020 15:05:05 -0400 Subject: [PATCH 4/5] Fix returnPartialData and improve types for cache.read{Query,Fragment}. --- src/cache/core/cache.ts | 10 ++- src/cache/core/types/Cache.ts | 2 + src/cache/core/types/DataProxy.ts | 36 ++++++++- src/cache/inmemory/__tests__/readFromStore.ts | 79 +++++++++++++++++++ 4 files changed, 120 insertions(+), 7 deletions(-) diff --git a/src/cache/core/cache.ts b/src/cache/core/cache.ts index 19eb8dd6127..9cc45b7c3e1 100644 --- a/src/cache/core/cache.ts +++ b/src/cache/core/cache.ts @@ -104,13 +104,14 @@ export abstract class ApolloCache implements DataProxy { * @param optimistic */ public readQuery( - options: DataProxy.Query, - optimistic: boolean = false, + options: Cache.ReadQueryOptions, + optimistic = !!options.optimistic, ): QueryType | null { return this.read({ rootId: options.id || 'ROOT_QUERY', query: options.query, variables: options.variables, + returnPartialData: options.returnPartialData, optimistic, }); } @@ -120,13 +121,14 @@ export abstract class ApolloCache implements DataProxy { private getFragmentDoc = wrap(getFragmentQueryDocument); public readFragment( - options: DataProxy.Fragment, - optimistic: boolean = false, + options: Cache.ReadFragmentOptions, + optimistic = !!options.optimistic, ): FragmentType | null { return this.read({ query: this.getFragmentDoc(options.fragment, options.fragmentName), variables: options.variables, rootId: options.id, + returnPartialData: options.returnPartialData, optimistic, }); } diff --git a/src/cache/core/types/Cache.ts b/src/cache/core/types/Cache.ts index 8d2412629ad..281e0a1599a 100644 --- a/src/cache/core/types/Cache.ts +++ b/src/cache/core/types/Cache.ts @@ -45,6 +45,8 @@ export namespace Cache { } export import DiffResult = DataProxy.DiffResult; + export import ReadQueryOptions = DataProxy.ReadQueryOptions; + export import ReadFragmentOptions = DataProxy.ReadFragmentOptions; export import WriteQueryOptions = DataProxy.WriteQueryOptions; export import WriteFragmentOptions = DataProxy.WriteFragmentOptions; export import Fragment = DataProxy.Fragment; diff --git a/src/cache/core/types/DataProxy.ts b/src/cache/core/types/DataProxy.ts index a256829d09f..0d6f0a774b5 100644 --- a/src/cache/core/types/DataProxy.ts +++ b/src/cache/core/types/DataProxy.ts @@ -20,7 +20,7 @@ export namespace DataProxy { /** * The root id to be used. Defaults to "ROOT_QUERY", which is the ID of the * root query object. This property makes writeQuery capable of writing data - * to any object in the cache, which renders writeFragment mostly useless. + * to any object in the cache. */ id?: string; } @@ -54,6 +54,36 @@ export namespace DataProxy { variables?: TVariables; } + export interface ReadQueryOptions + extends Query { + /** + * Whether to return incomplete data rather than null. + * Defaults to false. + */ + returnPartialData?: boolean; + /** + * Whether to read from optimistic or non-optimistic cache data. If + * this named option is provided, the optimistic parameter of the + * readQuery method can be omitted. Defaults to false. + */ + optimistic?: boolean; + } + + export interface ReadFragmentOptions + extends Fragment { + /** + * Whether to return incomplete data rather than null. + * Defaults to false. + */ + returnPartialData?: boolean; + /** + * Whether to read from optimistic or non-optimistic cache data. If + * this named option is provided, the optimistic parameter of the + * readQuery method can be omitted. Defaults to false. + */ + optimistic?: boolean; + } + export interface WriteQueryOptions extends Query { /** @@ -97,7 +127,7 @@ export interface DataProxy { * Reads a GraphQL query from the root query id. */ readQuery( - options: DataProxy.Query, + options: DataProxy.ReadQueryOptions, optimistic?: boolean, ): QueryType | null; @@ -107,7 +137,7 @@ export interface DataProxy { * provided to select the correct fragment. */ readFragment( - options: DataProxy.Fragment, + options: DataProxy.ReadFragmentOptions, optimistic?: boolean, ): FragmentType | null; diff --git a/src/cache/inmemory/__tests__/readFromStore.ts b/src/cache/inmemory/__tests__/readFromStore.ts index fc1f02a9f8b..e44a44ee675 100644 --- a/src/cache/inmemory/__tests__/readFromStore.ts +++ b/src/cache/inmemory/__tests__/readFromStore.ts @@ -557,6 +557,85 @@ describe('reading from the store', () => { }).toThrowError(/Can't find field 'missingField' on ROOT_QUERY object/); }); + it('readQuery supports returnPartialData', () => { + const cache = new InMemoryCache; + const aQuery = gql`query { a }`; + const bQuery = gql`query { b }`; + const abQuery = gql`query { a b }`; + + cache.writeQuery({ + query: aQuery, + data: { a: 123 }, + }); + + expect(cache.readQuery({ query: bQuery })).toBe(null); + expect(cache.readQuery({ query: abQuery })).toBe(null); + + expect(cache.readQuery({ + query: bQuery, + returnPartialData: true, + })).toEqual({}); + + expect(cache.readQuery({ + query: abQuery, + returnPartialData: true, + })).toEqual({ a: 123 }); + }); + + it('readFragment supports returnPartialData', () => { + const cache = new InMemoryCache; + const id = cache.identify({ + __typename: "ABObject", + id: 321, + }); + + const aFragment = gql`fragment AFragment on ABObject { a }`; + const bFragment = gql`fragment BFragment on ABObject { b }`; + const abFragment = gql`fragment ABFragment on ABObject { a b }`; + + expect(cache.readFragment({ id, fragment: aFragment })).toBe(null); + expect(cache.readFragment({ id, fragment: bFragment })).toBe(null); + expect(cache.readFragment({ id, fragment: abFragment })).toBe(null); + + const ref = cache.writeFragment({ + id, + fragment: aFragment, + data: { + __typename: "ABObject", + a: 123, + }, + }); + expect(isReference(ref)).toBe(true); + expect(ref!.__ref).toBe(id); + + expect(cache.readFragment({ + id, + fragment: bFragment, + })).toBe(null); + + expect(cache.readFragment({ + id, + fragment: abFragment, + })).toBe(null); + + expect(cache.readFragment({ + id, + fragment: bFragment, + returnPartialData: true, + })).toEqual({ + __typename: "ABObject", + }); + + expect(cache.readFragment({ + id, + fragment: abFragment, + returnPartialData: true, + })).toEqual({ + __typename: "ABObject", + a: 123, + }); + }); + it('distinguishes between missing @client and non-@client fields', () => { const query = gql` query { From 1d0f96fdc70bd2352f0e5d55fb945f280d05909e Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Wed, 30 Sep 2020 16:48:01 -0400 Subject: [PATCH 5/5] Mention PR #7098 in CHANGELOG.md. --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 316ba17741f..0f93f9b02a4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,9 @@ - In addition to the `result.data` property, `useQuery` and `useLazyQuery` will now provide a `result.previousData` property, which can be useful when a network request is pending and `result.data` is undefined, since `result.previousData` can be rendered instead of rendering an empty/loading state.
[@hwillson](https://github.com/hwillson) in [#7082](https://github.com/apollographql/apollo-client/pull/7082) +- Ensure `cache.readQuery` and `cache.readFragment` always return `TData | null`, instead of throwing an exception when missing fields are encountered.
+ [@benjamn](https://github.com/benjamn) in [#7098](https://github.com/apollographql/apollo-client/pull/7098) + ## Apollo Client 3.2.1 ## Bug Fixes