Add an HttpClient interface and NodeHttpClient implementation. #1218

dcr-stripe · 2021-08-12T20:58:01Z

Notify

r? @richardm-stripe + @tjfontaine-stripe (for extra 👀 )

Summary

Adds an HttpClient interface and moves out the Node http/https request logic into a NodeHttpClient implementation. Update Stripe config to allow specifying a custom HttpClient.

This is the first step in allowing a custom client (say one that uses fetch for #997).

The interface is summarized in the typescript. In brief:

makeRequest returns a Response as a promise.
Response#toStream(streamCompleteCallback) returns a streamable version of the content. The Node behavior is unchanged here of just returning the raw response itself with the headers tacked on.
Response#toJSON() consumes the response and returns the JSON representation as a promise.

This should effectively be a no-op, but this should be released as its own version in case there are issues.

You can see an example rough implementation of this for fetch in #1208.

Test plan

Added unit tests both to StripeResource and to NodeHttpClient itself.

jt-stripe

A few questions about the TS definitions and a suggestion for how to expose this... I didn't carefully review all the implementation details yet!

jt-stripe · 2021-08-12T21:45:55Z

types/net/net.d.ts

+         * Converts the response content into a JSON object, failing if JSON
+         * couldn't be parsed.
+         */
+        toJSON(): Promise<object>;


Do we have a union type that's, like, all possible stripe resource types?

I don't think so unfortunately. @richardm-stripe to confirm?

We don't but could easily have one if we wanted.

Though object feels better to me here, since HTTPClient should be mostly stripe-domain-agnostic.

types/net/net.d.ts

jt-stripe · 2021-08-12T21:51:28Z

lib/net/NodeHttpClient.js

+const defaultHttpAgent = new http.Agent({keepAlive: true});
+const defaultHttpsAgent = new https.Agent({keepAlive: true});


Is there a reason not to move this into the constructor? It'd be nice to not have side effects on import in the case that this is being imported in a non-node environment.

We do need a singleton -- if the user instantiates multiple stripe clients (without overriding the agent) then they should all share the same connection pool. We could lazily instantiate it when the constructor is called, though. Although, maybe we should instead ensure that this file is not required in non-node environments? It is NodeHttpClient after all.

I think this is resolved by @dcr-stripe 's latest changes

lib/stripe.js

lib/net/NodeHttpClient.js

richardm-stripe · 2021-08-13T15:04:32Z

lib/net/HttpClient.js

+  }
+
+  /** Helper to make a consistent timeout error across implementations. */
+  static makeTimeoutError() {


This is great

richardm-stripe · 2021-08-13T15:11:30Z

types/net/net.d.ts

+       * The streamCompleteCallback should be invoked by the response
+       * implementation when the stream has been consumed.
+       */
+      toStream(streamCompleteCallback: () => void): any; //eslint-disable-line @typescript-eslint/no-explicit-any


Should we pursue generic type parameters?

export interface HttpClient<Response> { makeRequest(...): Promise<Response> } interface HttpClientResponse<RawResponse, Stream> { ... getRawResponse(): RawResponse, toStream(...): Stream, ... }

richardm-stripe · 2021-08-13T15:15:10Z

test/StripeResource.spec.js

+            if (err) {
+              return done(err);
+            }
+            const result = await stripe.charges.create(options.data);


I think this is missing a .catch so if stripe.charges.create fails the error won't be properly propagated to the test runner

richardm-stripe · 2021-08-13T15:20:15Z

test/StripeResource.spec.js

@@ -208,6 +216,98 @@ describe('StripeResource', () => {
        );
      });

