Repository: stripe/stripe-node
Pull request #1236 (2021-09-08 15:29:45)

Add an HTTP client which uses fetch.
### Notify

r? @richardm-stripe 

### Summary

Adds an `HttpClient` implementation that uses the [Fetch API](https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API).

This paves the way for unblocking https://github.com/stripe/stripe-node/issues/997.

### Test plan

The common tests are shared between NodeHttpClient and FetchHttpClient to make sure both meet the requirements for the interface (modulo some streaming/raw response tests, as this is implementation-dependant). In order to test `fetch` as part of our Node testing environment, the [node-fetch](https://www.npmjs.com/package/node-fetch) polyfill is used.


dcr-stripe: Added fixes. PTAL @richardm-stripe ? Thanks for the feedback!

Patch:
diff --git a/.eslintrc.js b/.eslintrc.js
index 15769965..5111f4c6 100644
--- a/.eslintrc.js
+++ b/.eslintrc.js
@@ -25,7 +25,7 @@ module.exports = {
       },
     ],
     'capitalized-comments': 'off',
-    'class-methods-use-this': 'error',
+    'class-methods-use-this': 'off',
     'comma-dangle': 'off',
     'comma-spacing': 'off',
     'comma-style': ['error', 'last'],
diff --git a/lib/net/FetchHttpClient.js b/lib/net/FetchHttpClient.js
new file mode 100644
index 00000000..8ced9a42
--- /dev/null
+++ b/lib/net/FetchHttpClient.js
@@ -0,0 +1,126 @@
+'use strict';
+
+const {HttpClient, HttpClientResponse} = require('./HttpClient');
+
+/**
+ * HTTP client which uses a `fetch` function to issue requests. This fetch
+ * function is expected to be the Web Fetch API function or an equivalent, such
+ * as the function provided by the node-fetch package (https://github.com/node-fetch/node-fetch).
+ */
+class FetchHttpClient extends HttpClient {
+  constructor(fetchFn) {
+    super();
+    this._fetchFn = fetchFn;
+  }
+
+  /** @override. */
+  getClientName() {
+    return 'fetch';
+  }
+
+  makeRequest(
+    host,
+    port,
+    path,
+    method,
+    headers,
+    requestData,
+    protocol,
+    timeout
+  ) {
+    const isInsecureConnection = protocol === 'http';
+
+    const url = new URL(
+      path,
+      `${isInsecureConnection ? 'http' : 'https'}://${host}`
+    );
+    url.port = port;
+
+    const fetchPromise = this._fetchFn(url.toString(), {
+      method,
+      headers,
+      body: requestData || undefined,
+    });
+
+    // The Fetch API does not support passing in a timeout natively, so a
+    // timeout promise is constructed to race against the fetch and preempt the
+    // request, simulating a timeout.
+    //
+    // This timeout behavior differs from Node:
+    // - Fetch uses a single timeout for the entire length of the request.
+    // - Node is more fine-grained and resets the timeout after each stage of
+    //   the request.
+    //
+    // As an example, if the timeout is set to 30s and the connection takes 20s
+    // to be established followed by 20s for the body, Fetch would timeout but
+    // Node would not. The more fine-grained timeout cannot be implemented with
+    // fetch.
+    let pendingTimeoutId;
+    const timeoutPromise = new Promise((_, reject) => {
+      pendingTimeoutId = setTimeout(() => {
+        pendingTimeoutId = null;
+        reject(HttpClient.makeTimeoutError());
+      }, timeout);
+    });
+
+    return Promise.race([fetchPromise, timeoutPromise])
+      .then((res) => {
+        return new FetchHttpClientResponse(res);
+      })
+      .finally(() => {
+        if (pendingTimeoutId) {
+          clearTimeout(pendingTimeoutId);
+        }
+      });
+  }
+}
+
+class FetchHttpClientResponse extends HttpClientResponse {
+  constructor(res) {
+    super(
+      res.status,
+      FetchHttpClientResponse._transformHeadersToObject(res.headers)
+    );
+    this._res = res;
+  }
+
+  getRawResponse() {
+    return this._res;
+  }
+
+  toStream(streamCompleteCallback) {
+    // Unfortunately `fetch` does not have event handlers for when the stream is
+    // completely read. We therefore invoke the streamCompleteCallback right
+    // away. This callback emits a response event with metadata and completes
+    // metrics, so it's ok to do this without waiting for the stream to be
+    // completely read.
+    streamCompleteCallback();
+
+    // Fetch's `body` property is expected to be a readable stream of the body.
+    return this._res.body;
+  }
+
+  toJSON() {
+    return this._res.json();
+  }
+
+  static _transformHeadersToObject(headers) {
+    // Fetch uses a Headers instance so this must be converted to a barebones
+    // JS object to meet the HttpClient interface.
+    const headersObj = {};
+
+    for (const entry of headers) {
+      if (!Array.isArray(entry) || entry.length != 2) {
+        throw new Error(
+          'Response objects produced by the fetch function given to FetchHttpClient do not have an iterable headers map. Response#headers should be an iterable object.'
+        );
+      }
+
+      headersObj[entry[0]] = entry[1];
+    }
+
+    return headersObj;
+  }
+}
+
+module.exports = {FetchHttpClient, FetchHttpClientResponse};
diff --git a/lib/net/HttpClient.js b/lib/net/HttpClient.js
index 407286e3..eca551a0 100644
--- a/lib/net/HttpClient.js
+++ b/lib/net/HttpClient.js
@@ -1,7 +1,5 @@
 'use strict';
 
-/* eslint-disable class-methods-use-this */
-
 /**
  * Encapsulates the logic for issuing a request to the Stripe API. This is an
  * experimental interface and is not yet stable.
diff --git a/lib/net/NodeHttpClient.js b/lib/net/NodeHttpClient.js
index 96e0a7ec..c5012321 100644
--- a/lib/net/NodeHttpClient.js
+++ b/lib/net/NodeHttpClient.js
@@ -1,7 +1,5 @@
 'use strict';
 
-/* eslint-disable class-methods-use-this */
-
 const http = require('http');
 const https = require('https');
 
diff --git a/package.json b/package.json
index ca8d1ba5..c1b3a196 100644
--- a/package.json
+++ b/package.json
@@ -39,6 +39,7 @@
     "mocha": "^8.3.2",
     "mocha-junit-reporter": "^1.23.1",
     "nock": "^13.1.1",
+    "node-fetch": "^2.6.2",
     "nyc": "^15.1.0",
     "prettier": "^1.16.4",
     "typescript": "^3.7.2"
diff --git a/yarn.lock b/yarn.lock
index 719e2f26..bc5cb884 100644
--- a/yarn.lock
+++ b/yarn.lock
@@ -1716,6 +1716,11 @@ nock@^13.1.1:
     lodash.set "^4.3.2"
     propagate "^2.0.0"
 
+node-fetch@^2.6.2:
+  version "2.6.2"
+  resolved "https://registry.yarnpkg.com/node-fetch/-/node-fetch-2.6.2.tgz#986996818b73785e47b1965cc34eb093a1d464d0"
+  integrity sha512-aLoxToI6RfZ+0NOjmWAgn9+LEd30YCkJKFSyWacNZdEKTit/ZMcKjGkTRo8uWEsnIb/hfKecNPEbln02PdWbcA==
+
 node-preload@^0.2.1:
   version "0.2.1"
   resolved "https://registry.yarnpkg.com/node-preload/-/node-preload-0.2.1.tgz#c03043bb327f417a18fee7ab7ee57b408a144301"


