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

Support returning streams from callbacks #145

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

rageshkrishna
Copy link

You can now pass a Readable stream into rawBody inside thenCallback.

Fixes #98

You can now pass a `Readable` stream into `rawBody` inside
`thenCallback`.
@CLAassistant
Copy link

CLAassistant commented May 18, 2023

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@pimterry
Copy link
Member

Thanks @rageshkrishna! This looks very promising. My one concern is that this results in rawBody streams being supported in both beforeX callbacks and thenCallback, but behaving entirely different in each - beforeX buffering the entire thing, while thenCallback directly streams into the live body.

I think rather than being inconsistent, would be better to either a) explicitly disallow this in beforeX callbacks for now, or b) somehow support this in beforeX callbacks fully, streaming the data without buffering. I could be persuaded either way... This might also interact with your proposal in #143. What do you think?

@@ -1,6 +1,9 @@
import * as zlib from 'zlib';
import { getLocal } from "../../..";
import { expect, fetch, isNode, isWeb, headersToObject } from "../../test-utils";
import * as http from 'node:http';
Copy link
Member

Choose a reason for hiding this comment

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

The tests are failing because of this - this spec runs in browsers too, so you can't use Node modules like this.

You could make this test node-only, and then this would work if you import this directly as http (the tests are already configured to exclude that in browser builds) but it would be better to write this so it will pass in both node & browsers.

I think you can just use streams directly without http.request to test this equally well, in the same way we do testing in https://github.com/httptoolkit/mockttp/blob/main/test/integration/handlers/stream-response.spec.ts.

Copy link
Author

Choose a reason for hiding this comment

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

Oops :) The one part of the dev environment that I hadn't setup happens to be exactly where I created a test failure. I picked node:http instinctively because that's what I was going to use in my planned use case for this feature. I'll try to figure this out today.

@rageshkrishna
Copy link
Author

I think rather than being inconsistent, would be better to either a) explicitly disallow this in beforeX callbacks for now, or b) somehow support this in beforeX callbacks fully, streaming the data without buffering. I could be persuaded either way... This might also interact with your proposal in #143. What do you think?

I'm all for consistency. I may be wrong about this (being new to the project and all), but it seemed like the beforeX callbacks expect to have a full body available for inspection today. Let me take a deeper look at this and see if I can make it work without it being a breaking change. Thanks for your feedback!

@pimterry
Copy link
Member

it seemed like the beforeX callbacks expect to have a full body available for inspection today

Yes, they do, but that doesn't have to mean they couldn't subsequently stream responses returned from the callback anyway. I'd be interested if you find a nice way to make buffering the input optional, but I suspect that's very complicated without breaking changes, and it's a somewhat independent problem.

In effect, right now they do wait for the incoming request to complete beforehand, but with this change they'd also wait & buffer afterwards. We could remove that second wait, and then it'd be relatively consistent with thenCallback.

It'd still be a bit awkward to use in streaming scenarios because if your original request or upstream server is also streaming then you have a big delay before your callback, but that already happens today, and it does open up some use cases regardless and matches the new thenCallback behaviour much more closely.

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.

Any example to implement event stream mocking
3 participants