+      it('throws an error on invalid JSON', (done) => {


Would there be anyway to add a test for when there's a connection error after the headers have been transmitted, but halfway through when the body is being transmitted?

(this should be pretty similar to the test case above on line #200).

That's the one case I can think of where (correct me if I'm wrong) I don't think we have any test coverage but it still would be good to know that this PR is consistent with the previous behavior (and that fetch and nodehttpclient will behave consistently, too)

richardm-stripe

This is absolutely wonderful. In particular I think the decision to use a promise interface for HttpClient vs. more conservatively attempting to preserve the callbacky interface represents a huge improvement to the understandability of the code and will make a far better experience for implementers of this interface.

I left a few comments, though just some ideas, nothing blocking. I'll re-assign you to the PR, feel free to assign me again once you've had a look/responded and I will approve.

…ions alive.

…iling.

dcr-stripe

Great ideas all around. PTAL @richardm-stripe

dcr-stripe · 2021-08-13T17:13:34Z

test/StripeResource.spec.js

@@ -208,6 +216,98 @@ describe('StripeResource', () => {
        );
      });

+      it('throws an error on invalid JSON', (done) => {


dcr-stripe · 2021-08-13T17:42:32Z

types/net/net.d.ts

+       * The streamCompleteCallback should be invoked by the response
+       * implementation when the stream has been consumed.
+       */
+      toStream(streamCompleteCallback: () => void): any; //eslint-disable-line @typescript-eslint/no-explicit-any


dcr-stripe · 2021-08-13T17:44:55Z

test/StripeResource.spec.js

+            if (err) {
+              return done(err);
+            }
+            const result = await stripe.charges.create(options.data);


test/net/NodeHttpClient.spec.js

richardm-stripe

LGTM, (pending fixing that TS complaint about any)

Co-authored-by: Richard Marmorstein <[email protected]>

Custom request timeout message was never used since we were incorrectly requiring the HttpClient class. I believe this has stopped working on stripe#1218.

stripe-ci assigned richardm-stripe and tjfontaine-stripe Aug 12, 2021

Add an HttpClient interface and NodeHttpClient implementation.

3f6a815

dcr-stripe force-pushed the dcr-http-client-interface branch from 1efb6fb to 3f6a815 Compare August 12, 2021 20:59

dcr-stripe requested review from richardm-stripe and tjfontaine-stripe August 12, 2021 20:59

jt-stripe reviewed Aug 12, 2021

View reviewed changes

Update stale makeRequest definition.

90293d3

richardm-stripe reviewed Aug 12, 2021

View reviewed changes

lib/net/NodeHttpClient.js Outdated Show resolved Hide resolved

richardm-stripe reviewed Aug 13, 2021

View reviewed changes

richardm-stripe assigned dcr-stripe and unassigned richardm-stripe Aug 13, 2021

dcr-stripe added 2 commits August 13, 2021 12:04

Add createNodeHttpClient method.

c23b6e6

Split out Response into own top-level class.

ca74ca9

dcr-stripe force-pushed the dcr-http-client-interface branch from f73feaa to eabc505 Compare August 13, 2021 16:04

Update getTestServerStripe to use an agent that does not keep connect…

f68ec30

…ions alive.

dcr-stripe force-pushed the dcr-http-client-interface branch from eabc505 to f68ec30 Compare August 13, 2021 16:22

Add test where server responds with header and partial body before fa…

066004b

…iling.

dcr-stripe force-pushed the dcr-http-client-interface branch from 5f671af to e774e89 Compare August 13, 2021 17:42

dcr-stripe commented Aug 13, 2021

View reviewed changes

stripe-ci assigned richardm-stripe and unassigned tjfontaine-stripe and dcr-stripe Aug 13, 2021

richardm-stripe reviewed Aug 13, 2021

View reviewed changes

test/net/NodeHttpClient.spec.js Outdated Show resolved Hide resolved

richardm-stripe approved these changes Aug 13, 2021

View reviewed changes

stripe-ci unassigned richardm-stripe Aug 13, 2021

stripe-ci assigned dcr-stripe Aug 13, 2021

dcr-stripe and others added 3 commits August 13, 2021 14:04

Add generics.

861da4f

Add missing catch.

f83a043

Update test/net/NodeHttpClient.spec.js

5ca4726

Co-authored-by: Richard Marmorstein <[email protected]>

dcr-stripe force-pushed the dcr-http-client-interface branch from 38e75b4 to 5ca4726 Compare August 13, 2021 18:04

Fix typescript warning about any.

e076999

dcr-stripe merged commit 6f9c519 into master Aug 13, 2021

dcr-stripe deleted the dcr-http-client-interface branch August 13, 2021 18:11

bpinto added a commit to bpinto/stripe-node that referenced this pull request Jan 21, 2022

fix(http-client): use named import

08fb2d0

Custom request timeout message was never used since we were incorrectly requiring the HttpClient class. I believe this has stopped working on stripe#1218.

bpinto mentioned this pull request Jan 21, 2022

fix(http-client): use named import #1335

Merged

bpinto added a commit to bpinto/stripe-node that referenced this pull request Jan 21, 2022

fix(http-client): use named import

87d1aba

Custom request timeout message was never used since we were incorrectly requiring the HttpClient class. I believe this has stopped working on stripe#1218.

bpinto added a commit to bpinto/stripe-node that referenced this pull request Jan 28, 2022

fix(http-client): use named import

d662acf

Custom request timeout message was never used since we were incorrectly requiring the HttpClient class. I believe this has stopped working on stripe#1218.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add an HttpClient interface and NodeHttpClient implementation. #1218

Add an HttpClient interface and NodeHttpClient implementation. #1218

dcr-stripe commented Aug 12, 2021

jt-stripe left a comment

jt-stripe Aug 12, 2021

dcr-stripe Aug 12, 2021

richardm-stripe Aug 12, 2021

jt-stripe Aug 12, 2021

richardm-stripe Aug 12, 2021 •

edited

Loading

jt-stripe Aug 12, 2021

richardm-stripe Aug 13, 2021

richardm-stripe Aug 13, 2021

dcr-stripe Aug 13, 2021

richardm-stripe Aug 13, 2021

dcr-stripe Aug 13, 2021

richardm-stripe Aug 13, 2021

dcr-stripe Aug 13, 2021

richardm-stripe left a comment

dcr-stripe left a comment

dcr-stripe Aug 13, 2021

dcr-stripe Aug 13, 2021

dcr-stripe Aug 13, 2021

richardm-stripe left a comment

		const defaultHttpAgent = new http.Agent({keepAlive: true});
		const defaultHttpsAgent = new https.Agent({keepAlive: true});

Add an HttpClient interface and NodeHttpClient implementation. #1218

Add an HttpClient interface and NodeHttpClient implementation. #1218

Conversation

dcr-stripe commented Aug 12, 2021

Notify

Summary

Test plan

jt-stripe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richardm-stripe Aug 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richardm-stripe left a comment

Choose a reason for hiding this comment

dcr-stripe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richardm-stripe left a comment

Choose a reason for hiding this comment

richardm-stripe Aug 12, 2021 •

edited

Loading