Test patch:
diff --git a/test/net/FetchHttpClient.spec.js b/test/net/FetchHttpClient.spec.js
new file mode 100644
index 00000000..d5f60620
--- /dev/null
+++ b/test/net/FetchHttpClient.spec.js
@@ -0,0 +1,61 @@
+'use strict';
+
+const expect = require('chai').expect;
+const fetch = require('node-fetch');
+const {Readable} = require('stream');
+const {FetchHttpClient} = require('../../lib/net/FetchHttpClient');
+
+const createFetchHttpClient = () => {
+  return new FetchHttpClient(fetch);
+};
+
+const {createHttpClientTestSuite, ArrayReadable} = require('./helpers');
+
+createHttpClientTestSuite(
+  'FetchHttpClient',
+  createFetchHttpClient,
+  (setupNock, sendRequest) => {
+    describe('raw stream', () => {
+      it('getRawResponse()', async () => {
+        setupNock().reply(200);
+        const response = await sendRequest();
+        expect(response.getRawResponse()).to.be.an.instanceOf(fetch.Response);
+      });
+
+      it('toStream returns the body as a stream', async () => {
+        setupNock().reply(200, () => new ArrayReadable(['hello, world!']));
+
+        const response = await sendRequest();
+
+        return new Promise((resolve) => {
+          const stream = response.toStream(() => true);
+
+          // node-fetch returns a Node Readable here. In a Web API context, this
+          // would be a Web ReadableStream.
+          expect(stream).to.be.an.instanceOf(Readable);
+
+          let streamedContent = '';
+          stream.on('data', (chunk) => {
+            streamedContent += chunk;
+          });
+          stream.on('end', () => {
+            expect(streamedContent).to.equal('hello, world!');
+            resolve();
+          });
+        });
+      });
+
+      it('toStream invokes the streamCompleteCallback', async () => {
+        setupNock().reply(200, () => new ArrayReadable(['hello, world!']));
+
+        const response = await sendRequest();
+
+        return new Promise((resolve) => {
+          response.toStream(() => {
+            resolve();
+          });
+        });
+      });
+    });
+  }
+);
diff --git a/test/net/NodeHttpClient.spec.js b/test/net/NodeHttpClient.spec.js
index 84855ee0..1f5a574c 100644
--- a/test/net/NodeHttpClient.spec.js
+++ b/test/net/NodeHttpClient.spec.js
@@ -1,142 +1,17 @@
 'use strict';
 
-const {Readable} = require('stream');
-
 const http = require('http');
-const nock = require('nock');
 const expect = require('chai').expect;
-// const {fail} = require('chai').assert;
 
 const {createNodeHttpClient} = require('../../lib/Stripe');
