From 27c85d0151db8c14f142de6d8c36da9f4c7e2cd2 Mon Sep 17 00:00:00 2001 From: MrDoomBringer Date: Mon, 25 Jul 2022 15:10:55 -0400 Subject: [PATCH] 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;