From c2657a868e1c079d50f63ce5c0850572d491238a Mon Sep 17 00:00:00 2001 From: Bram Gotink Date: Fri, 1 Nov 2019 02:13:27 +0100 Subject: [PATCH] fix(fromFetch): passing already aborted signal to init aborts fetch * fix(fromFetch): copy init object instead of modifying it Modifying the object can lead to problems if the init object is reused in multiple calls to fromFetch. * fix(fromFetch): cancel fetch if init.signal is already aborted --- spec/observables/dom/fetch-spec.ts | 38 +++++++++++++++++++++++++--- src/internal/observable/dom/fetch.ts | 18 ++++++++----- 2 files changed, 45 insertions(+), 11 deletions(-) diff --git a/spec/observables/dom/fetch-spec.ts b/spec/observables/dom/fetch-spec.ts index 3722705abc..b8346a4f39 100644 --- a/spec/observables/dom/fetch-spec.ts +++ b/spec/observables/dom/fetch-spec.ts @@ -10,11 +10,15 @@ function mockFetchImpl(input: string | Request, init?: RequestInit): Promise((resolve, reject) => { if (init.signal) { + if (init.signal.aborted) { + reject(new MockDOMException()); + return; + } init.signal.addEventListener('abort', () => { reject(new MockDOMException()); }); } - return Promise.resolve(null).then(() => { + Promise.resolve(null).then(() => { resolve((mockFetchImpl as any).respondWith); }); }); @@ -173,13 +177,23 @@ describe('fromFetch', () => { }); it('should allow passing of init object', done => { - const myInit = {}; - const fetch$ = fromFetch('/foo', myInit); + const fetch$ = fromFetch('/foo', {method: 'HEAD'}); + fetch$.subscribe({ + error: done, + complete: done, + }); + expect(mockFetch.calls[0].init.method).to.equal('HEAD'); + }); + + it('should pass in a signal with the init object without mutating the init', done => { + const myInit = {method: 'DELETE'}; + const fetch$ = fromFetch('/bar', myInit); fetch$.subscribe({ error: done, complete: done, }); - expect(mockFetch.calls[0].init).to.equal(myInit); + expect(mockFetch.calls[0].init.method).to.equal(myInit.method); + expect(mockFetch.calls[0].init).not.to.equal(myInit); expect(mockFetch.calls[0].init.signal).not.to.be.undefined; }); @@ -198,4 +212,20 @@ describe('fromFetch', () => { // The subscription will not be closed until the error fires when the promise resolves. expect(subscription.closed).to.be.false; }); + + it('should treat passed already aborted signals as a cancellation token which triggers an error', done => { + const controller = new MockAbortController(); + controller.abort(); + const signal = controller.signal as any; + const fetch$ = fromFetch('/foo', { signal }); + const subscription = fetch$.subscribe({ + error: err => { + expect(err).to.be.instanceof(MockDOMException); + done(); + } + }); + expect(mockFetch.calls[0].init.signal.aborted).to.be.true; + // The subscription will not be closed until the error fires when the promise resolves. + expect(subscription.closed).to.be.false; + }); }); diff --git a/src/internal/observable/dom/fetch.ts b/src/internal/observable/dom/fetch.ts index 22953d9767..b91f8a7405 100644 --- a/src/internal/observable/dom/fetch.ts +++ b/src/internal/observable/dom/fetch.ts @@ -61,14 +61,18 @@ export function fromFetch(input: string | Request, init?: RequestInit): Observab if (init) { // If a signal is provided, just have it teardown. It's a cancellation token, basically. if (init.signal) { - outerSignalHandler = () => { - if (!signal.aborted) { - controller.abort(); - } - }; - init.signal.addEventListener('abort', outerSignalHandler); + if (init.signal.aborted) { + controller.abort(); + } else { + outerSignalHandler = () => { + if (!signal.aborted) { + controller.abort(); + } + }; + init.signal.addEventListener('abort', outerSignalHandler); + } } - init.signal = signal; + init = { ...init, signal }; } else { init = { signal }; }