fix: fix headers and abort signals (#41)

- make sure we honour headers passed as api and constructor args
- do not share abort signals between requests

Author: achingbrain

package.json

@@ -33,6 +33,7 @@
   "license": "MIT",
   "dependencies": {
     "abort-controller": "^3.0.0",
+    "any-signal": "^1.1.0",
     "buffer": "^5.4.2",
     "err-code": "^2.0.0",
     "fs-extra": "^9.0.0",
@@ -45,7 +46,7 @@
     "stream-to-it": "^0.2.0"
   },
   "devDependencies": {
-    "aegir": "^21.8.0",
+    "aegir": "^21.10.0",
     "delay": "^4.3.0",
     "it-all": "^1.0.1",
     "it-drain": "^1.0.0",

src/http.js

@@ -2,10 +2,11 @@
 'use strict'
 
 const fetch = require('node-fetch')
-const merge = require('merge-options')
+const merge = require('merge-options').bind({ ignoreUndefined: true })
 const { URL, URLSearchParams } = require('iso-url')
 const TextDecoder = require('./text-encoder')
 const AbortController = require('abort-controller')
+const anySignal = require('any-signal')
 
 const Request = fetch.Request
 const Headers = fetch.Headers
@@ -32,24 +33,32 @@ const timeout = (promise, ms, abortController) => {
 
   const start = Date.now()
 
+  const timedOut = () => {
+    const time = Date.now() - start
+
+    return time >= ms
+  }
+
   return new Promise((resolve, reject) => {
+    const timeoutID = setTimeout(() => {
+      if (timedOut()) {
+        reject(new TimeoutError())
+        abortController.abort()
+      }
+    }, ms)
+
     const after = (next) => {
       return (res) => {
         clearTimeout(timeoutID)
-        const time = Date.now() - start
 
-        if (time >= ms) {
-          abortController.abort()
+        if (timedOut()) {
           reject(new TimeoutError())
           return
         }
 
-        if (next) {
-          next(res)
-        }
+        next(res)
       }
     }
-    const timeoutID = setTimeout(after(), ms)
 
     promise
       .then(after(resolve), after(reject))
@@ -88,18 +97,6 @@ class HTTP {
   constructor (options = {}) {
     /** @type {APIOptions} */
     this.opts = merge(defaults, options)
-    this.opts.headers = new Headers(options.headers)
-
-    // connect internal abort to external
-    this.abortController = new AbortController()
-
-    if (this.opts.signal) {
-      this.opts.signal.addEventListener('abort', () => {
-        this.abortController.abort()
-      })
-    }
-
-    this.opts.signal = this.abortController.signal
   }
 
   /**
@@ -144,13 +141,14 @@ class HTTP {
       opts.headers.set('content-type', 'application/json')
     }
 
+    const abortController = new AbortController()
+    const signal = anySignal([abortController.signal, opts.signal])
+
     const response = await timeout(fetch(url, {
       ...opts,
-
-      // node-fetch implements it's own timeout in an addition to the spec so do not
-      // pass the timeout value on, otherwise there are two sources of timeout errors
+      signal,
       timeout: undefined
-    }), opts.timeout, this.abortController)
+    }), opts.timeout, abortController)
 
     if (!response.ok && opts.throwHttpErrors) {
       if (opts.handleError) {
@@ -188,7 +186,10 @@ class HTTP {
    * @returns {Promise<Response>}
    */
   post (resource, options = {}) {
-    return this.fetch(resource, merge(this.opts, options, { method: 'POST' }))
+    return this.fetch(resource, {
+      ...options,
+      method: 'POST'
+    })
   }
 
   /**
@@ -197,7 +198,10 @@ class HTTP {
    * @returns {Promise<Response>}
    */
   get (resource, options = {}) {
-    return this.fetch(resource, merge(this.opts, options, { method: 'GET' }))
+    return this.fetch(resource, {
+      ...options,
+      method: 'GET'
+    })
   }
 
   /**
@@ -206,7 +210,10 @@ class HTTP {
    * @returns {Promise<Response>}
    */
   put (resource, options = {}) {
-    return this.fetch(resource, merge(this.opts, options, { method: 'PUT' }))
+    return this.fetch(resource, {
+      ...options,
+      method: 'PUT'
+    })
   }
 
   /**
@@ -215,7 +222,10 @@ class HTTP {
    * @returns {Promise<Response>}
    */
   delete (resource, options = {}) {
-    return this.fetch(resource, merge(this.opts, options, { method: 'DELETE' }))
+    return this.fetch(resource, {
+      ...options,
+      method: 'DELETE'
+    })
   }
 
   /**
@@ -224,7 +234,10 @@ class HTTP {
    * @returns {Promise<Response>}
    */
   options (resource, options = {}) {
-    return this.fetch(resource, merge(this.opts, options, { method: 'OPTIONS' }))
+    return this.fetch(resource, {
+      ...options,
+      method: 'OPTIONS'
+    })
   }
 }
 

test/http.spec.js

@@ -30,6 +30,27 @@ describe('http', function () {
     })).to.eventually.be.rejectedWith().instanceOf(HTTP.TimeoutError)
   })
 
+  it('respects headers', async function () {
+    const req = await HTTP.post(`${process.env.ECHO_SERVER}/echo/headers`, {
+      headers: {
+        foo: 'bar'
+      }
+    })
+    const rsp = await req.json()
+    expect(rsp).to.have.property('foo', 'bar')
+  })
+
+  it('respects constructor headers', async function () {
+    const http = new HTTP({
+      headers: {
+        bar: 'baz'
+      }
+    })
+    const req = await http.post(`${process.env.ECHO_SERVER}/echo/headers`)
+    const rsp = await req.json()
+    expect(rsp).to.have.property('bar', 'baz')
+  })
+
   it('makes a JSON request', async () => {
     const req = await HTTP.post(`${process.env.ECHO_SERVER}/echo`, {
       json: {