From bc7316b5cf4d4790f357bcb05bb7894b2c6825ba Mon Sep 17 00:00:00 2001 From: achingbrain Date: Sat, 2 May 2020 15:10:21 +0100 Subject: [PATCH 1/4] fix: make sure we honour headers passed as api and constructor args Adds tests for the same. Depends on: - [ ] https://github.com/ipfs/aegir/pull/553 --- package.json | 2 +- src/http.js | 30 +++++++++++++++++++++--------- test/http.spec.js | 21 +++++++++++++++++++++ 3 files changed, 43 insertions(+), 10 deletions(-) diff --git a/package.json b/package.json index 6d95106..617daa0 100644 --- a/package.json +++ b/package.json @@ -45,7 +45,7 @@ "stream-to-it": "^0.2.0" }, "devDependencies": { - "aegir": "^21.8.0", + "aegir": "ipfs/aegir#feat/echo-request-headers", "delay": "^4.3.0", "it-all": "^1.0.1", "it-drain": "^1.0.0", diff --git a/src/http.js b/src/http.js index a15cd4b..81c81cd 100644 --- a/src/http.js +++ b/src/http.js @@ -2,7 +2,7 @@ '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') @@ -146,9 +146,6 @@ class HTTP { 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 timeout: undefined }), opts.timeout, this.abortController) @@ -188,7 +185,10 @@ class HTTP { * @returns {Promise} */ post (resource, options = {}) { - return this.fetch(resource, merge(this.opts, options, { method: 'POST' })) + return this.fetch(resource, { + ...options, + method: 'POST' + }) } /** @@ -197,7 +197,10 @@ class HTTP { * @returns {Promise} */ get (resource, options = {}) { - return this.fetch(resource, merge(this.opts, options, { method: 'GET' })) + return this.fetch(resource, { + ...options, + method: 'GET' + }) } /** @@ -206,7 +209,10 @@ class HTTP { * @returns {Promise} */ put (resource, options = {}) { - return this.fetch(resource, merge(this.opts, options, { method: 'PUT' })) + return this.fetch(resource, { + ...options, + method: 'PUT' + }) } /** @@ -215,7 +221,10 @@ class HTTP { * @returns {Promise} */ delete (resource, options = {}) { - return this.fetch(resource, merge(this.opts, options, { method: 'DELETE' })) + return this.fetch(resource, { + ...options, + method: 'DELETE' + }) } /** @@ -224,7 +233,10 @@ class HTTP { * @returns {Promise} */ options (resource, options = {}) { - return this.fetch(resource, merge(this.opts, options, { method: 'OPTIONS' })) + return this.fetch(resource, { + ...options, + method: 'OPTIONS' + }) } } diff --git a/test/http.spec.js b/test/http.spec.js index e5cfdd3..b79ea62 100644 --- a/test/http.spec.js +++ b/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: { From faee8187184f9d717130aa81228e4b0e7b4748ac Mon Sep 17 00:00:00 2001 From: achingbrain Date: Sun, 3 May 2020 08:35:56 +0100 Subject: [PATCH 2/4] chore: do not convert headers into Headers object --- src/http.js | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/src/http.js b/src/http.js index 81c81cd..ddf12bd 100644 --- a/src/http.js +++ b/src/http.js @@ -32,24 +32,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,7 +96,7 @@ class HTTP { constructor (options = {}) { /** @type {APIOptions} */ this.opts = merge(defaults, options) - this.opts.headers = new Headers(options.headers) + this.opts.headers = options.headers // connect internal abort to external this.abortController = new AbortController() From c12054e80c8a790fa134a4a5f596bbd9a73c4ea0 Mon Sep 17 00:00:00 2001 From: achingbrain Date: Sun, 3 May 2020 08:52:40 +0100 Subject: [PATCH 3/4] fix: do not share abort signals between requests chore: PR comments --- package.json | 1 + src/http.js | 19 ++++++------------- 2 files changed, 7 insertions(+), 13 deletions(-) diff --git a/package.json b/package.json index 617daa0..226d0ed 100644 --- a/package.json +++ b/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", diff --git a/src/http.js b/src/http.js index ddf12bd..2ee9a20 100644 --- a/src/http.js +++ b/src/http.js @@ -6,6 +6,7 @@ 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 @@ -96,18 +97,6 @@ class HTTP { constructor (options = {}) { /** @type {APIOptions} */ this.opts = merge(defaults, options) - this.opts.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 } /** @@ -152,10 +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, + signal, timeout: undefined - }), opts.timeout, this.abortController) + }), opts.timeout, abortController) if (!response.ok && opts.throwHttpErrors) { if (opts.handleError) { From 6966db1f5a91d6a93381d7d41204f2c4139c19ee Mon Sep 17 00:00:00 2001 From: achingbrain Date: Tue, 5 May 2020 15:09:18 +0100 Subject: [PATCH 4/4] chore: update dep version --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 226d0ed..5eac8fb 100644 --- a/package.json +++ b/package.json @@ -46,7 +46,7 @@ "stream-to-it": "^0.2.0" }, "devDependencies": { - "aegir": "ipfs/aegir#feat/echo-request-headers", + "aegir": "^21.10.0", "delay": "^4.3.0", "it-all": "^1.0.1", "it-drain": "^1.0.0",