From ae5a3a882a051efe9bc1f7f200331e6835644bb4 Mon Sep 17 00:00:00 2001 From: Lenz Weber-Tronic Date: Wed, 5 Jul 2023 15:21:34 +0200 Subject: [PATCH 01/11] keep `observer` reference out of `readMultipartBody` --- src/link/http/createHttpLink.ts | 14 +++- src/link/http/parseAndCheckHttpResponse.ts | 88 +++++++++------------- 2 files changed, 45 insertions(+), 57 deletions(-) diff --git a/src/link/http/createHttpLink.ts b/src/link/http/createHttpLink.ts index f0858cc0f17..11917384917 100644 --- a/src/link/http/createHttpLink.ts +++ b/src/link/http/createHttpLink.ts @@ -9,7 +9,7 @@ import { selectURI } from './selectURI'; import { handleError, readMultipartBody, - readJsonBody + parseAndCheckHttpResponse } from './parseAndCheckHttpResponse'; import { checkFetcher } from './checkFetcher'; import type { @@ -182,18 +182,24 @@ export const createHttpLink = (linkOptions: HttpOptions = {}) => { // removal of window.fetch, which is unlikely but not impossible. const currentFetch = preferredFetch || maybe(() => fetch) || backupFetch; + const observerNext = observer.next.bind(observer); currentFetch!(chosenURI, options) .then(response => { operation.setContext({ response }); const ctype = response.headers?.get('content-type'); if (ctype !== null && /^multipart\/mixed/i.test(ctype)) { - return readMultipartBody(response, observer); + return readMultipartBody(response, observerNext); } else { - return readJsonBody(response, operation, observer); + return parseAndCheckHttpResponse(operation)(response).then(observerNext); } }) - .catch(err => handleError(err, observer)); + .then(() => { + observer.complete(); + }) + .catch(err => { + handleError(err, observer) + }); return () => { // XXX support canceling this request diff --git a/src/link/http/parseAndCheckHttpResponse.ts b/src/link/http/parseAndCheckHttpResponse.ts index bd83842c2d7..28bb3ad13db 100644 --- a/src/link/http/parseAndCheckHttpResponse.ts +++ b/src/link/http/parseAndCheckHttpResponse.ts @@ -17,7 +17,7 @@ export type ServerParseError = Error & { export async function readMultipartBody< T extends object = Record ->(response: Response, observer: Observer) { +>(response: Response, nextValue: (value: T) => void) { if (TextDecoder === undefined) { throw new Error( "TextDecoder must be defined in the environment: please import a polyfill." @@ -74,52 +74,47 @@ export async function readMultipartBody< const body = message.slice(i); if (body) { - try { - const result = parseJsonBody(response, body); - if ( - Object.keys(result).length > 1 || - "data" in result || - "incremental" in result || - "errors" in result || - "payload" in result - ) { - if (isApolloPayloadResult(result)) { - let next = {}; - if ("payload" in result) { - next = { ...result.payload }; - } - if ("errors" in result) { - next = { - ...next, - extensions: { - ...("extensions" in next ? next.extensions : null as any), - [PROTOCOL_ERRORS_SYMBOL]: result.errors - }, - }; - } - observer.next?.(next as T); - } else { - // for the last chunk with only `hasNext: false` - // we don't need to call observer.next as there is no data/errors - observer.next?.(result); + const result = parseJsonBody(response, body); + if ( + Object.keys(result).length > 1 || + "data" in result || + "incremental" in result || + "errors" in result || + "payload" in result + ) { + if (isApolloPayloadResult(result)) { + let next = {}; + if ("payload" in result) { + next = { ...result.payload }; + } + if ("errors" in result) { + next = { + ...next, + extensions: { + ...("extensions" in next ? next.extensions : null as any), + [PROTOCOL_ERRORS_SYMBOL]: result.errors + }, + }; } - } else if ( - // If the chunk contains only a "hasNext: false", we can call - // observer.complete() immediately. - Object.keys(result).length === 1 && - "hasNext" in result && - !result.hasNext - ) { - observer.complete?.(); + nextValue(next as T); + } else { + // for the last chunk with only `hasNext: false` + // we don't need to call observer.next as there is no data/errors + nextValue(result); } - } catch (err) { - handleError(err, observer); + } else if ( + // If the chunk contains only a "hasNext: false", we can call + // observer.complete() immediately. + Object.keys(result).length === 1 && + "hasNext" in result && + !result.hasNext + ) { + return; } } bi = buffer.indexOf(boundary); } } - observer.complete?.(); } export function parseHeaders(headerText: string): Record { @@ -206,19 +201,6 @@ export function handleError(err: any, observer: Observer) { observer.error?.(err); } -export function readJsonBody>( - response: Response, - operation: Operation, - observer: Observer -) { - parseAndCheckHttpResponse(operation)(response) - .then((result) => { - observer.next?.(result); - observer.complete?.(); - }) - .catch((err) => handleError(err, observer)); -} - export function parseAndCheckHttpResponse(operations: Operation | Operation[]) { return (response: Response) => response From fd3a79bf15161d1fe674be2f1b649b01e057c817 Mon Sep 17 00:00:00 2001 From: Lenz Weber-Tronic Date: Wed, 5 Jul 2023 15:23:42 +0200 Subject: [PATCH 02/11] reset controller reference after request --- src/link/batch-http/batchHttpLink.ts | 6 ++++-- src/link/http/createHttpLink.ts | 6 ++++-- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/src/link/batch-http/batchHttpLink.ts b/src/link/batch-http/batchHttpLink.ts index fbc746af67c..7f58316ab92 100644 --- a/src/link/batch-http/batchHttpLink.ts +++ b/src/link/batch-http/batchHttpLink.ts @@ -157,10 +157,10 @@ export class BatchHttpLink extends ApolloLink { return fromError(parseError); } - let controller: any; + let controller: AbortController | undefined; if (!(options as any).signal) { const { controller: _controller, signal } = createSignalIfSupported(); - controller = _controller; + controller = _controller as AbortController; if (controller) (options as any).signal = signal; } @@ -173,12 +173,14 @@ export class BatchHttpLink extends ApolloLink { }) .then(parseAndCheckHttpResponse(operations)) .then(result => { + controller = undefined; // we have data and can send it to back up the link chain observer.next(result); observer.complete(); return result; }) .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 diff --git a/src/link/http/createHttpLink.ts b/src/link/http/createHttpLink.ts index 11917384917..38dfe95985d 100644 --- a/src/link/http/createHttpLink.ts +++ b/src/link/http/createHttpLink.ts @@ -119,10 +119,10 @@ export const createHttpLink = (linkOptions: HttpOptions = {}) => { body.variables = filterOperationVariables(body.variables, operation.query); } - let controller: any; + let controller: AbortController | undefined; if (!(options as any).signal) { const { controller: _controller, signal } = createSignalIfSupported(); - controller = _controller; + controller = _controller as AbortController; if (controller) (options as any).signal = signal; } @@ -195,9 +195,11 @@ export const createHttpLink = (linkOptions: HttpOptions = {}) => { } }) .then(() => { + controller = undefined; observer.complete(); }) .catch(err => { + controller = undefined; handleError(err, observer) }); From bf6c2b6f392857c91bd683c42d91aeb3c1523e97 Mon Sep 17 00:00:00 2001 From: Lenz Weber-Tronic Date: Wed, 5 Jul 2023 17:22:49 +0200 Subject: [PATCH 03/11] tests --- src/link/http/__tests__/HttpLink.ts | 108 ++++++++++++++++++++-------- 1 file changed, 79 insertions(+), 29 deletions(-) diff --git a/src/link/http/__tests__/HttpLink.ts b/src/link/http/__tests__/HttpLink.ts index f0525edc4ca..bc467641950 100644 --- a/src/link/http/__tests__/HttpLink.ts +++ b/src/link/http/__tests__/HttpLink.ts @@ -13,7 +13,7 @@ import { HttpLink } from '../HttpLink'; import { createHttpLink } from '../createHttpLink'; import { ClientParseError } from '../serializeFetchParameter'; import { ServerParseError } from '../parseAndCheckHttpResponse'; -import { ServerError } from '../../..'; +import { FetchResult, ServerError } from '../../..'; import { voidFetchDuringEachTest } from './helpers'; import { itAsync } from '../../../testing'; @@ -1325,46 +1325,96 @@ describe('HttpLink', () => { }), ); }); - itAsync('supports being cancelled and does not throw', (resolve, reject) => { - let called = false; - class AbortController { + + describe('AbortController', () => { + let aborted = false; + let created = false; + beforeEach(() => { created = aborted = false; }) + + class AbortControllerMock { + constructor() { + created = true; + } signal: {}; abort = () => { - called = true; + aborted = true; }; } - (global as any).AbortController = AbortController; - - fetch.mockReturnValueOnce(Promise.resolve({ text })); - text.mockReturnValueOnce( - Promise.resolve('{ "data": { "hello": "world" } }'), - ); - - const link = createHttpLink({ uri: 'data', fetch: fetch as any }); - - const sub = execute(link, { query: sampleQuery }).subscribe({ - next: result => { - reject('result should not have been called'); + const originalAbortController = globalThis.AbortController; + beforeAll(() => { + globalThis.AbortController = AbortControllerMock as any; + }) + afterAll(() => { + globalThis.AbortController = originalAbortController; + }) + + beforeEach( () => { + fetch.mockResolvedValueOnce({ text }); + text.mockResolvedValueOnce('{ "data": { "hello": "world" } }'); + }) + afterEach(() => { + fetch.mockReset(); + text.mockReset(); + }) + + const failingObserver: Observer = { + next: () => { + fail('result should not have been called'); }, error: e => { - reject(e); + fail(e); }, complete: () => { - reject('complete should not have been called'); + fail('complete should not have been called'); }, + } + + it('unsubscribing cancels internally created AbortController', () => { + const link = createHttpLink({ uri: 'data', fetch: fetch as any }); + + const sub = execute(link, { query: sampleQuery }).subscribe(failingObserver); + sub.unsubscribe(); + + expect(created).toBe(true); + expect(aborted).toBe(true); }); - sub.unsubscribe(); - setTimeout( - makeCallback(resolve, reject, () => { - delete (global as any).AbortController; - expect(called).toBe(true); - fetch.mockReset(); - text.mockReset(); - }), - 150, - ); + it('passing a signal in means no AbortController is created internally', () => { + const link = createHttpLink({ uri: 'data', fetch: fetch as any, fetchOptions: { signal: {} } }); + + const sub = execute(link, { query: sampleQuery } ).subscribe(failingObserver); + sub.unsubscribe(); + + expect(created).toBe(false); + expect(aborted).toBe(false); + }); + + it('resolving fetch does not cause the AbortController to be aborted', async () => { + // (the request is already finished at that point) + const link = createHttpLink({ uri: 'data', fetch: fetch as any }); + + await new Promise(resolve => execute(link, { query: sampleQuery }).subscribe({ + complete: resolve + })); + + expect(created).toBe(true); + expect(aborted).toBe(false); + }); + + it('throwing an error from fetch does not cause the AbortController to be aborted', async () => { + fetch.mockReset(); + fetch.mockRejectedValueOnce("This is an error!") + // the request would be closed by the browser in the case of an error anyways + const link = createHttpLink({ uri: 'data', fetch: fetch as any }); + + await new Promise(resolve => execute(link, { query: sampleQuery }).subscribe({ + error: resolve + })); + + expect(created).toBe(true); + expect(aborted).toBe(false); + }); }); const body = '{'; From 0b361950a55ecc730866c4ba9f58cd17a079eee6 Mon Sep 17 00:00:00 2001 From: Lenz Weber-Tronic Date: Wed, 5 Jul 2023 17:23:11 +0200 Subject: [PATCH 04/11] inline simplified version of `createSignalIfSupported` --- src/link/batch-http/batchHttpLink.ts | 8 +++----- src/link/http/createHttpLink.ts | 8 +++----- src/link/http/createSignalIfSupported.ts | 5 +++++ 3 files changed, 11 insertions(+), 10 deletions(-) diff --git a/src/link/batch-http/batchHttpLink.ts b/src/link/batch-http/batchHttpLink.ts index 7f58316ab92..6da53c06a27 100644 --- a/src/link/batch-http/batchHttpLink.ts +++ b/src/link/batch-http/batchHttpLink.ts @@ -16,7 +16,6 @@ import { selectHttpOptionsAndBodyInternal, defaultPrinter, fallbackHttpConfig, - createSignalIfSupported, } from '../http'; import { BatchLink } from '../batch'; import { filterOperationVariables } from "../utils/filterOperationVariables"; @@ -158,10 +157,9 @@ export class BatchHttpLink extends ApolloLink { } let controller: AbortController | undefined; - if (!(options as any).signal) { - const { controller: _controller, signal } = createSignalIfSupported(); - controller = _controller as AbortController; - if (controller) (options as any).signal = signal; + if (!options.signal && typeof AbortController !== 'undefined') { + controller = new AbortController(); + options.signal = controller.signal; } return new Observable(observer => { diff --git a/src/link/http/createHttpLink.ts b/src/link/http/createHttpLink.ts index 38dfe95985d..e2d0724b5a3 100644 --- a/src/link/http/createHttpLink.ts +++ b/src/link/http/createHttpLink.ts @@ -20,7 +20,6 @@ import { defaultPrinter, fallbackHttpConfig } from './selectHttpOptionsAndBody'; -import { createSignalIfSupported } from './createSignalIfSupported'; import { rewriteURIForGET } from './rewriteURIForGET'; import { fromError, filterOperationVariables } from '../utils'; import { @@ -120,10 +119,9 @@ export const createHttpLink = (linkOptions: HttpOptions = {}) => { } let controller: AbortController | undefined; - if (!(options as any).signal) { - const { controller: _controller, signal } = createSignalIfSupported(); - controller = _controller as AbortController; - if (controller) (options as any).signal = signal; + if (!options.signal && typeof AbortController !== 'undefined') { + controller = new AbortController(); + options.signal = controller.signal; } // If requested, set method to GET if there are no mutations. diff --git a/src/link/http/createSignalIfSupported.ts b/src/link/http/createSignalIfSupported.ts index 560e8a92645..a4f4260bdea 100644 --- a/src/link/http/createSignalIfSupported.ts +++ b/src/link/http/createSignalIfSupported.ts @@ -1,3 +1,8 @@ +/** + * @deprecated + * This is not used internally any more and will be removed in + * the next major version of Apollo Client. + */ export const createSignalIfSupported = () => { if (typeof AbortController === 'undefined') return { controller: false, signal: false }; From 3d9fddbcfca7352fde2b292abeac9cf50eba9af2 Mon Sep 17 00:00:00 2001 From: Lenz Weber-Tronic Date: Wed, 5 Jul 2023 17:27:42 +0200 Subject: [PATCH 05/11] changeset --- .changeset/smooth-goats-cheat.md | 13 +++++++++++++ 1 file changed, 13 insertions(+) create mode 100644 .changeset/smooth-goats-cheat.md diff --git a/.changeset/smooth-goats-cheat.md b/.changeset/smooth-goats-cheat.md new file mode 100644 index 00000000000..ffda1fc90d9 --- /dev/null +++ b/.changeset/smooth-goats-cheat.md @@ -0,0 +1,13 @@ +--- +'@apollo/client': minor +--- + +`HttpLink`/`BatchHttpLink`: Abort AbortController signal more granularly. +Before this change, when `HttpLink`/`BatchHttpLink` created an `AbortController` +internally, that would always be `.abort`ed after the request was finished in any +way. +This could cause issues with Sentry Session Replay and Next.js App Router Cache +invalidations, which just replayed the fetch with the same options - including the +cancelled `AbortSignal`. +With this change, the `AbortController` will only be `.abort()`ed by outside events, +not as a consequence of the request finishing or erroring. From f9f1022164878acf681d20154a6032a39ebb16cc Mon Sep 17 00:00:00 2001 From: Lenz Weber-Tronic Date: Thu, 6 Jul 2023 12:25:46 +0200 Subject: [PATCH 06/11] use correct type in `handleError`, remove optional chaining --- src/link/http/parseAndCheckHttpResponse.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/link/http/parseAndCheckHttpResponse.ts b/src/link/http/parseAndCheckHttpResponse.ts index 28bb3ad13db..9413815347d 100644 --- a/src/link/http/parseAndCheckHttpResponse.ts +++ b/src/link/http/parseAndCheckHttpResponse.ts @@ -2,10 +2,10 @@ import { responseIterator } from "./responseIterator"; import type { Operation } from "../core"; import { throwServerError } from "../utils"; import { PROTOCOL_ERRORS_SYMBOL } from '../../errors'; -import type { Observer } from "../../utilities"; import { isApolloPayloadResult } from '../../utilities/common/incrementalResult'; +import type { SubscriptionObserver } from "zen-observable-ts"; const { hasOwnProperty } = Object.prototype; @@ -160,7 +160,7 @@ export function parseJsonBody(response: Response, bodyText: string): T { } } -export function handleError(err: any, observer: Observer) { +export function handleError(err: any, observer: SubscriptionObserver) { 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 @@ -195,10 +195,10 @@ export function handleError(err: any, observer: Observer) { // status code of above would be a 401 // in the UI you want to show data where you can, errors as data where you can // and use correct http status codes - observer.next?.(err.result); + observer.next(err.result); } - observer.error?.(err); + observer.error(err); } export function parseAndCheckHttpResponse(operations: Operation | Operation[]) { From 766964be8af7f7c0762cccca00849f8bec9c3419 Mon Sep 17 00:00:00 2001 From: Lenz Weber-Tronic Date: Thu, 6 Jul 2023 12:48:41 +0200 Subject: [PATCH 07/11] restructure tests as suggested in PR review --- src/link/http/__tests__/HttpLink.ts | 86 ++++++++++++++++------------- 1 file changed, 48 insertions(+), 38 deletions(-) diff --git a/src/link/http/__tests__/HttpLink.ts b/src/link/http/__tests__/HttpLink.ts index bc467641950..f4ff8854ac3 100644 --- a/src/link/http/__tests__/HttpLink.ts +++ b/src/link/http/__tests__/HttpLink.ts @@ -1326,37 +1326,25 @@ describe('HttpLink', () => { ); }); - describe('AbortController', () => { - let aborted = false; - let created = false; - beforeEach(() => { created = aborted = false; }) - - class AbortControllerMock { - constructor() { - created = true; + describe.only('AbortController', () => { + function mockGlobalAbortController() { + const mockStatus = { + aborted: false, + created: false + } + class AbortControllerMock { + constructor() { + mockStatus.created = true; + } + signal: {}; + abort = () => { + mockStatus.aborted = true; + }; } - signal: {}; - abort = () => { - aborted = true; - }; - } - const originalAbortController = globalThis.AbortController; - beforeAll(() => { globalThis.AbortController = AbortControllerMock as any; - }) - afterAll(() => { - globalThis.AbortController = originalAbortController; - }) - - beforeEach( () => { - fetch.mockResolvedValueOnce({ text }); - text.mockResolvedValueOnce('{ "data": { "hello": "world" } }'); - }) - afterEach(() => { - fetch.mockReset(); - text.mockReset(); - }) + return mockStatus; + } const failingObserver: Observer = { next: () => { @@ -1370,27 +1358,49 @@ describe('HttpLink', () => { }, } + // clean up between tests + const originalAbortController = globalThis.AbortController; + afterEach(() => { + globalThis.AbortController = originalAbortController; + fetch.mockReset(); + text.mockReset(); + }) + + it('unsubscribing cancels internally created AbortController', () => { + const mock = mockGlobalAbortController(); + fetch.mockResolvedValueOnce({ text }); + text.mockResolvedValueOnce('{ "data": { "hello": "world" } }'); + const link = createHttpLink({ uri: 'data', fetch: fetch as any }); const sub = execute(link, { query: sampleQuery }).subscribe(failingObserver); sub.unsubscribe(); - expect(created).toBe(true); - expect(aborted).toBe(true); + expect(mock.created).toBe(true); + expect(mock.aborted).toBe(true); }); it('passing a signal in means no AbortController is created internally', () => { - const link = createHttpLink({ uri: 'data', fetch: fetch as any, fetchOptions: { signal: {} } }); + const realAbortController = new AbortController(); + const mock = mockGlobalAbortController(); + fetch.mockResolvedValueOnce({ text }); + text.mockResolvedValueOnce('{ "data": { "hello": "world" } }'); + + const link = createHttpLink({ uri: 'data', fetch: fetch as any, fetchOptions: { signal: realAbortController.signal } }); const sub = execute(link, { query: sampleQuery } ).subscribe(failingObserver); sub.unsubscribe(); - expect(created).toBe(false); - expect(aborted).toBe(false); + expect(mock.created).toBe(false); + expect(mock.aborted).toBe(false); }); it('resolving fetch does not cause the AbortController to be aborted', async () => { + const mock = mockGlobalAbortController(); + fetch.mockResolvedValueOnce({ text }); + text.mockResolvedValueOnce('{ "data": { "hello": "world" } }'); + // (the request is already finished at that point) const link = createHttpLink({ uri: 'data', fetch: fetch as any }); @@ -1398,12 +1408,12 @@ describe('HttpLink', () => { complete: resolve })); - expect(created).toBe(true); - expect(aborted).toBe(false); + expect(mock.created).toBe(true); + expect(mock.aborted).toBe(false); }); it('throwing an error from fetch does not cause the AbortController to be aborted', async () => { - fetch.mockReset(); + const mock = mockGlobalAbortController(); fetch.mockRejectedValueOnce("This is an error!") // the request would be closed by the browser in the case of an error anyways const link = createHttpLink({ uri: 'data', fetch: fetch as any }); @@ -1412,8 +1422,8 @@ describe('HttpLink', () => { error: resolve })); - expect(created).toBe(true); - expect(aborted).toBe(false); + expect(mock.created).toBe(true); + expect(mock.aborted).toBe(false); }); }); From 0723cce55bda8e5d9df5a664aea16a8f0f3eeaee Mon Sep 17 00:00:00 2001 From: Lenz Weber-Tronic Date: Thu, 6 Jul 2023 12:49:13 +0200 Subject: [PATCH 08/11] suggested wording change --- src/link/http/__tests__/HttpLink.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/link/http/__tests__/HttpLink.ts b/src/link/http/__tests__/HttpLink.ts index f4ff8854ac3..d5717845d1b 100644 --- a/src/link/http/__tests__/HttpLink.ts +++ b/src/link/http/__tests__/HttpLink.ts @@ -1367,7 +1367,7 @@ describe('HttpLink', () => { }) - it('unsubscribing cancels internally created AbortController', () => { + it('unsubscribing cancels internally created AbortController when request hasn\'t completed', () => { const mock = mockGlobalAbortController(); fetch.mockResolvedValueOnce({ text }); text.mockResolvedValueOnce('{ "data": { "hello": "world" } }'); From 87675271846ff070adc958aa854aab4fb36b6c1f Mon Sep 17 00:00:00 2001 From: Lenz Weber-Tronic Date: Tue, 11 Jul 2023 13:16:52 +0200 Subject: [PATCH 09/11] Update .changeset/smooth-goats-cheat.md Co-authored-by: Jerel Miller --- .changeset/smooth-goats-cheat.md | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/.changeset/smooth-goats-cheat.md b/.changeset/smooth-goats-cheat.md index ffda1fc90d9..f1b7beb1c80 100644 --- a/.changeset/smooth-goats-cheat.md +++ b/.changeset/smooth-goats-cheat.md @@ -2,12 +2,9 @@ '@apollo/client': minor --- -`HttpLink`/`BatchHttpLink`: Abort AbortController signal more granularly. +`HttpLink`/`BatchHttpLink`: Abort the `AbortController` signal more granularly. Before this change, when `HttpLink`/`BatchHttpLink` created an `AbortController` -internally, that would always be `.abort`ed after the request was finished in any -way. -This could cause issues with Sentry Session Replay and Next.js App Router Cache -invalidations, which just replayed the fetch with the same options - including the -cancelled `AbortSignal`. +internally, the signal would always be `.abort`ed after the request was completed. This could cause issues with Sentry Session Replay and Next.js App Router Cache invalidations, which just replayed the fetch with the same options - including the cancelled `AbortSignal`. + With this change, the `AbortController` will only be `.abort()`ed by outside events, -not as a consequence of the request finishing or erroring. +not as a consequence of the request completing. From bf36ad4567b190ef0da1148246b4007561e1591f Mon Sep 17 00:00:00 2001 From: Lenz Weber-Tronic Date: Tue, 11 Jul 2023 13:56:00 +0200 Subject: [PATCH 10/11] PR feedback --- src/link/http/__tests__/HttpLink.ts | 65 +++++++++++++---------------- 1 file changed, 30 insertions(+), 35 deletions(-) diff --git a/src/link/http/__tests__/HttpLink.ts b/src/link/http/__tests__/HttpLink.ts index d5717845d1b..8eb957eebee 100644 --- a/src/link/http/__tests__/HttpLink.ts +++ b/src/link/http/__tests__/HttpLink.ts @@ -1326,24 +1326,24 @@ describe('HttpLink', () => { ); }); - describe.only('AbortController', () => { - function mockGlobalAbortController() { - const mockStatus = { - aborted: false, - created: false - } + describe('AbortController', () => { + const originalAbortController = globalThis.AbortController; + afterEach(() => { + globalThis.AbortController = originalAbortController; + }); + + function trackGlobalAbortControllers() { + const instances: AbortController[] = [] class AbortControllerMock { constructor() { - mockStatus.created = true; + const instance = new originalAbortController() + instances.push(instance) + return instance } - signal: {}; - abort = () => { - mockStatus.aborted = true; - }; } globalThis.AbortController = AbortControllerMock as any; - return mockStatus; + return instances; } const failingObserver: Observer = { @@ -1358,17 +1358,13 @@ describe('HttpLink', () => { }, } - // clean up between tests - const originalAbortController = globalThis.AbortController; - afterEach(() => { - globalThis.AbortController = originalAbortController; + beforeEach(() => { fetch.mockReset(); text.mockReset(); - }) - + }); - it('unsubscribing cancels internally created AbortController when request hasn\'t completed', () => { - const mock = mockGlobalAbortController(); + it("aborts the request when unsubscribing before the request has completed", () => { + const abortControllers = trackGlobalAbortControllers(); fetch.mockResolvedValueOnce({ text }); text.mockResolvedValueOnce('{ "data": { "hello": "world" } }'); @@ -1377,27 +1373,26 @@ describe('HttpLink', () => { const sub = execute(link, { query: sampleQuery }).subscribe(failingObserver); sub.unsubscribe(); - expect(mock.created).toBe(true); - expect(mock.aborted).toBe(true); + expect(abortControllers.length).toBe(1); + expect(abortControllers[0].signal.aborted).toBe(true); }); - it('passing a signal in means no AbortController is created internally', () => { - const realAbortController = new AbortController(); - const mock = mockGlobalAbortController(); + it('a passed-in signal will be forwarded to the `fetch` call and not be overwritten by an internally-created one', () => { + const externalAbortController = new AbortController(); fetch.mockResolvedValueOnce({ text }); text.mockResolvedValueOnce('{ "data": { "hello": "world" } }'); - const link = createHttpLink({ uri: 'data', fetch: fetch as any, fetchOptions: { signal: realAbortController.signal } }); + const link = createHttpLink({ uri: 'data', fetch: fetch as any, fetchOptions: { signal: externalAbortController.signal } }); const sub = execute(link, { query: sampleQuery } ).subscribe(failingObserver); sub.unsubscribe(); - expect(mock.created).toBe(false); - expect(mock.aborted).toBe(false); + expect(fetch.mock.calls.length).toBe(1); + expect(fetch.mock.calls[0][1]).toEqual(expect.objectContaining({ signal: externalAbortController.signal })) }); it('resolving fetch does not cause the AbortController to be aborted', async () => { - const mock = mockGlobalAbortController(); + const abortControllers = trackGlobalAbortControllers(); fetch.mockResolvedValueOnce({ text }); text.mockResolvedValueOnce('{ "data": { "hello": "world" } }'); @@ -1408,12 +1403,12 @@ describe('HttpLink', () => { complete: resolve })); - expect(mock.created).toBe(true); - expect(mock.aborted).toBe(false); + expect(abortControllers.length).toBe(1); + expect(abortControllers[0].signal.aborted).toBe(false); }); - it('throwing an error from fetch does not cause the AbortController to be aborted', async () => { - const mock = mockGlobalAbortController(); + it('an unsuccessful fetch does not cause the AbortController to be aborted', async () => { + const abortControllers = trackGlobalAbortControllers(); fetch.mockRejectedValueOnce("This is an error!") // the request would be closed by the browser in the case of an error anyways const link = createHttpLink({ uri: 'data', fetch: fetch as any }); @@ -1422,8 +1417,8 @@ describe('HttpLink', () => { error: resolve })); - expect(mock.created).toBe(true); - expect(mock.aborted).toBe(false); + expect(abortControllers.length).toBe(1); + expect(abortControllers[0].signal.aborted).toBe(false); }); }); From 13b2ca93611432458664a8ef15ba73eec247c5cf Mon Sep 17 00:00:00 2001 From: Lenz Weber-Tronic Date: Tue, 11 Jul 2023 14:57:01 +0200 Subject: [PATCH 11/11] move `fetch` mock into `mockFetch` function --- .../batch-http/__tests__/batchHttpLink.ts | 99 ++++++++++++++++++- src/link/http/__tests__/HttpLink.ts | 18 ++-- 2 files changed, 107 insertions(+), 10 deletions(-) diff --git a/src/link/batch-http/__tests__/batchHttpLink.ts b/src/link/batch-http/__tests__/batchHttpLink.ts index 208ae6f9b43..471aaea5d60 100644 --- a/src/link/batch-http/__tests__/batchHttpLink.ts +++ b/src/link/batch-http/__tests__/batchHttpLink.ts @@ -4,9 +4,10 @@ import { ASTNode, print, stripIgnoredCharacters } from 'graphql'; import { ApolloLink } from '../../core/ApolloLink'; import { execute } from '../../core/execute'; -import { Observable } from '../../../utilities/observables/Observable'; +import { Observable, Observer } from '../../../utilities/observables/Observable'; import { BatchHttpLink } from '../batchHttpLink'; import { itAsync } from '../../../testing'; +import { FetchResult } from '../../core'; const sampleQuery = gql` query SampleQuery { @@ -1029,4 +1030,100 @@ describe('SharedHttpTest', () => { new Error('BatchHttpLink: Trying to send a client-only query to the server. To send to the server, ensure a non-client field is added to the query or enable the `transformOptions.removeClientFields` option.') ); }); + + describe('AbortController', () => { + const originalAbortController = globalThis.AbortController; + afterEach(() => { + globalThis.AbortController = originalAbortController; + }); + + function trackGlobalAbortControllers() { + const instances: AbortController[] = [] + class AbortControllerMock { + constructor() { + const instance = new originalAbortController() + instances.push(instance) + return instance + } + } + + globalThis.AbortController = AbortControllerMock as any; + return instances; + } + + const failingObserver: Observer = { + next: () => { + fail('result should not have been called'); + }, + error: e => { + fail(e); + }, + complete: () => { + fail('complete should not have been called'); + }, + } + + function mockFetch() { + const text = jest.fn(async () => '{}'); + const fetch = jest.fn(async (uri, options) => ({ text })); + return { text, fetch } + } + + it("aborts the request when unsubscribing before the request has completed", () => { + const { fetch } = mockFetch(); + const abortControllers = trackGlobalAbortControllers(); + + const link = createHttpLink({ uri: 'data', fetch: fetch as any }); + + const sub = execute(link, { query: sampleQuery }).subscribe(failingObserver); + sub.unsubscribe(); + + expect(abortControllers.length).toBe(1); + expect(abortControllers[0].signal.aborted).toBe(true); + }); + + it('a passed-in signal will be forwarded to the `fetch` call and not be overwritten by an internally-created one', () => { + const { fetch } = mockFetch(); + const externalAbortController = new AbortController(); + + const link = createHttpLink({ uri: 'data', fetch: fetch as any, fetchOptions: { signal: externalAbortController.signal } }); + + const sub = execute(link, { query: sampleQuery } ).subscribe(failingObserver); + sub.unsubscribe(); + + expect(fetch.mock.calls.length).toBe(1); + expect(fetch.mock.calls[0][1]).toEqual(expect.objectContaining({ signal: externalAbortController.signal })) + }); + + it('resolving fetch does not cause the AbortController to be aborted', async () => { + const { text, fetch } = mockFetch(); + const abortControllers = trackGlobalAbortControllers(); + text.mockResolvedValueOnce('{ "data": { "hello": "world" } }'); + + // (the request is already finished at that point) + const link = createHttpLink({ uri: 'data', fetch: fetch as any }); + + await new Promise(resolve => execute(link, { query: sampleQuery }).subscribe({ + complete: resolve + })); + + expect(abortControllers.length).toBe(1); + expect(abortControllers[0].signal.aborted).toBe(false); + }); + + it('an unsuccessful fetch does not cause the AbortController to be aborted', async () => { + const { fetch } = mockFetch(); + const abortControllers = trackGlobalAbortControllers(); + fetch.mockRejectedValueOnce("This is an error!") + // the request would be closed by the browser in the case of an error anyways + const link = createHttpLink({ uri: 'data', fetch: fetch as any }); + + await new Promise(resolve => execute(link, { query: sampleQuery }).subscribe({ + error: resolve + })); + + expect(abortControllers.length).toBe(1); + expect(abortControllers[0].signal.aborted).toBe(false); + }); + }); }); diff --git a/src/link/http/__tests__/HttpLink.ts b/src/link/http/__tests__/HttpLink.ts index 8eb957eebee..1941f8d4e58 100644 --- a/src/link/http/__tests__/HttpLink.ts +++ b/src/link/http/__tests__/HttpLink.ts @@ -1358,15 +1358,15 @@ describe('HttpLink', () => { }, } - beforeEach(() => { - fetch.mockReset(); - text.mockReset(); - }); + function mockFetch() { + const text = jest.fn(async () => '{}'); + const fetch = jest.fn(async (uri, options) => ({ text })); + return { text, fetch } + } it("aborts the request when unsubscribing before the request has completed", () => { + const { fetch } = mockFetch(); const abortControllers = trackGlobalAbortControllers(); - fetch.mockResolvedValueOnce({ text }); - text.mockResolvedValueOnce('{ "data": { "hello": "world" } }'); const link = createHttpLink({ uri: 'data', fetch: fetch as any }); @@ -1378,9 +1378,8 @@ describe('HttpLink', () => { }); it('a passed-in signal will be forwarded to the `fetch` call and not be overwritten by an internally-created one', () => { + const { fetch } = mockFetch(); const externalAbortController = new AbortController(); - fetch.mockResolvedValueOnce({ text }); - text.mockResolvedValueOnce('{ "data": { "hello": "world" } }'); const link = createHttpLink({ uri: 'data', fetch: fetch as any, fetchOptions: { signal: externalAbortController.signal } }); @@ -1392,8 +1391,8 @@ describe('HttpLink', () => { }); it('resolving fetch does not cause the AbortController to be aborted', async () => { + const { text, fetch } = mockFetch(); const abortControllers = trackGlobalAbortControllers(); - fetch.mockResolvedValueOnce({ text }); text.mockResolvedValueOnce('{ "data": { "hello": "world" } }'); // (the request is already finished at that point) @@ -1408,6 +1407,7 @@ describe('HttpLink', () => { }); it('an unsuccessful fetch does not cause the AbortController to be aborted', async () => { + const { fetch } = mockFetch(); const abortControllers = trackGlobalAbortControllers(); fetch.mockRejectedValueOnce("This is an error!") // the request would be closed by the browser in the case of an error anyways