From 09ac78aadb928727346b73d5a5bd196153fb9764 Mon Sep 17 00:00:00 2001 From: Brian Kim Date: Tue, 18 May 2021 21:08:51 -0400 Subject: [PATCH 1/3] pass along missing field errors to the user --- src/cache/core/types/common.ts | 9 ++- src/core/ObservableQuery.ts | 11 +++ src/errors/index.ts | 14 ++-- src/react/hooks/__tests__/useQuery.test.tsx | 77 +++++++++++++++++++++ 4 files changed, 105 insertions(+), 6 deletions(-) diff --git a/src/cache/core/types/common.ts b/src/cache/core/types/common.ts index 87fb2c066a5..fdad9b47547 100644 --- a/src/cache/core/types/common.ts +++ b/src/cache/core/types/common.ts @@ -18,14 +18,19 @@ import { StorageType } from '../../inmemory/policies'; // Readonly, somewhat surprisingly. export type SafeReadonly = T extends object ? Readonly : T; -export class MissingFieldError { +export class MissingFieldError extends Error { constructor( public readonly message: string, public readonly path: (string | number)[], public readonly query: import('graphql').DocumentNode, public readonly clientOnly: boolean, public readonly variables?: Record, - ) {} + ) { + super(message); + // We're not using `Object.setPrototypeOf` here as it isn't fully + // supported on Android (see issue #3236). + (this as any).__proto__ = MissingFieldError.prototype; + } } export interface FieldSpecifier { diff --git a/src/core/ObservableQuery.ts b/src/core/ObservableQuery.ts index 76b31b12d51..998cbb9577a 100644 --- a/src/core/ObservableQuery.ts +++ b/src/core/ObservableQuery.ts @@ -162,6 +162,7 @@ export class ObservableQuery< !this.queryManager.transform(this.options.query).hasForcedResolvers ) { const diff = this.queryInfo.getDiff(); + // XXX the only reason this typechecks is that diff.result is inferred as any result.data = ( diff.complete || this.options.returnPartialData @@ -180,6 +181,16 @@ export class ObservableQuery< } else { result.partial = true; } + + if ( + !diff.complete && + !this.options.partialRefetch && + !result.loading && + !result.data && + !result.error + ) { + result.error = new ApolloError({ clientErrors: diff.missing }); + } } if (saveAsLastResult) { diff --git a/src/errors/index.ts b/src/errors/index.ts index 99b279d8588..956fa3df4e5 100644 --- a/src/errors/index.ts +++ b/src/errors/index.ts @@ -15,10 +15,12 @@ export function isApolloError(err: Error): err is ApolloError { const generateErrorMessage = (err: ApolloError) => { let message = ''; // If we have GraphQL errors present, add that to the error message. - if (isNonEmptyArray(err.graphQLErrors)) { - err.graphQLErrors.forEach((graphQLError: GraphQLError) => { - const errorMessage = graphQLError - ? graphQLError.message + if (isNonEmptyArray(err.graphQLErrors) || isNonEmptyArray(err.clientErrors)) { + const errors = ((err.graphQLErrors || []) as readonly Error[]) + .concat(err.clientErrors || []); + errors.forEach((error: Error) => { + const errorMessage = error + ? error.message : 'Error message not found.'; message += `${errorMessage}\n`; }); @@ -36,6 +38,7 @@ const generateErrorMessage = (err: ApolloError) => { export class ApolloError extends Error { public message: string; public graphQLErrors: ReadonlyArray; + public clientErrors: ReadonlyArray; public networkError: Error | ServerParseError | ServerError | null; // An object that can be used to provide some additional information @@ -48,17 +51,20 @@ export class ApolloError extends Error { // value or the constructed error will be meaningless. constructor({ graphQLErrors, + clientErrors, networkError, errorMessage, extraInfo, }: { graphQLErrors?: ReadonlyArray; + clientErrors?: ReadonlyArray; networkError?: Error | ServerParseError | ServerError | null; errorMessage?: string; extraInfo?: any; }) { super(errorMessage); this.graphQLErrors = graphQLErrors || []; + this.clientErrors = clientErrors || []; this.networkError = networkError || null; this.message = errorMessage || generateErrorMessage(this); this.extraInfo = extraInfo; diff --git a/src/react/hooks/__tests__/useQuery.test.tsx b/src/react/hooks/__tests__/useQuery.test.tsx index 5969458ac7e..126c93f6bd1 100644 --- a/src/react/hooks/__tests__/useQuery.test.tsx +++ b/src/react/hooks/__tests__/useQuery.test.tsx @@ -2645,6 +2645,83 @@ describe('useQuery Hook', () => { }); }); + describe('Missing Fields', () => { + itAsync( + 'should have errors populated with missing field errors from the cache', + (resolve, reject) => { + const carQuery: DocumentNode = gql` + query cars($id: Int) { + cars(id: $id) { + id + make + model + vin + __typename + } + } + `; + + const carData = { + cars: [ + { + id: 1, + make: 'Audi', + model: 'RS8', + vine: 'DOLLADOLLABILL', + __typename: 'Car' + } + ] + }; + + const mocks = [ + { + request: { query: carQuery, variables: { id: 1 } }, + result: { data: carData } + }, + ]; + + let renderCount = 0; + function App() { + const { loading, data, error } = useQuery(carQuery, { + variables: { id: 1 }, + }); + + switch (renderCount) { + case 0: + expect(loading).toBeTruthy(); + expect(data).toBeUndefined(); + expect(error).toBeUndefined(); + break; + case 1: + expect(loading).toBeFalsy(); + expect(data).toBeUndefined(); + expect(error).toBeDefined(); + // TODO: ApolloError.name is Error for some reason + // expect(error!.name).toBe(ApolloError); + expect(error!.clientErrors.length).toEqual(1); + expect(error!.message).toMatch(/Can't find field 'vin' on Car:1/); + break; + default: + throw new Error("Unexpected render"); + } + + renderCount += 1; + return null; + } + + render( + + + + ); + + return wait(() => { + expect(renderCount).toBe(2); + }).then(resolve, reject); + }, + ); + }); + describe('Previous data', () => { itAsync('should persist previous data when a query is re-run', (resolve, reject) => { const query = gql` From 2de299a5a8cd74643959d69a312f7ddb29b4c6d2 Mon Sep 17 00:00:00 2001 From: Brian Kim Date: Wed, 19 May 2021 12:43:35 -0400 Subject: [PATCH 2/3] update CHANGELOG and package.json --- CHANGELOG.md | 2 ++ package.json | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index cb8d9d49fc8..6bbdd9067fa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -67,6 +67,8 @@ - Fully remove result cache entries from LRU dependency system when the corresponding entities are removed from `InMemoryCache` by eviction, or by any other means.
[@sofianhn](https://github.com/sofianhn) and [@benjamn](https://github.com/benjamn) in [#8147](https://github.com/apollographql/apollo-client/pull/8147) +- Expose missing field errors in results.
[@brainkim](github.com/brainkim) in [#8262](https://github.com/apollographql/apollo-client/pull/8262) + ### Documentation TBD diff --git a/package.json b/package.json index 22927e59e6d..0a2ecc44ff8 100644 --- a/package.json +++ b/package.json @@ -55,7 +55,7 @@ { "name": "apollo-client", "path": "./dist/apollo-client.cjs.min.js", - "maxSize": "26.6 kB" + "maxSize": "26.62 kB" } ], "peerDependencies": { From 9ec7f1720a2a6b446c206061d72b4cbacac694e9 Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Fri, 23 Apr 2021 16:51:05 -0400 Subject: [PATCH 3/3] Remove context.clientOnly tracking from executeSelectionSetImpl. https://github.com/apollographql/apollo-client/pull/8262#discussion_r636328571 --- src/cache/core/types/common.ts | 1 - src/cache/inmemory/__tests__/entityStore.ts | 4 ---- src/cache/inmemory/__tests__/policies.ts | 1 - src/cache/inmemory/__tests__/readFromStore.ts | 2 -- src/cache/inmemory/readFromStore.ts | 19 ------------------- 5 files changed, 27 deletions(-) diff --git a/src/cache/core/types/common.ts b/src/cache/core/types/common.ts index fdad9b47547..5c7ac7db401 100644 --- a/src/cache/core/types/common.ts +++ b/src/cache/core/types/common.ts @@ -23,7 +23,6 @@ export class MissingFieldError extends Error { public readonly message: string, public readonly path: (string | number)[], public readonly query: import('graphql').DocumentNode, - public readonly clientOnly: boolean, public readonly variables?: Record, ) { super(message); diff --git a/src/cache/inmemory/__tests__/entityStore.ts b/src/cache/inmemory/__tests__/entityStore.ts index 5f300956dfc..396b683aede 100644 --- a/src/cache/inmemory/__tests__/entityStore.ts +++ b/src/cache/inmemory/__tests__/entityStore.ts @@ -1213,14 +1213,12 @@ describe('EntityStore', () => { 'Can\'t find field \'hobby\' on Author:{"name":"Ted Chiang"} object', ["authorOfBook", "hobby"], expect.anything(), // query - false, // clientOnly expect.anything(), // variables ), new MissingFieldError( 'Can\'t find field \'publisherOfBook\' on ROOT_QUERY object', ["publisherOfBook"], expect.anything(), // query - false, // clientOnly expect.anything(), // variables ), ], @@ -1814,7 +1812,6 @@ describe('EntityStore', () => { "Dangling reference to missing Author:2 object", ["book", "author"], expect.anything(), // query - false, // clientOnly expect.anything(), // variables ), ]; @@ -2084,7 +2081,6 @@ describe('EntityStore', () => { 'Can\'t find field \'title\' on Book:{"isbn":"031648637X"} object', ["book", "title"], expect.anything(), // query - false, // clientOnly expect.anything(), // variables ), ], diff --git a/src/cache/inmemory/__tests__/policies.ts b/src/cache/inmemory/__tests__/policies.ts index 0c7267553a2..0721b76f896 100644 --- a/src/cache/inmemory/__tests__/policies.ts +++ b/src/cache/inmemory/__tests__/policies.ts @@ -1352,7 +1352,6 @@ describe("type policies", function () { `Can't find field 'result' on Job:{"name":"Job #${jobNumber}"} object`, ["jobs", jobNumber - 1, "result"], expect.anything(), // query - false, // clientOnly expect.anything(), // variables ); } diff --git a/src/cache/inmemory/__tests__/readFromStore.ts b/src/cache/inmemory/__tests__/readFromStore.ts index 54d9e691741..c3c1deff2a6 100644 --- a/src/cache/inmemory/__tests__/readFromStore.ts +++ b/src/cache/inmemory/__tests__/readFromStore.ts @@ -729,7 +729,6 @@ describe('reading from the store', () => { }`, ["normal", "missing"], query, - false, // clientOnly {}, // variables ), new MissingFieldError( @@ -738,7 +737,6 @@ describe('reading from the store', () => { }`, ["clientOnly", "missing"], query, - true, // clientOnly {}, // variables ), ]); diff --git a/src/cache/inmemory/readFromStore.ts b/src/cache/inmemory/readFromStore.ts index 803855c7526..36caf7ea1d4 100644 --- a/src/cache/inmemory/readFromStore.ts +++ b/src/cache/inmemory/readFromStore.ts @@ -46,7 +46,6 @@ interface ReadContext extends ReadMergeModifyContext { canonizeResults: boolean; fragmentMap: FragmentMap; path: (string | number)[]; - clientOnly: boolean; }; export type ExecResult = { @@ -62,7 +61,6 @@ function missingFromInvariant( err.message, context.path.slice(), context.query, - context.clientOnly, context.variables, ); } @@ -241,7 +239,6 @@ export class StoreReader { canonizeResults, fragmentMap: createFragmentMap(getFragmentDefinitions(query)), path: [], - clientOnly: false, }, }); @@ -344,20 +341,6 @@ export class StoreReader { const resultName = resultKeyNameFromField(selection); context.path.push(resultName); - // If this field has an @client directive, then the field and - // everything beneath it is client-only, meaning it will never be - // sent to the server. - const wasClientOnly = context.clientOnly; - // Once we enter a client-only subtree of the query, we can avoid - // repeatedly checking selection.directives. - context.clientOnly = wasClientOnly || !!( - // We don't use the hasDirectives helper here, because it looks - // for directives anywhere inside the AST node, whereas we only - // care about directives directly attached to this field. - selection.directives && - selection.directives.some(d => d.name.value === "client") - ); - if (fieldValue === void 0) { if (!addTypenameToDocument.added(selection)) { getMissing().push( @@ -407,8 +390,6 @@ export class StoreReader { objectsToMerge.push({ [resultName]: fieldValue }); } - context.clientOnly = wasClientOnly; - invariant(context.path.pop() === resultName); } else {