From db58fc154ea7f136b79598774a027a5bdc799756 Mon Sep 17 00:00:00 2001 From: Jerel Miller Date: Mon, 3 Jul 2023 01:06:44 -0600 Subject: [PATCH 1/5] Add `.catch` handler for rejected promises to prevent unhandled rejection errors --- src/utilities/promises/decoration.ts | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/utilities/promises/decoration.ts b/src/utilities/promises/decoration.ts index 1ace8efe2ee..a8d5210b3f9 100644 --- a/src/utilities/promises/decoration.ts +++ b/src/utilities/promises/decoration.ts @@ -29,6 +29,11 @@ export function createFulfilledPromise(value: TValue) { export function createRejectedPromise(reason: unknown) { const promise = Promise.reject(reason) as RejectedPromise; + // prevent unhandled error rejections since this is usually used with __use + // which synchronously reads the `status` and `reason` properties to throw + // the error. + promise.catch(() => {}); + promise.status = 'rejected'; promise.reason = reason; From 5e79131e09216ab23014d777b8d638aadc619b4a Mon Sep 17 00:00:00 2001 From: Jerel Miller Date: Mon, 3 Jul 2023 01:08:56 -0600 Subject: [PATCH 2/5] Expect errors returned in deferred chunks throw when error policy is `none` --- src/react/cache/QueryReference.ts | 4 +- .../hooks/__tests__/useSuspenseQuery.test.tsx | 73 +++++-------------- 2 files changed, 18 insertions(+), 59 deletions(-) diff --git a/src/react/cache/QueryReference.ts b/src/react/cache/QueryReference.ts index d593198136f..8a047d1a0e2 100644 --- a/src/react/cache/QueryReference.ts +++ b/src/react/cache/QueryReference.ts @@ -202,9 +202,7 @@ export class InternalQueryReference { } this.result = result; - this.promise = result.data - ? createFulfilledPromise(result) - : createRejectedPromise(result); + this.promise = createRejectedPromise(error); this.deliver(this.promise); } diff --git a/src/react/hooks/__tests__/useSuspenseQuery.test.tsx b/src/react/hooks/__tests__/useSuspenseQuery.test.tsx index b5c3b738878..b19038f51d1 100644 --- a/src/react/hooks/__tests__/useSuspenseQuery.test.tsx +++ b/src/react/hooks/__tests__/useSuspenseQuery.test.tsx @@ -6101,7 +6101,9 @@ describe('useSuspenseQuery', () => { consoleSpy.mockRestore(); }); - it('discards partial data and does not throw errors returned in incremental chunks but returns them in `error` property', async () => { + it('discards partial data and throws errors returned in incremental chunks', async () => { + const consoleSpy = jest.spyOn(console, 'error').mockImplementation(); + const query = gql` query { hero { @@ -6196,37 +6198,9 @@ describe('useSuspenseQuery', () => { }); await waitFor(() => { - expect(result.current).toMatchObject({ - data: { - hero: { - heroFriends: [ - { - id: '1000', - name: 'Luke Skywalker', - }, - { - id: '1003', - name: 'Leia Organa', - }, - ], - name: 'R2-D2', - }, - }, - networkStatus: NetworkStatus.error, - error: new ApolloError({ - graphQLErrors: [ - new GraphQLError( - 'homeWorld for character with ID 1000 could not be fetched.', - { path: ['hero', 'heroFriends', 0, 'homeWorld'] } - ), - ], - }), - }); + expect(renders.errorCount).toBe(1); }); - expect(result.current.error).toBeInstanceOf(ApolloError); - - expect(renders.count).toBe(3); expect(renders.suspenseCount).toBe(1); expect(renders.frames).toMatchObject([ { @@ -6248,33 +6222,20 @@ describe('useSuspenseQuery', () => { networkStatus: NetworkStatus.ready, error: undefined, }, - { - data: { - hero: { - heroFriends: [ - { - id: '1000', - name: 'Luke Skywalker', - }, - { - id: '1003', - name: 'Leia Organa', - }, - ], - name: 'R2-D2', - }, - }, - networkStatus: NetworkStatus.error, - error: new ApolloError({ - graphQLErrors: [ - new GraphQLError( - 'homeWorld for character with ID 1000 could not be fetched.', - { path: ['hero', 'heroFriends', 0, 'homeWorld'] } - ), - ], - }), - }, ]); + + const [error] = renders.errors as ApolloError[]; + + expect(error).toBeInstanceOf(ApolloError); + expect(error.networkError).toBeNull(); + expect(error.graphQLErrors).toEqual([ + new GraphQLError( + 'homeWorld for character with ID 1000 could not be fetched.', + { path: ['hero', 'heroFriends', 0, 'homeWorld'] } + ), + ]); + + consoleSpy.mockRestore(); }); it('adds partial data and does not throw errors returned in incremental chunks but returns them in `error` property with errorPolicy set to `all`', async () => { From 401aaab95fd703cd0b3f54d82782fc9feea7faa4 Mon Sep 17 00:00:00 2001 From: Jerel Miller Date: Mon, 3 Jul 2023 01:10:04 -0600 Subject: [PATCH 3/5] Remove unneed duplicate setter --- src/react/cache/QueryReference.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/src/react/cache/QueryReference.ts b/src/react/cache/QueryReference.ts index 8a047d1a0e2..2da24aea881 100644 --- a/src/react/cache/QueryReference.ts +++ b/src/react/cache/QueryReference.ts @@ -201,7 +201,6 @@ export class InternalQueryReference { return; } - this.result = result; this.promise = createRejectedPromise(error); this.deliver(this.promise); } From 22b695c1bb80f0a6cde9b0be8f2573133174e081 Mon Sep 17 00:00:00 2001 From: Jerel Miller Date: Mon, 3 Jul 2023 01:15:33 -0600 Subject: [PATCH 4/5] Add changeset --- .changeset/sour-weeks-suffer.md | 9 +++++++++ 1 file changed, 9 insertions(+) create mode 100644 .changeset/sour-weeks-suffer.md diff --git a/.changeset/sour-weeks-suffer.md b/.changeset/sour-weeks-suffer.md new file mode 100644 index 00000000000..d193f144b63 --- /dev/null +++ b/.changeset/sour-weeks-suffer.md @@ -0,0 +1,9 @@ +--- +'@apollo/client': patch +--- + +Throw errors in `useSuspenseQuery` for errors returned in incremental chunks when `errorPolicy` is `none`. This provides a more consistent behavior of the `errorPolicy` in the hook. + +### Potentially breaking change + +Previously, if you issued a query with `@defer` and relied on `errorPolicy: 'none'` to set the `error` property returned from `useSuspenseQuery` when the error was returned in an incremental chunk, this error is now thrown. Switch the `errorPolicy` to `all` to avoid throwing the error and instead return it in the `error` property. From 110d982c0cb00c9ce38978e06ad597911599a0f4 Mon Sep 17 00:00:00 2001 From: Jerel Miller Date: Mon, 3 Jul 2023 10:20:06 -0600 Subject: [PATCH 5/5] Minor tweak to comment about `.catch` handler Co-authored-by: Lenz Weber-Tronic --- src/utilities/promises/decoration.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/utilities/promises/decoration.ts b/src/utilities/promises/decoration.ts index a8d5210b3f9..2b32e2e6623 100644 --- a/src/utilities/promises/decoration.ts +++ b/src/utilities/promises/decoration.ts @@ -29,9 +29,7 @@ export function createFulfilledPromise(value: TValue) { export function createRejectedPromise(reason: unknown) { const promise = Promise.reject(reason) as RejectedPromise; - // prevent unhandled error rejections since this is usually used with __use - // which synchronously reads the `status` and `reason` properties to throw - // the error. + // prevent potential edge cases leaking unhandled error rejections promise.catch(() => {}); promise.status = 'rejected';