Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Throw errors returned from incremental chunks when error policy is none #11032

Merged
merged 5 commits into from
Jul 3, 2023
Merged

Throw errors returned from incremental chunks when error policy is none #11032

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
db58fc1
Add `.catch` handler for rejected promises to prevent unhandled rejec…
jerelmiller Jul 3, 2023
5e79131
Expect errors returned in deferred chunks throw when error policy is …
jerelmiller Jul 3, 2023
401aaab
Remove unneed duplicate setter
jerelmiller Jul 3, 2023
22b695c
Add changeset
jerelmiller Jul 3, 2023
110d982
Minor tweak to comment about `.catch` handler
jerelmiller Jul 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 9 additions & 0 deletions .changeset/sour-weeks-suffer.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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.
5 changes: 1 addition & 4 deletions src/react/cache/QueryReference.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,10 +201,7 @@ export class InternalQueryReference<TData = unknown> {
return;
}

this.result = result;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not clear in this diff, but this.result is set above in this same function. This removes the unneeded duplication.

this.promise = result.data
? createFulfilledPromise(result)
: createRejectedPromise(result);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Turns out we never reached this case before! We were throwing the entire result as the error rather than the error itself, which is a bug. This change in behavior exposed this issue.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we never reached this before, why do we reach it now?
If I'm not mistaken, you didn't change any code flow above this block - and there should be plenty of cases where result.data could be undefined (getCurrentResult should return undefined whenever the data would be {}).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps I'm making too much of an assumption with that claim. The reason I feel this way because this was throwing the result instead of the error and none of my tests for handling errors caught this. I have tests for all sorts of variations of errors (error on first render, error on refetch, error on incremental chunks of @defer, etc), so I think either the code block above this happened to catch all of those, or something else was amiss. I think the result.data just happened to be defined on all cases where it made it past line 195.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, I'll be refactoring this logic anyways in likely my next PR, so expect this stuff to change and work more robustly. I had to get this change in there first to help that refactor.

this.promise = createRejectedPromise(error);
this.deliver(this.promise);
}

Expand Down
73 changes: 17 additions & 56 deletions src/react/hooks/__tests__/useSuspenseQuery.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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([
{
Expand All @@ -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 () => {
Expand Down
5 changes: 5 additions & 0 deletions src/utilities/promises/decoration.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,11 @@ export function createFulfilledPromise<TValue>(value: TValue) {
export function createRejectedPromise<TValue = unknown>(reason: unknown) {
const promise = Promise.reject(reason) as RejectedPromise<TValue>;

// 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(() => {});
jerelmiller marked this conversation as resolved.
Show resolved Hide resolved

promise.status = 'rejected';
promise.reason = reason;

Expand Down