From 27c85d0151db8c14f142de6d8c36da9f4c7e2cd2 Mon Sep 17 00:00:00 2001 From: MrDoomBringer Date: Mon, 25 Jul 2022 15:10:55 -0400 Subject: [PATCH 1/4] Squash error handling Mutations basic run ApolloLink Revert "Mutations basic run" This reverts commit b3a130d63cf0b782a82b066d75e8d41100c3f986. Test support Rename error_network -> errorNetwork onError link support bump bundlesize from 29.95 -> 29.98KB Consolidate errorNetwork away --- package.json | 2 +- src/cache/core/types/Cache.ts | 4 ++ src/core/QueryManager.ts | 2 + src/link/core/types.ts | 3 +- src/link/error/index.ts | 8 ++- src/link/http/__tests__/HttpLink.ts | 54 +++++++------------ .../__tests__/parseAndCheckHttpResponse.ts | 27 +++++++--- src/link/http/parseAndCheckHttpResponse.ts | 36 +++++++------ src/link/utils/throwServerError.ts | 2 +- src/react/hooks/useQuery.ts | 5 +- 10 files changed, 76 insertions(+), 67 deletions(-) diff --git a/package.json b/package.json index 7e0df8c8f7a..4671c2f2f59 100644 --- a/package.json +++ b/package.json @@ -56,7 +56,7 @@ { "name": "apollo-client", "path": "./dist/apollo-client.min.cjs", - "maxSize": "29.95kB" + "maxSize": "29.98kB" } ], "engines": { diff --git a/src/cache/core/types/Cache.ts b/src/cache/core/types/Cache.ts index 4086d34a45e..362e9c79ed8 100644 --- a/src/cache/core/types/Cache.ts +++ b/src/cache/core/types/Cache.ts @@ -1,6 +1,8 @@ import { DataProxy } from './DataProxy'; import { Modifier, Modifiers } from './common'; import { ApolloCache } from '../cache'; +import { ServerError } from '../../../core'; +import { GraphQLError } from 'graphql'; export namespace Cache { export type WatchCallback = ( @@ -23,6 +25,8 @@ export namespace Cache { { dataId?: string; result: TResult; + error?: ServerError; + errors?: ReadonlyArray; } export interface DiffOptions< diff --git a/src/core/QueryManager.ts b/src/core/QueryManager.ts index ee4c31edc1b..6e3e4ddd7b5 100644 --- a/src/core/QueryManager.ts +++ b/src/core/QueryManager.ts @@ -1056,6 +1056,7 @@ export class QueryManager { // Throwing here effectively calls observer.error. throw queryInfo.markError(new ApolloError({ graphQLErrors: result.errors, + networkError: result.error, })); } queryInfo.markResult(result, options, cacheWriteBehavior); @@ -1070,6 +1071,7 @@ export class QueryManager { if (hasErrors && options.errorPolicy !== "ignore") { aqr.errors = result.errors; + aqr.error = result.error; aqr.networkStatus = NetworkStatus.error; } diff --git a/src/link/core/types.ts b/src/link/core/types.ts index 847cf626816..801767332bb 100644 --- a/src/link/core/types.ts +++ b/src/link/core/types.ts @@ -2,7 +2,7 @@ import { DocumentNode, ExecutionResult } from 'graphql'; export { DocumentNode }; import { Observable } from '../../utilities'; - +import { ApolloError } from '../../errors'; export interface GraphQLRequest { query: DocumentNode; variables?: Record; @@ -25,6 +25,7 @@ export interface FetchResult< TContext = Record, TExtensions = Record > extends ExecutionResult { + error?: ApolloError; data?: TData | null | undefined; extensions?: TExtensions; context?: TContext; diff --git a/src/link/error/index.ts b/src/link/error/index.ts index 110383880b7..84f5b338e8a 100644 --- a/src/link/error/index.ts +++ b/src/link/error/index.ts @@ -34,9 +34,10 @@ export function onError(errorHandler: ErrorHandler): ApolloLink { try { sub = forward(operation).subscribe({ next: result => { - if (result.errors) { + if (result.errors || result.error) { retriedResult = errorHandler({ graphQLErrors: result.errors, + networkError: result.error, response: result, operation, forward, @@ -58,10 +59,7 @@ export function onError(errorHandler: ErrorHandler): ApolloLink { operation, networkError, //Network errors can return GraphQL errors on for example a 403 - graphQLErrors: - networkError && - networkError.result && - networkError.result.errors, + graphQLErrors: networkError?.result?.errors, forward, }); if (retriedResult) { diff --git a/src/link/http/__tests__/HttpLink.ts b/src/link/http/__tests__/HttpLink.ts index 7114770ae32..1cfc0381aa3 100644 --- a/src/link/http/__tests__/HttpLink.ts +++ b/src/link/http/__tests__/HttpLink.ts @@ -1026,15 +1026,18 @@ describe('HttpLink', () => { describe('Error handling', () => { let responseBody: any; + const networkError = new Error(`Response not successful: Received status code 400.`) as ServerError; const text = jest.fn(() => { const responseBodyText = '{}'; responseBody = JSON.parse(responseBodyText); + responseBody.errorNetwork = networkError return Promise.resolve(responseBodyText); }); const textWithData = jest.fn(() => { responseBody = { data: { stub: { id: 1 } }, errors: [{ message: 'dangit' }], + errorNetwork: networkError, }; return Promise.resolve(JSON.stringify(responseBody)); @@ -1043,6 +1046,7 @@ describe('HttpLink', () => { const textWithErrors = jest.fn(() => { responseBody = { errors: [{ message: 'dangit' }], + errorNetwork: networkError, }; return Promise.resolve(JSON.stringify(responseBody)); @@ -1053,18 +1057,13 @@ describe('HttpLink', () => { beforeEach(() => { fetch.mockReset(); }); - itAsync('makes it easy to do stuff on a 401', (resolve, reject) => { + itAsync('observables continue after a 401 error', (resolve, reject) => { const middleware = new ApolloLink((operation, forward) => { return new Observable(ob => { fetch.mockReturnValueOnce(Promise.resolve({ status: 401, text })); const op = forward(operation); const sub = op.subscribe({ next: ob.next.bind(ob), - error: makeCallback(resolve, reject, (e: ServerError) => { - expect(e.message).toMatch(/Received status code 401/); - expect(e.statusCode).toEqual(401); - ob.error(e); - }), complete: ob.complete.bind(ob), }); @@ -1078,7 +1077,9 @@ describe('HttpLink', () => { execute(link, { query: sampleQuery }).subscribe( result => { - reject('next should have been thrown from the network'); + expect(result.errorNetwork?.statusCode).toEqual(401); + expect(result.errorNetwork?.message).toMatch(/Received status code 401/); + resolve(); }, () => {}, ); @@ -1090,13 +1091,11 @@ describe('HttpLink', () => { execute(link, { query: sampleQuery }).subscribe( result => { - reject('next should have been thrown from the network'); + expect(result.errorNetwork?.statusCode).toBe(400); + expect(result.errorNetwork?.message).toMatch(/Received status code 400/); + expect(result).toEqual(responseBody); + resolve(); }, - makeCallback(resolve, reject, (e: ServerError) => { - expect(e.message).toMatch(/Received status code 400/); - expect(e.statusCode).toBe(400); - expect(e.result).toEqual(responseBody); - }), ); }); itAsync('throws an error if response code is > 300 and returns data', (resolve, reject) => { @@ -1106,18 +1105,11 @@ describe('HttpLink', () => { const link = createHttpLink({ uri: 'data', fetch: fetch as any }); - let called = false; - execute(link, { query: sampleQuery }).subscribe( result => { - called = true; + expect(result.errorNetwork?.statusCode).toBe(400); + expect(result.errorNetwork?.message).toMatch(/Received status code 400/); expect(result).toEqual(responseBody); - }, - e => { - expect(called).toBe(true); - expect(e.message).toMatch(/Received status code 400/); - expect(e.statusCode).toBe(400); - expect(e.result).toEqual(responseBody); resolve(); }, ); @@ -1131,12 +1123,9 @@ describe('HttpLink', () => { execute(link, { query: sampleQuery }).subscribe( result => { - reject('should not have called result because we have no data'); - }, - e => { - expect(e.message).toMatch(/Received status code 400/); - expect(e.statusCode).toBe(400); - expect(e.result).toEqual(responseBody); + expect(result.errorNetwork?.statusCode).toEqual(400); + expect(result.errorNetwork?.message).toMatch(/Received status code 400/); + expect(result.errorNetwork?.result).toBe(undefined); resolve(); }, ); @@ -1148,13 +1137,10 @@ describe('HttpLink', () => { execute(link, { query: sampleQuery }).subscribe( result => { - reject('next should have been thrown from the network'); + expect(result.errorNetwork?.message).toMatch(/Server response was missing for query 'SampleQuery'./); + resolve(); }, - makeCallback(resolve, reject, (e: Error) => { - expect(e.message).toMatch( - /Server response was missing for query 'SampleQuery'/, - ); - }), + ); }); itAsync("throws if the body can't be stringified", (resolve, reject) => { diff --git a/src/link/http/__tests__/parseAndCheckHttpResponse.ts b/src/link/http/__tests__/parseAndCheckHttpResponse.ts index 79d79cc7487..c961f770f17 100644 --- a/src/link/http/__tests__/parseAndCheckHttpResponse.ts +++ b/src/link/http/__tests__/parseAndCheckHttpResponse.ts @@ -18,7 +18,7 @@ describe('parseAndCheckResponse', () => { fetchMock.restore(); }); - const operations = [createOperation({}, { query })]; + const operations = [createOperation({}, { query, operationName:"SampleQuery" })]; itAsync('throws a parse error with a status code on unparsable response', (resolve, reject) => { const status = 400; @@ -45,12 +45,13 @@ describe('parseAndCheckResponse', () => { }); fetch('error') .then(parseAndCheckHttpResponse(operations)) - .then(reject) - .catch(e => { - expect(e.statusCode).toBe(status); - expect(e.name).toBe('ServerError'); + .then(({ data, errorNetwork:e }) => { + expect(data).toEqual('fail'); + expect(e.name).toEqual("ServerError"); + expect(e.statusCode).toEqual(status); expect(e).toHaveProperty('response'); - expect(e).toHaveProperty('result'); + expect(e.message).toEqual(`Response not successful: Received status code ${status}.`) + expect(e.result).toEqual(undefined) resolve(); }) .catch(reject); @@ -58,9 +59,21 @@ describe('parseAndCheckResponse', () => { itAsync('throws a server error on incorrect data', (resolve, reject) => { const data = { hello: 'world' }; //does not contain data or erros + const status = 200; fetchMock.mock('begin:/incorrect', data); fetch('incorrect') .then(parseAndCheckHttpResponse(operations)) + .then(({ data, errorNetwork:e }) => { + expect(data).toEqual(undefined); + expect(e.name).toEqual("ServerError"); + expect(e.statusCode).toEqual(status); + expect(e).toHaveProperty('response'); + const message = `Server response was missing for query '${operations.map(op => op.operationName)}'.`; + expect(e.message).toEqual(message) + expect(e.result).toEqual(undefined) + resolve(); + }) + /* .then(reject) .catch(e => { expect(e.statusCode).toBe(200); @@ -68,7 +81,7 @@ describe('parseAndCheckResponse', () => { expect(e).toHaveProperty('response'); expect(e.result).toEqual(data); resolve(); - }) + })*/ .catch(reject); }); diff --git a/src/link/http/parseAndCheckHttpResponse.ts b/src/link/http/parseAndCheckHttpResponse.ts index 2f314fd0a5b..33e80949b02 100644 --- a/src/link/http/parseAndCheckHttpResponse.ts +++ b/src/link/http/parseAndCheckHttpResponse.ts @@ -1,6 +1,5 @@ import { Operation } from '../core'; -import { throwServerError } from '../utils'; - +import { ServerError } from '../utils'; const { hasOwnProperty } = Object.prototype; export type ServerParseError = Error & { @@ -29,11 +28,13 @@ export function parseAndCheckHttpResponse( .then((result: any) => { if (response.status >= 300) { // Network error - throwServerError( - response, - result, - `Response not successful: Received status code ${response.status}`, - ); + const message = `Response not successful: Received status code ${response.status}.`; + const error = new Error(message) as ServerError; + error.name = 'ServerError'; + error.response = response; + error.statusCode = response.status; + result.error = error; + return result; } if ( @@ -41,16 +42,17 @@ export function parseAndCheckHttpResponse( !hasOwnProperty.call(result, 'data') && !hasOwnProperty.call(result, 'errors') ) { - // Data error - throwServerError( - response, - result, - `Server response was missing for query '${ - Array.isArray(operations) - ? operations.map(op => op.operationName) - : operations.operationName - }'.`, - ); + // Data error (Missing a useful response from the server) + const message = `Server response was missing for query '${ + Array.isArray(operations) + ? operations.map(op => op.operationName) + : operations.operationName + }'.`; + const error = new Error(message) as ServerError; + error.name = 'ServerError'; + error.response = response; + error.statusCode = response.status; + result.error = error; } return result; }); diff --git a/src/link/utils/throwServerError.ts b/src/link/utils/throwServerError.ts index b9283d16cd3..c1c6148e4e0 100644 --- a/src/link/utils/throwServerError.ts +++ b/src/link/utils/throwServerError.ts @@ -1,6 +1,6 @@ export type ServerError = Error & { response: Response; - result: Record; + result: Record | undefined; statusCode: number; }; diff --git a/src/react/hooks/useQuery.ts b/src/react/hooks/useQuery.ts index 348a121b58b..c0f76d802b0 100644 --- a/src/react/hooks/useQuery.ts +++ b/src/react/hooks/useQuery.ts @@ -560,7 +560,10 @@ class InternalState { // decided upon, we map errors (graphQLErrors) to the error options. // TODO: Is it possible for both result.error and result.errors to be // defined here? - queryResult.error = new ApolloError({ graphQLErrors: result.errors }); + queryResult.error = new ApolloError({ + graphQLErrors: result.errors, + networkError: result.error, + }); } return queryResult; From e2866df1d9fce62d08f7c3cdc6af594c98f79db6 Mon Sep 17 00:00:00 2001 From: MrDoomBringer Date: Mon, 22 Aug 2022 14:35:01 -0400 Subject: [PATCH 2/4] Handle result.error already existing --- src/core/QueryManager.ts | 2 +- src/link/http/parseAndCheckHttpResponse.ts | 13 +++++++++++-- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/src/core/QueryManager.ts b/src/core/QueryManager.ts index 6e3e4ddd7b5..09c8a846063 100644 --- a/src/core/QueryManager.ts +++ b/src/core/QueryManager.ts @@ -1047,7 +1047,7 @@ export class QueryManager { ), result => { - const hasErrors = isNonEmptyArray(result.errors); + const hasErrors = isNonEmptyArray(result.errors) || result.error; // If we interrupted this request by calling getResultsFromLink again // with the same QueryInfo object, we ignore the old results. diff --git a/src/link/http/parseAndCheckHttpResponse.ts b/src/link/http/parseAndCheckHttpResponse.ts index 33e80949b02..d2ff2f0cfcc 100644 --- a/src/link/http/parseAndCheckHttpResponse.ts +++ b/src/link/http/parseAndCheckHttpResponse.ts @@ -1,3 +1,4 @@ +import { ApolloError } from '../../errors'; import { Operation } from '../core'; import { ServerError } from '../utils'; const { hasOwnProperty } = Object.prototype; @@ -33,7 +34,11 @@ export function parseAndCheckHttpResponse( error.name = 'ServerError'; error.response = response; error.statusCode = response.status; - result.error = error; + if (result.error) { + result.error.networkError = error; + } else { + result.error = new ApolloError({networkError: error}); + } return result; } @@ -52,7 +57,11 @@ export function parseAndCheckHttpResponse( error.name = 'ServerError'; error.response = response; error.statusCode = response.status; - result.error = error; + if (result.error) { + result.error.networkError = error; + } else { + result.error = new ApolloError({networkError: error}); + } } return result; }); From 844b05efd8307ee9ce7588c0d0213414187e544d Mon Sep 17 00:00:00 2001 From: MrDoomBringer Date: Mon, 22 Aug 2022 14:35:10 -0400 Subject: [PATCH 3/4] Update tests --- src/cache/core/types/Cache.ts | 4 --- src/core/__tests__/ObservableQuery.ts | 6 ++-- src/link/http/__tests__/HttpLink.ts | 31 +++++++++---------- .../__tests__/parseAndCheckHttpResponse.ts | 20 +++++------- 4 files changed, 26 insertions(+), 35 deletions(-) diff --git a/src/cache/core/types/Cache.ts b/src/cache/core/types/Cache.ts index 362e9c79ed8..4086d34a45e 100644 --- a/src/cache/core/types/Cache.ts +++ b/src/cache/core/types/Cache.ts @@ -1,8 +1,6 @@ import { DataProxy } from './DataProxy'; import { Modifier, Modifiers } from './common'; import { ApolloCache } from '../cache'; -import { ServerError } from '../../../core'; -import { GraphQLError } from 'graphql'; export namespace Cache { export type WatchCallback = ( @@ -25,8 +23,6 @@ export namespace Cache { { dataId?: string; result: TResult; - error?: ServerError; - errors?: ReadonlyArray; } export interface DiffOptions< diff --git a/src/core/__tests__/ObservableQuery.ts b/src/core/__tests__/ObservableQuery.ts index 9424a64df6b..98faf40f64c 100644 --- a/src/core/__tests__/ObservableQuery.ts +++ b/src/core/__tests__/ObservableQuery.ts @@ -1860,9 +1860,11 @@ describe('ObservableQuery', () => { }); itAsync('returns errors with data if errorPolicy is all', (resolve, reject) => { + const networkError = new ApolloError({errorMessage: "Oh no"}) + const queryManager = mockQueryManager(reject, { request: { query, variables }, - result: { data: dataOne, errors: [error] }, + result: { data: dataOne, errors: [error], error: networkError}, }); const observable = queryManager.watchQuery({ @@ -1877,7 +1879,7 @@ describe('ObservableQuery', () => { const currentResult = observable.getCurrentResult(); expect(currentResult.loading).toBe(false); expect(currentResult.errors).toEqual([error]); - expect(currentResult.error).toBeUndefined(); + expect(currentResult.error).toEqual(networkError); }).then(resolve, reject); }); diff --git a/src/link/http/__tests__/HttpLink.ts b/src/link/http/__tests__/HttpLink.ts index 1cfc0381aa3..5bcaa7fb25b 100644 --- a/src/link/http/__tests__/HttpLink.ts +++ b/src/link/http/__tests__/HttpLink.ts @@ -1026,18 +1026,15 @@ describe('HttpLink', () => { describe('Error handling', () => { let responseBody: any; - const networkError = new Error(`Response not successful: Received status code 400.`) as ServerError; const text = jest.fn(() => { const responseBodyText = '{}'; responseBody = JSON.parse(responseBodyText); - responseBody.errorNetwork = networkError return Promise.resolve(responseBodyText); }); const textWithData = jest.fn(() => { responseBody = { data: { stub: { id: 1 } }, errors: [{ message: 'dangit' }], - errorNetwork: networkError, }; return Promise.resolve(JSON.stringify(responseBody)); @@ -1046,7 +1043,6 @@ describe('HttpLink', () => { const textWithErrors = jest.fn(() => { responseBody = { errors: [{ message: 'dangit' }], - errorNetwork: networkError, }; return Promise.resolve(JSON.stringify(responseBody)); @@ -1077,8 +1073,9 @@ describe('HttpLink', () => { execute(link, { query: sampleQuery }).subscribe( result => { - expect(result.errorNetwork?.statusCode).toEqual(401); - expect(result.errorNetwork?.message).toMatch(/Received status code 401/); + const error = result.error?.networkError as ServerError + expect(error.statusCode).toEqual(401); + expect(error.message).toMatch(/Received status code 401/); resolve(); }, () => {}, @@ -1091,9 +1088,10 @@ describe('HttpLink', () => { execute(link, { query: sampleQuery }).subscribe( result => { - expect(result.errorNetwork?.statusCode).toBe(400); - expect(result.errorNetwork?.message).toMatch(/Received status code 400/); - expect(result).toEqual(responseBody); + const error = result.error?.networkError as ServerError + expect(error.statusCode).toBe(400); + expect(error.message).toMatch(/Received status code 400/); + resolve(); }, ); @@ -1107,9 +1105,9 @@ describe('HttpLink', () => { execute(link, { query: sampleQuery }).subscribe( result => { - expect(result.errorNetwork?.statusCode).toBe(400); - expect(result.errorNetwork?.message).toMatch(/Received status code 400/); - expect(result).toEqual(responseBody); + const error = result.error?.networkError as ServerError + expect(error.statusCode).toBe(400); + expect(error.message).toMatch(/Received status code 400/); resolve(); }, ); @@ -1123,9 +1121,9 @@ describe('HttpLink', () => { execute(link, { query: sampleQuery }).subscribe( result => { - expect(result.errorNetwork?.statusCode).toEqual(400); - expect(result.errorNetwork?.message).toMatch(/Received status code 400/); - expect(result.errorNetwork?.result).toBe(undefined); + const error = result.error?.networkError as ServerError + expect(error.statusCode).toEqual(400); + expect(error.message).toMatch(/Received status code 400/); resolve(); }, ); @@ -1137,7 +1135,8 @@ describe('HttpLink', () => { execute(link, { query: sampleQuery }).subscribe( result => { - expect(result.errorNetwork?.message).toMatch(/Server response was missing for query 'SampleQuery'./); + const error = result.error?.networkError as ServerError + expect(error.message).toMatch(/Server response was missing for query 'SampleQuery'./); resolve(); }, diff --git a/src/link/http/__tests__/parseAndCheckHttpResponse.ts b/src/link/http/__tests__/parseAndCheckHttpResponse.ts index c961f770f17..52745cd33ad 100644 --- a/src/link/http/__tests__/parseAndCheckHttpResponse.ts +++ b/src/link/http/__tests__/parseAndCheckHttpResponse.ts @@ -4,6 +4,7 @@ import fetchMock from 'fetch-mock'; import { createOperation } from '../../utils/createOperation'; import { parseAndCheckHttpResponse } from '../parseAndCheckHttpResponse'; import { itAsync } from '../../../testing'; +import { ServerError } from '../../utils'; const query = gql` query SampleQuery { @@ -36,7 +37,7 @@ describe('parseAndCheckResponse', () => { .catch(reject); }); - itAsync('throws a network error with a status code and result', (resolve, reject) => { + itAsync('returns a network error with a status code and result', (resolve, reject) => { const status = 403; const body = { data: 'fail' }; //does not contain data or errors fetchMock.mock('begin:/error', { @@ -45,7 +46,8 @@ describe('parseAndCheckResponse', () => { }); fetch('error') .then(parseAndCheckHttpResponse(operations)) - .then(({ data, errorNetwork:e }) => { + .then(({ data, error }) => { + const e = error.networkError as ServerError expect(data).toEqual('fail'); expect(e.name).toEqual("ServerError"); expect(e.statusCode).toEqual(status); @@ -57,13 +59,14 @@ describe('parseAndCheckResponse', () => { .catch(reject); }); - itAsync('throws a server error on incorrect data', (resolve, reject) => { + itAsync('returns a server error on incorrect data', (resolve, reject) => { const data = { hello: 'world' }; //does not contain data or erros const status = 200; fetchMock.mock('begin:/incorrect', data); fetch('incorrect') .then(parseAndCheckHttpResponse(operations)) - .then(({ data, errorNetwork:e }) => { + .then(({ data, error }) => { + const e = error.networkError as ServerError expect(data).toEqual(undefined); expect(e.name).toEqual("ServerError"); expect(e.statusCode).toEqual(status); @@ -73,15 +76,6 @@ describe('parseAndCheckResponse', () => { expect(e.result).toEqual(undefined) resolve(); }) - /* - .then(reject) - .catch(e => { - expect(e.statusCode).toBe(200); - expect(e.name).toBe('ServerError'); - expect(e).toHaveProperty('response'); - expect(e.result).toEqual(data); - resolve(); - })*/ .catch(reject); }); From 239677efd94d2b319dd33a21aa5933de33ee4821 Mon Sep 17 00:00:00 2001 From: MrDoomBringer Date: Mon, 22 Aug 2022 20:09:13 -0400 Subject: [PATCH 4/4] Bundlesize 29.98->30.01 --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 4671c2f2f59..15004a568fc 100644 --- a/package.json +++ b/package.json @@ -56,7 +56,7 @@ { "name": "apollo-client", "path": "./dist/apollo-client.min.cjs", - "maxSize": "29.98kB" + "maxSize": "30.01kB" } ], "engines": {