-const utils = require('../../lib/utils');
-const {fail} = require('assert');
-
-/**
- * Readable stream which will emit a data event for each value in the array
- * passed. Readable.from accomplishes this beyond Node 10.17.
- */
-class ArrayReadable extends Readable {
-  constructor(values) {
-    super();
-    this._index = 0;
-    this._values = values;
-  }
-
-  _read() {
-    if (this._index === this._values.length) {
-      // Destroy the stream once we've read all values.
-      this.push(null);
-    } else {
-      this.push(Buffer.from(this._values[this._index], 'utf8'));
-      this._index += 1;
-    }
-  }
-}
-
-describe('NodeHttpClient', () => {
-  const setupNock = () => {
-    return nock('http://stripe.com').get('/test');
-  };
-
-  const sendRequest = (options) => {
-    options = options || {};
-    return createNodeHttpClient().makeRequest(
-      'stripe.com',
-      options.port || 80,
-      '/test',
-      options.method || 'GET',
-      options.headers || {},
-      options.requestData,
-      'http',
-      options.timeout || 1000
-    );
-  };
-
-  afterEach(() => {
-    nock.cleanAll();
-  });
-
-  describe('makeRequest', () => {
-    it('rejects with a timeout error', async () => {
-      setupNock()
-        .delayConnection(31)
-        .reply(200, 'hello, world!');
-
-      try {
-        await sendRequest({timeout: 30});
-        fail();
-      } catch (e) {
-        expect(e.code).to.be.equal('ETIMEDOUT');
-      }
-    });
-
-    it('forwards any error', async () => {
-      setupNock().replyWithError('sample error');
-
-      try {
-        await sendRequest();
-        fail();
-      } catch (e) {
-        expect(e.message).to.be.equal('sample error');
-      }
-    });
-
-    it('sends request headers', async () => {
-      nock('http://stripe.com', {
-        reqheaders: {
-          sample: 'value',
-        },
-      })
-        .get('/test')
-        .reply(200);
-
-      await sendRequest({headers: {sample: 'value'}});
-    });
-
-    it('sends request data (POST)', (done) => {
-      const expectedData = utils.stringifyRequestData({id: 'test'});
-
-      nock('http://stripe.com')
-        .post('/test')
-        .reply(200, (uri, requestBody) => {
-          expect(requestBody).to.equal(expectedData);
-          done();
-        });
-
-      sendRequest({method: 'POST', requestData: expectedData});
-    });
-
-    it('custom port', async () => {
-      nock('http://stripe.com:1234')
-        .get('/test')
-        .reply(200);
-      await sendRequest({port: 1234});
-    });
 
-    describe('NodeHttpClientResponse', () => {
-      it('getStatusCode()', async () => {
-        setupNock().reply(418, 'hello, world!');
-
-        const response = await sendRequest();
-
-        expect(response.getStatusCode()).to.be.equal(418);
-      });
-
-      it('getHeaders()', async () => {
-        setupNock().reply(200, 'hello, world!', {
-          'X-Header-1': '123',
-          'X-Header-2': 'test',
-        });
-
-        const response = await sendRequest();
-
-        // Headers get transformed into lower case.
-        expect(response.getHeaders()).to.be.deep.equal({
-          'x-header-1': '123',
-          'x-header-2': 'test',
-        });
-      });
+const {createHttpClientTestSuite, ArrayReadable} = require('./helpers');
 
+createHttpClientTestSuite(
+  'NodeHttpClient',
+  createNodeHttpClient,
+  (setupNock, sendRequest) => {
+    describe('raw stream', () => {
       it('getRawResponse()', async () => {
         setupNock().reply(200);
 
@@ -184,32 +59,6 @@ describe('NodeHttpClient', () => {
           });
         });
       });
-
-      it('toJSON accumulates all data chunks in utf-8 encoding', async () => {
-        setupNock().reply(
-          200,
-          () => new ArrayReadable(['{"a', 'bc":', '"∑ 123', '"}'])
-        );
-
-        const response = await sendRequest();
-
-        const json = await response.toJSON();
-
-        expect(json).to.deep.equal({abc: '∑ 123'});
-      });
-
-      it('toJSON throws when JSON parsing fails', async () => {
-        setupNock().reply(200, '{"a');
-
-        const response = await sendRequest();
-
-        try {
-          await response.toJSON();
-          fail();
-        } catch (e) {
-          expect(e.message).to.be.equal('Unexpected end of JSON input');
-        }
-      });
     });
-  });
-});
+  }
+);
diff --git a/test/net/helpers.js b/test/net/helpers.js
new file mode 100644
index 00000000..139081aa
--- /dev/null
+++ b/test/net/helpers.js
@@ -0,0 +1,184 @@
+'use strict';
+
+const {Readable} = require('stream');
+
+const nock = require('nock');
+const expect = require('chai').expect;
+
+const utils = require('../../lib/utils');
+const {fail} = require('assert');
+
+/**
+ * Readable stream which will emit a data event for each value in the array
+ * passed. Readable.from accomplishes this beyond Node 10.17.
+ */
+class ArrayReadable extends Readable {
+  constructor(values) {
+    super();
+    this._index = 0;
+    this._values = values;
+  }
+
+  _read() {
+    if (this._index === this._values.length) {
+      // Destroy the stream once we've read all values.
+      this.push(null);
+    } else {
+      this.push(Buffer.from(this._values[this._index], 'utf8'));
+      this._index += 1;
+    }
+  }
+}
+
+/**
+ * Test runner which runs a common set of tests for a given HTTP client to make
+ * sure the client meets the interface expectations.
+ *
+ * This takes in a client name (for the test description) and function to create
+ * a client.
+ *
+ * This can be configured to run extra tests, providing the nock setup function
+ * and request function for those tests.
+ */
+const createHttpClientTestSuite = (
+  httpClientName,
+  createHttpClientFn,
+  extraTestsFn
+) => {
+  describe(`${httpClientName}`, () => {
+    const setupNock = () => {
+      return nock('http://stripe.com').get('/test');
+    };
+
+    const sendRequest = (options) => {
+      options = options || {};
+      return createHttpClientFn().makeRequest(
+        'stripe.com',
+        options.port || 80,
+        '/test',
+        options.method || 'GET',
+        options.headers || {},
+        options.requestData,
+        'http',
+        options.timeout || 1000
+      );
+    };
+
+    afterEach(() => {
+      nock.cleanAll();
+    });
+
+    describe('makeRequest', () => {
+      it('rejects with a timeout error', async () => {
+        setupNock()
+          .delayConnection(31)
+          .reply(200, 'hello, world!');
+
+        try {
+          await sendRequest({timeout: 30});
+          fail();
+        } catch (e) {
+          expect(e.code).to.be.equal('ETIMEDOUT');
+        }
+      });
+
+      it('forwards any error', async () => {
+        setupNock().replyWithError('sample error');
+
+        try {
+          await sendRequest();
+          fail();
+        } catch (e) {
+          expect(e.message).to.contain('sample error');
+        }
+      });
+
+      it('sends request headers', async () => {
+        nock('http://stripe.com', {
+          reqheaders: {
+            sample: 'value',
+          },
+        })
+          .get('/test')
+          .reply(200);
+
+        await sendRequest({headers: {sample: 'value'}});
+      });
+
+      it('sends request data (POST)', (done) => {
+        const expectedData = utils.stringifyRequestData({id: 'test'});
+
+        nock('http://stripe.com')
+          .post('/test')
+          .reply(200, (uri, requestBody) => {
+            expect(requestBody).to.equal(expectedData);
+            done();
+          });
+
+        sendRequest({method: 'POST', requestData: expectedData});
+      });
+
+      it('custom port', async () => {
+        nock('http://stripe.com:1234')
+          .get('/test')
+          .reply(200);
+        await sendRequest({port: 1234});
+      });
+
+      describe('NodeHttpClientResponse', () => {
+        it('getStatusCode()', async () => {
+          setupNock().reply(418, 'hello, world!');
+
+          const response = await sendRequest();
+
+          expect(response.getStatusCode()).to.be.equal(418);
+        });
+
+        it('getHeaders()', async () => {
+          setupNock().reply(200, 'hello, world!', {
+            'X-Header-1': '123',
+            'X-Header-2': 'test',
+          });
+
+          const response = await sendRequest();
+
+          // Headers get transformed into lower case.
+          expect(response.getHeaders()).to.be.deep.equal({
+            'x-header-1': '123',
+            'x-header-2': 'test',
+          });
+        });
+
+        it('toJSON accumulates all data chunks in utf-8 encoding', async () => {
+          setupNock().reply(
+            200,
+            () => new ArrayReadable(['{"a', 'bc":', '"∑ 123', '"}'])
+          );
+
+          const response = await sendRequest();
+
+          const json = await response.toJSON();
+
+          expect(json).to.deep.equal({abc: '∑ 123'});
+        });
+
+        it('toJSON throws when JSON parsing fails', async () => {
+          setupNock().reply(200, '{"a');
+
+          const response = await sendRequest();
+
+          try {
+            await response.toJSON();
+            fail();
+          } catch (e) {
+            expect(e.message).to.contain('Unexpected end of JSON input');
+          }
+        });
+      });
+    });
+
+    extraTestsFn(setupNock, sendRequest);
+  });
+};
+
+module.exports = {createHttpClientTestSuite, ArrayReadable};