Skip to content

Commit

Permalink
(Batch)HttpLink: Propagate AbortErrors with user-provided signals (#1…
Browse files Browse the repository at this point in the history
…1058)

Propagate `AbortError`s to the user when a user-provided `signal` is passed to the link. Previously, these links would swallow all `AbortErrors`, potentially causing queries and mutations to never resolve. As a result of this change, users are now expected to handle `AbortError`s when passing in a user-provided `signal`.
  • Loading branch information
phryneas authored Jul 13, 2023
1 parent c0ca707 commit 89bf33c
Show file tree
Hide file tree
Showing 6 changed files with 50 additions and 5 deletions.
5 changes: 5 additions & 0 deletions .changeset/polite-pianos-kiss.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@apollo/client': minor
---

(Batch)HttpLink: Propagate `AbortError`s to the user when a user-provided `signal` is passed to the link. Previously, these links would swallow all `AbortErrors`, potentially causing queries and mutations to never resolve. As a result of this change, users are now expected to handle `AbortError`s when passing in a user-provided `signal`.
7 changes: 7 additions & 0 deletions src/config/jest/setup.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,3 +9,10 @@ gql.disableFragmentWarnings();
process.on('unhandledRejection', () => {});

loadErrorMessageHandler();

function fail(reason = "fail was called in a test.") {
expect(reason).toBe(undefined);
}

// @ts-ignore
globalThis.fail = fail;
16 changes: 15 additions & 1 deletion src/link/batch-http/__tests__/batchHttpLink.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1064,7 +1064,7 @@ describe('SharedHttpTest', () => {
}

function mockFetch() {
const text = jest.fn(async () => '{}');
const text = jest.fn(async () => '{ "data": { "stub": { "id": "foo" } } }');
const fetch = jest.fn(async (uri, options) => ({ text }));
return { text, fetch }
}
Expand Down Expand Up @@ -1095,6 +1095,20 @@ describe('SharedHttpTest', () => {
expect(fetch.mock.calls[0][1]).toEqual(expect.objectContaining({ signal: externalAbortController.signal }))
});

it('aborting the internal signal will not cause an error', async () => {
try {
fetchMock.restore();
fetchMock.postOnce('data', async () => '{ "data": { "stub": { "id": "foo" } } }');
const abortControllers = trackGlobalAbortControllers();

const link = createHttpLink({ uri: '/data' });
execute(link, { query: sampleQuery } ).subscribe(failingObserver);
abortControllers[0].abort();
} finally {
fetchMock.restore();
}
});

it('resolving fetch does not cause the AbortController to be aborted', async () => {
const { text, fetch } = mockFetch();
const abortControllers = trackGlobalAbortControllers();
Expand Down
2 changes: 0 additions & 2 deletions src/link/batch-http/batchHttpLink.ts
Original file line number Diff line number Diff line change
Expand Up @@ -179,8 +179,6 @@ export class BatchHttpLink extends ApolloLink {
})
.catch(err => {
controller = undefined;
// fetch was cancelled so its already been cleaned up in the unsubscribe
if (err.name === 'AbortError') return;
// if it is a network error, BUT there is graphql result info
// fire the next observer before calling error
// this gives apollo-client (and react-apollo) the `graphqlErrors` and `networkErrors`
Expand Down
24 changes: 23 additions & 1 deletion src/link/http/__tests__/HttpLink.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1359,7 +1359,7 @@ describe('HttpLink', () => {
}

function mockFetch() {
const text = jest.fn(async () => '{}');
const text = jest.fn(async () => '{ "data": { "stub": { "id": "foo" } } }');
const fetch = jest.fn(async (uri, options) => ({ text }));
return { text, fetch }
}
Expand Down Expand Up @@ -1390,6 +1390,28 @@ describe('HttpLink', () => {
expect(fetch.mock.calls[0][1]).toEqual(expect.objectContaining({ signal: externalAbortController.signal }))
});

it('a passed-in signal that is cancelled will fail the observable with an `AbortError`', async () => {
try {
fetchMock.restore();
fetchMock.postOnce('data', async () => '{ "data": { "stub": { "id": "foo" } } }');

const externalAbortController = new AbortController();

const link = createHttpLink({ uri: '/data', fetchOptions: { signal: externalAbortController.signal } });

const error = await new Promise<Error>(resolve => {
execute(link, { query: sampleQuery } ).subscribe({
...failingObserver,
error: resolve,
});
externalAbortController.abort();
});
expect(error.name).toBe("AbortError")
} finally {
fetchMock.restore();
}
});

it('resolving fetch does not cause the AbortController to be aborted', async () => {
const { text, fetch } = mockFetch();
const abortControllers = trackGlobalAbortControllers();
Expand Down
1 change: 0 additions & 1 deletion src/link/http/parseAndCheckHttpResponse.ts
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,6 @@ export function parseJsonBody<T>(response: Response, bodyText: string): T {
}

export function handleError(err: any, observer: SubscriptionObserver<any>) {
if (err.name === "AbortError") return;
// if it is a network error, BUT there is graphql result info fire
// the next observer before calling error this gives apollo-client
// (and react-apollo) the `graphqlErrors` and `networkErrors` to
Expand Down

0 comments on commit 89bf33c

Please sign in to comment.