From a205644ce6ed8a93f306743bf4a868b1f3406076 Mon Sep 17 00:00:00 2001 From: hwillson Date: Thu, 1 Oct 2020 15:16:06 -0400 Subject: [PATCH 1/6] Adjust onError to use the Observable's error callback by default This means errors are returned through a Link's Observable by default, instead of thrown as an uncaught exception. --- src/link/core/ApolloLink.ts | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/link/core/ApolloLink.ts b/src/link/core/ApolloLink.ts index 62ef169ca3c..f9dce653aa2 100644 --- a/src/link/core/ApolloLink.ts +++ b/src/link/core/ApolloLink.ts @@ -141,11 +141,13 @@ export class ApolloLink { throw new InvariantError('request is not implemented'); } - protected onError(reason: any) { - throw reason; + protected onError(error: any, observer: ZenObservable.Observer) { + observer.error!(error); } - public setOnError(fn: (reason: any) => any): this { + public setOnError( + fn: (error: any, observer?: ZenObservable.Observer) => any + ): this { this.onError = fn; return this; } From 754d48dcb39eebe9a906026b8f31ce192c25f668 Mon Sep 17 00:00:00 2001 From: hwillson Date: Thu, 1 Oct 2020 15:19:19 -0400 Subject: [PATCH 2/6] New tests and changes to validate errors through the link chain Instead of testing errors as uncaught exceptions. --- .../mocking/__tests__/MockedProvider.test.tsx | 137 ++++++++++++++---- .../MockedProvider.test.tsx.snap | 52 +++++++ 2 files changed, 157 insertions(+), 32 deletions(-) diff --git a/src/utilities/testing/mocking/__tests__/MockedProvider.test.tsx b/src/utilities/testing/mocking/__tests__/MockedProvider.test.tsx index a89efcb2881..84800eb056c 100644 --- a/src/utilities/testing/mocking/__tests__/MockedProvider.test.tsx +++ b/src/utilities/testing/mocking/__tests__/MockedProvider.test.tsx @@ -76,7 +76,7 @@ describe('General use', () => { errorThrown = false; }); - itAsync('should mock the data', async (resolve, reject) => { + itAsync('should mock the data', (resolve, reject) => { function Component({ username }: Variables) { const { loading, data } = useQuery(query, { variables }); if (!loading) { @@ -94,7 +94,7 @@ describe('General use', () => { return wait().then(resolve, reject); }); - itAsync('should allow querying with the typename', async (resolve, reject) => { + itAsync('should allow querying with the typename', (resolve, reject) => { function Component({ username }: Variables) { const { loading, data } = useQuery(query, { variables }); if (!loading) { @@ -122,7 +122,7 @@ describe('General use', () => { return wait().then(resolve, reject); }); - itAsync('should allow using a custom cache', async (resolve, reject) => { + itAsync('should allow using a custom cache', (resolve, reject) => { const cache = new InMemoryCache(); cache.writeQuery({ query, @@ -147,9 +147,12 @@ describe('General use', () => { return wait().then(resolve, reject); }); - itAsync('should error if the variables in the mock and component do not match', async (resolve, reject) => { + itAsync('should error if the variables in the mock and component do not match', (resolve, reject) => { function Component({ ...variables }: Variables) { - useQuery(query, { variables }); + const { loading, error } = useQuery(query, { variables }); + if (!loading) { + expect(error).toMatchSnapshot(); + } return null; } @@ -157,22 +160,21 @@ describe('General use', () => { username: 'other_user' }; - const link = ApolloLink.from([errorLink, new MockLink(mocks)]); - render( - + ); - return wait(() => { - expect(errorThrown).toBeTruthy(); - }).then(resolve, reject); + return wait().then(resolve, reject); }); - itAsync('should error if the variables do not deep equal', async (resolve, reject) => { + itAsync('should error if the variables do not deep equal', (resolve, reject) => { function Component({ ...variables }: Variables) { - useQuery(query, { variables }); + const { loading, error } = useQuery(query, { variables }); + if (!loading) { + expect(error).toMatchSnapshot(); + } return null; } @@ -194,20 +196,16 @@ describe('General use', () => { age: 42 }; - const link = ApolloLink.from([errorLink, new MockLink(mocks2)]); - render( - + ); - return wait(() => { - expect(errorThrown).toBeTruthy(); - }).then(resolve, reject); + return wait().then(resolve, reject); }); - itAsync('should not error if the variables match but have different order', async (resolve, reject) => { + itAsync('should not error if the variables match but have different order', (resolve, reject) => { function Component({ ...variables }: Variables) { const { loading, data } = useQuery(query, { variables }); if (!loading) { @@ -243,7 +241,7 @@ describe('General use', () => { return wait().then(resolve, reject); }); - itAsync('should support mocking a network error', async (resolve, reject) => { + itAsync('should support mocking a network error', (resolve, reject) => { function Component({ ...variables }: Variables) { const { loading, error } = useQuery(query, { variables }); if (!loading) { @@ -271,9 +269,12 @@ describe('General use', () => { return wait().then(resolve, reject); }); - itAsync('should error if the query in the mock and component do not match', async (resolve, reject) => { + itAsync('should error if the query in the mock and component do not match', (resolve, reject) => { function Component({ ...variables }: Variables) { - useQuery(query, { variables }); + const { loading, error } = useQuery(query, { variables }); + if (!loading) { + expect(error).toMatchSnapshot(); + } return null; } @@ -293,17 +294,13 @@ describe('General use', () => { } ]; - const link = ApolloLink.from([errorLink, new MockLink(mocksDifferentQuery)]); - render( - + ); - return wait(() => { - expect(errorThrown).toBeTruthy(); - }).then(resolve, reject); + return wait().then(resolve, reject); }); it('should pass down props prop in mock as props for the component', () => { @@ -334,7 +331,7 @@ describe('General use', () => { unmount(); }); - itAsync('should support returning mocked results from a function', async (resolve, reject) => { + itAsync('should support returning mocked results from a function', (resolve, reject) => { let resultReturned = false; const testUser = { @@ -390,10 +387,86 @@ describe('General use', () => { return wait().then(resolve, reject); }); + + itAsync('should return "No more mocked responses" errors in response', (resolve, reject) => { + function Component() { + const { loading, error } = useQuery(query); + if (!loading) { + expect(error).toMatchSnapshot(); + } + return null; + } + + const link = ApolloLink.from([errorLink, new MockLink([])]); + + render( + + + + ); + + return wait(() => { + // The "No more mocked responses" error should not be thrown as an + // uncaught exception. + expect(errorThrown).toBeFalsy(); + }).then(resolve, reject); + }); + + itAsync('should return "Mocked response should contain" errors in response', (resolve, reject) => { + function Component({ ...variables }: Variables) { + const { loading, error } = useQuery(query, { variables }); + if (!loading) { + expect(error).toMatchSnapshot(); + } + return null; + } + + const link = ApolloLink.from([errorLink, new MockLink([ + { + request: { + query, + variables + } + } + ])]); + + render( + + + + ); + + return wait(() => { + // The "Mocked response should contain" error should not be thrown as an + // uncaught exception. + expect(errorThrown).toBeFalsy(); + }).then(resolve, reject); + }); + + itAsync('should support custom error handling using setOnError', (resolve, reject) => { + function Component({ ...variables }: Variables) { + useQuery(query, { variables }); + return null; + } + + const mockLink = new MockLink([]); + mockLink.setOnError(error => { + expect(error).toMatchSnapshot(); + }); + const link = ApolloLink.from([errorLink, mockLink]); + + render( + + + + ); + + return wait().then(resolve, reject); + }); }); describe('@client testing', () => { - itAsync('should support @client fields with a custom cache', async (resolve, reject) => { + itAsync('should support @client fields with a custom cache', (resolve, reject) => { const cache = new InMemoryCache(); cache.writeQuery({ @@ -432,7 +505,7 @@ describe('@client testing', () => { return wait().then(resolve, reject); }); - itAsync('should support @client fields with field policies', async (resolve, reject) => { + itAsync('should support @client fields with field policies', (resolve, reject) => { const cache = new InMemoryCache({ typePolicies: { Query: { diff --git a/src/utilities/testing/mocking/__tests__/__snapshots__/MockedProvider.test.tsx.snap b/src/utilities/testing/mocking/__tests__/__snapshots__/MockedProvider.test.tsx.snap index 32de2833292..98eb92671e6 100644 --- a/src/utilities/testing/mocking/__tests__/__snapshots__/MockedProvider.test.tsx.snap +++ b/src/utilities/testing/mocking/__tests__/__snapshots__/MockedProvider.test.tsx.snap @@ -7,6 +7,36 @@ Object { } `; +exports[`General use should error if the query in the mock and component do not match 1`] = ` +[Error: No more mocked responses for the query: query GetUser($username: String!) { + user(username: $username) { + id + __typename + } +} +, variables: {"username":"mock_username"}] +`; + +exports[`General use should error if the variables do not deep equal 1`] = ` +[Error: No more mocked responses for the query: query GetUser($username: String!) { + user(username: $username) { + id + __typename + } +} +, variables: {"username":"some_user","age":42}] +`; + +exports[`General use should error if the variables in the mock and component do not match 1`] = ` +[Error: No more mocked responses for the query: query GetUser($username: String!) { + user(username: $username) { + id + __typename + } +} +, variables: {"username":"other_user"}] +`; + exports[`General use should mock the data 1`] = ` Object { "__typename": "User", @@ -22,3 +52,25 @@ Object { }, } `; + +exports[`General use should return "Mocked response should contain" errors in response 1`] = `[Error: Mocked response should contain either result or error: {"query":"query GetUser($username: String!) {\\n user(username: $username) {\\n id\\n __typename\\n }\\n}\\n"}]`; + +exports[`General use should return "No more mocked responses" errors in response 1`] = ` +[Error: No more mocked responses for the query: query GetUser($username: String!) { + user(username: $username) { + id + __typename + } +} +, variables: {}] +`; + +exports[`General use should support custom error handling using setOnError 1`] = ` +[Error: No more mocked responses for the query: query GetUser($username: String!) { + user(username: $username) { + id + __typename + } +} +, variables: {"username":"mock_username"}] +`; From 8ce27c628b82a5e1d4fea7f661569c34a9bb599b Mon Sep 17 00:00:00 2001 From: hwillson Date: Thu, 1 Oct 2020 15:20:45 -0400 Subject: [PATCH 3/6] Send MockLink config errors through ApolloLink.onError --- .../mocking/__tests__/MockedProvider.test.tsx | 25 +++++++++ .../MockedProvider.test.tsx.snap | 2 + src/utilities/testing/mocking/mockLink.ts | 54 ++++++++++--------- 3 files changed, 56 insertions(+), 25 deletions(-) diff --git a/src/utilities/testing/mocking/__tests__/MockedProvider.test.tsx b/src/utilities/testing/mocking/__tests__/MockedProvider.test.tsx index 84800eb056c..378a5e8ebde 100644 --- a/src/utilities/testing/mocking/__tests__/MockedProvider.test.tsx +++ b/src/utilities/testing/mocking/__tests__/MockedProvider.test.tsx @@ -463,6 +463,31 @@ describe('General use', () => { return wait().then(resolve, reject); }); + + itAsync('should pipe exceptions thrown in custom onError functions through the link chain', (resolve, reject) => { + function Component({ ...variables }: Variables) { + const { loading, error } = useQuery(query, { variables }); + if (!loading) { + console.log(error); + expect(error).toMatchSnapshot(); + } + return null; + } + + const mockLink = new MockLink([]); + mockLink.setOnError(() => { + throw new Error('oh no!'); + }); + const link = ApolloLink.from([errorLink, mockLink]); + + render( + + + + ); + + return wait().then(resolve, reject); + }); }); describe('@client testing', () => { diff --git a/src/utilities/testing/mocking/__tests__/__snapshots__/MockedProvider.test.tsx.snap b/src/utilities/testing/mocking/__tests__/__snapshots__/MockedProvider.test.tsx.snap index 98eb92671e6..89d4ffd6cef 100644 --- a/src/utilities/testing/mocking/__tests__/__snapshots__/MockedProvider.test.tsx.snap +++ b/src/utilities/testing/mocking/__tests__/__snapshots__/MockedProvider.test.tsx.snap @@ -53,6 +53,8 @@ Object { } `; +exports[`General use should pipe exceptions thrown in custom onError functions through the link chain 1`] = `[Error: oh no!]`; + exports[`General use should return "Mocked response should contain" errors in response 1`] = `[Error: Mocked response should contain either result or error: {"query":"query GetUser($username: String!) {\\n user(username: $username) {\\n id\\n __typename\\n }\\n}\\n"}]`; exports[`General use should return "No more mocked responses" errors in response 1`] = ` diff --git a/src/utilities/testing/mocking/mockLink.ts b/src/utilities/testing/mocking/mockLink.ts index c74b5dba244..8456147a806 100644 --- a/src/utilities/testing/mocking/mockLink.ts +++ b/src/utilities/testing/mocking/mockLink.ts @@ -85,49 +85,53 @@ export class MockLink extends ApolloLink { } ); + let configError: Error; + if (!response || typeof responseIndex === 'undefined') { - this.onError(new Error( + configError = new Error( `No more mocked responses for the query: ${print( operation.query )}, variables: ${JSON.stringify(operation.variables)}` - )); - return null; - } - - this.mockedResponsesByKey[key].splice(responseIndex, 1); - - const { newData } = response!; - - if (newData) { - response!.result = newData(); - this.mockedResponsesByKey[key].push(response!); - } + ); + } else { + this.mockedResponsesByKey[key].splice(responseIndex, 1); - const { result, error, delay } = response!; + const { newData } = response; + if (newData) { + response.result = newData(); + this.mockedResponsesByKey[key].push(response); + } - if (!result && !error) { - this.onError(new Error( - `Mocked response should contain either result or error: ${key}` - )); + if (!response.result && !response.error) { + configError = new Error( + `Mocked response should contain either result or error: ${key}` + ); + } } return new Observable(observer => { let timer = setTimeout( () => { - if (error) { - observer.error(error); + if (configError) { + try { + this.onError(configError, observer); + } catch (error) { + observer.error(error); + } + } else if (response!.error) { + observer.error(response!.error); } else { - if (result) { + if (response!.result) { observer.next( - typeof result === 'function' - ? (result as ResultFunction)() - : result + typeof response!.result === 'function' + ? (response!.result as ResultFunction)() + : response!.result ); } observer.complete(); } }, - delay ? delay : 0 + response?.delay || 0 ); return () => { From 15947cbe7030a97f2740648279312cafcb0622e6 Mon Sep 17 00:00:00 2001 From: hwillson Date: Thu, 1 Oct 2020 15:25:04 -0400 Subject: [PATCH 4/6] Remove custom setOnError functions that conflict with error testing Now that errors are back to being handled in the link chain, some of the custom setOnError functions we added are rejecting tests before we have a chance to validate the expected errors. This commit removes the conflicting setOnError functions. --- src/__tests__/client.ts | 6 +++--- src/__tests__/fetchMore.ts | 4 ++-- src/__tests__/mutationResults.ts | 2 +- src/__tests__/optimistic.ts | 2 +- src/core/__tests__/fetchPolicies.ts | 8 ++++---- src/utilities/testing/mocking/mockQueryManager.ts | 2 +- 6 files changed, 12 insertions(+), 12 deletions(-) diff --git a/src/__tests__/client.ts b/src/__tests__/client.ts index f015f25f7bf..7f7c84fa2ef 100644 --- a/src/__tests__/client.ts +++ b/src/__tests__/client.ts @@ -1768,7 +1768,7 @@ describe('client', () => { }); itAsync('fails if network request fails', (resolve, reject) => { - const link = mockSingleLink().setOnError(error => { throw error }); // no queries = no replies. + const link = mockSingleLink(); // no queries = no replies. const client = new ApolloClient({ link, cache: new InMemoryCache({ addTypename: false }), @@ -2028,7 +2028,7 @@ describe('client', () => { request: { query: mutation }, result: { data }, error: networkError, - }).setOnError(reject), + }), cache: new InMemoryCache({ addTypename: false }), }); @@ -2475,7 +2475,7 @@ describe('client', () => { { request: { query }, result: { data } }, { request: { query }, error: new Error('This is an error!') }, { request: { query }, result: { data: dataTwo } } - ).setOnError(reject); + ); const client = new ApolloClient({ link, cache: new InMemoryCache({ addTypename: false }), diff --git a/src/__tests__/fetchMore.ts b/src/__tests__/fetchMore.ts index 9e983f7d935..7b7a5512387 100644 --- a/src/__tests__/fetchMore.ts +++ b/src/__tests__/fetchMore.ts @@ -620,7 +620,7 @@ describe('fetchMore on an observable query', () => { request: { query, variables: variablesMore }, error: fetchMoreError, delay: 5, - }).setOnError(reject); + }); const client = new ApolloClient({ link, @@ -674,7 +674,7 @@ describe('fetchMore on an observable query', () => { request: { query, variables: variablesMore }, error: fetchMoreError, delay: 5, - }).setOnError(reject); + }); let calledFetchMore = false; diff --git a/src/__tests__/mutationResults.ts b/src/__tests__/mutationResults.ts index ed36c587cba..3da12270b34 100644 --- a/src/__tests__/mutationResults.ts +++ b/src/__tests__/mutationResults.ts @@ -124,7 +124,7 @@ describe('mutation results', () => { link: mockSingleLink({ request: { query: queryWithTypename } as any, result, - }, ...mockedResponses).setOnError(reject), + }, ...mockedResponses), cache: new InMemoryCache({ dataIdFromObject: (obj: any) => { if (obj.id && obj.__typename) { diff --git a/src/__tests__/optimistic.ts b/src/__tests__/optimistic.ts index 28a7e82d7e4..9dd5bc74978 100644 --- a/src/__tests__/optimistic.ts +++ b/src/__tests__/optimistic.ts @@ -123,7 +123,7 @@ describe('optimistic mutation results', () => { const link = mockSingleLink({ request: { query }, result, - }, ...mockedResponses).setOnError(reject); + }, ...mockedResponses); const client = new ApolloClient({ link, diff --git a/src/core/__tests__/fetchPolicies.ts b/src/core/__tests__/fetchPolicies.ts index 5f1f0731982..b67128eb701 100644 --- a/src/core/__tests__/fetchPolicies.ts +++ b/src/core/__tests__/fetchPolicies.ts @@ -65,14 +65,14 @@ const createLink = (reject: (reason: any) => any) => result: { data: result }, }).setOnError(reject); -const createFailureLink = (reject: (reason: any) => any) => +const createFailureLink = () => mockSingleLink({ request: { query }, error: new Error('query failed'), }, { request: { query }, result: { data: result }, - }).setOnError(reject); + }); const createMutationLink = (reject: (reason: any) => any) => // fetch the data @@ -149,7 +149,7 @@ describe('network-only', () => { }); const client = new ApolloClient({ - link: inspector.concat(createFailureLink(reject)), + link: inspector.concat(createFailureLink()), cache: new InMemoryCache({ addTypename: false }), }); @@ -280,7 +280,7 @@ describe('no-cache', () => { }); const client = new ApolloClient({ - link: inspector.concat(createFailureLink(reject)), + link: inspector.concat(createFailureLink()), cache: new InMemoryCache({ addTypename: false }), }); diff --git a/src/utilities/testing/mocking/mockQueryManager.ts b/src/utilities/testing/mocking/mockQueryManager.ts index 59c3cbe0f19..ce2ad1aa700 100644 --- a/src/utilities/testing/mocking/mockQueryManager.ts +++ b/src/utilities/testing/mocking/mockQueryManager.ts @@ -9,7 +9,7 @@ export default ( ...mockedResponses: MockedResponse[] ) => { return new QueryManager({ - link: mockSingleLink(...mockedResponses).setOnError(reject), + link: mockSingleLink(...mockedResponses), cache: new InMemoryCache({ addTypename: false }), }); }; From a2bf2c2fe61ebff3d368d0722e42589aa0edb3e8 Mon Sep 17 00:00:00 2001 From: hwillson Date: Thu, 1 Oct 2020 15:51:10 -0400 Subject: [PATCH 5/6] Changelog update --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9af95232892..90f869623fa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,10 @@ - `HttpLink` will now automatically strip any unused `variables` before sending queries to the GraphQL server, since those queries are very likely to fail validation, according to the [All Variables Used](https://spec.graphql.org/draft/#sec-All-Variables-Used) rule in the GraphQL specification. If you depend on the preservation of unused variables, you can restore the previous behavior by passing `includeUnusedVariables: true` to the `HttpLink` constructor (which is typically passed as `options.link` to the `ApolloClient` constructor).
[@benjamn](https://github.com/benjamn) in [#7127](https://github.com/apollographql/apollo-client/pull/7127) +- Ensure `MockLink` (used by `MockedProvider`) returns mock configuration errors (e.g. `No more mocked responses for the query ...`) through the Link's `Observable`, instead of throwing them. These errors are now available through the `error` property of a result.
+ [@hwillson](https://github.com/hwillson) in [#7110](https://github.com/apollographql/apollo-client/pull/7110) + > Returning mock configuration errors through the Link's `Observable` was the default behavior in Apollo Client 2.x. We changed it for 3, but the change has been problematic for those looking to migrate from 2.x to 3. We've decided to change this back with the understanding that not many people want or are relying on `MockLink`'s throwing exception approach. If you want to change this functionality, you can define custom error handling through `MockLink.setOnError`. + ## Improvements - Support inheritance of type and field policies, according to `possibleTypes`.
From d1ce23eba19f3326e4e381e82f0888fc0e97eb66 Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Fri, 9 Oct 2020 17:26:13 -0400 Subject: [PATCH 6/6] Let mockLink.onError return false to prevent observer.error. https://github.com/apollographql/apollo-client/pull/7110#discussion_r500419910 --- src/link/core/ApolloLink.ts | 22 ++++++++++--- src/utilities/testing/mocking/mockLink.ts | 38 +++++++++++++---------- 2 files changed, 39 insertions(+), 21 deletions(-) diff --git a/src/link/core/ApolloLink.ts b/src/link/core/ApolloLink.ts index f9dce653aa2..2cc0aa8e26f 100644 --- a/src/link/core/ApolloLink.ts +++ b/src/link/core/ApolloLink.ts @@ -141,13 +141,25 @@ export class ApolloLink { throw new InvariantError('request is not implemented'); } - protected onError(error: any, observer: ZenObservable.Observer) { - observer.error!(error); + protected onError( + error: any, + observer: ZenObservable.Observer, + ): false | void { + if (observer && observer.error) { + observer.error(error); + // Returning false indicates that observer.error does not need to be + // called again, since it was already called (on the previous line). + // Calling observer.error again would not cause any real problems, + // since only the first call matters, but custom onError functions + // might have other reasons for wanting to prevent the default + // behavior by returning false. + return false; + } + // Throw errors will be passed to observer.error. + throw error; } - public setOnError( - fn: (error: any, observer?: ZenObservable.Observer) => any - ): this { + public setOnError(fn: ApolloLink["onError"]): this { this.onError = fn; return this; } diff --git a/src/utilities/testing/mocking/mockLink.ts b/src/utilities/testing/mocking/mockLink.ts index 8456147a806..a754c3640bc 100644 --- a/src/utilities/testing/mocking/mockLink.ts +++ b/src/utilities/testing/mocking/mockLink.ts @@ -110,29 +110,35 @@ export class MockLink extends ApolloLink { } return new Observable(observer => { - let timer = setTimeout( - () => { - if (configError) { - try { - this.onError(configError, observer); - } catch (error) { - observer.error(error); + const timer = setTimeout(() => { + if (configError) { + try { + // The onError function can return false to indicate that + // configError need not be passed to observer.error. For + // example, the default implementation of onError calls + // observer.error(configError) and then returns false to + // prevent this extra (harmless) observer.error call. + if (this.onError(configError, observer) !== false) { + throw configError; } - } else if (response!.error) { - observer.error(response!.error); + } catch (error) { + observer.error(error); + } + } else if (response) { + if (response.error) { + observer.error(response.error); } else { - if (response!.result) { + if (response.result) { observer.next( - typeof response!.result === 'function' - ? (response!.result as ResultFunction)() - : response!.result + typeof response.result === 'function' + ? (response.result as ResultFunction)() + : response.result ); } observer.complete(); } - }, - response?.delay || 0 - ); + } + }, response && response.delay || 0); return () => { clearTimeout(timer);