Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Binary streaming #1157

Merged
merged 4 commits into from
Jun 25, 2021
Merged

Binary streaming #1157

merged 4 commits into from
Jun 25, 2021

Conversation

richardm-stripe
Copy link
Contributor

@richardm-stripe richardm-stripe commented May 11, 2021

Notify

r? @dcr-stripe
cc @stripe/api-libraries

Summary

This PR adds support for defining 'streaming' stripe methods on the Stripe client. Unlike existing methods, 'streaming' methods do not buffer the response body into memory and try to parse it into json -- they pass the underlying http.Request object to the caller, so that the caller can pipe it to a file or socket without having to buffer it.

The motivation for this is an upcoming /pdf method that will return a pdf instead of JSON.

Also incidentally adds the ability to set "host" per-request, this was useful in my testing.

@richardm-stripe richardm-stripe marked this pull request as draft May 12, 2021 03:52
@richardm-stripe richardm-stripe force-pushed the richardm-binary-streaming branch from cf771e6 to f825d89 Compare June 21, 2021 19:39
@richardm-stripe richardm-stripe force-pushed the richardm-binary-streaming branch from 10cea53 to e0469af Compare June 21, 2021 19:43
@richardm-stripe richardm-stripe marked this pull request as ready for review June 24, 2021 15:48
Copy link
Contributor

@dcr-stripe dcr-stripe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall! Extremely minor comments - thanks Richard!

lib/StripeResource.js Outdated Show resolved Hide resolved
lib/StripeResource.js Show resolved Hide resolved
@richardm-stripe richardm-stripe force-pushed the richardm-binary-streaming branch from 87f1e7b to 2923bde Compare June 25, 2021 03:00
@richardm-stripe
Copy link
Contributor Author

r? @dcr-stripe - the problem was that there were test cases that were supposed to trigger timeouts by never calling res.end(). In these cases, the "finish" or "close" events wouldn't fire. There's not really a cute way to automatically close the server in these cases after the test case is finished, so I had the helper expose a closeServer function to trigger manually on the test case, for now.
2923bde

Copy link
Contributor

@dcr-stripe dcr-stripe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

problem was that there were test cases that were supposed to trigger timeouts by never calling res.end(). In these cases, the "finish" or "close" events wouldn't fire. There's not really a cute way to automatically close the server in these cases after the test case is finished, so I had the helper expose a closeServer function to trigger manually on the test case, for now.

Ahhh good find. Makes sense! Thanks for digging into this Richard - LGTM! 🚢

@richardm-stripe richardm-stripe merged commit 12ebce4 into master Jun 25, 2021
@richardm-stripe richardm-stripe deleted the richardm-binary-streaming branch June 25, 2021 15:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants