Support returning streams from callbacks #145
Conversation
You can now pass a `Readable` stream into `rawBody` inside `thenCallback`.
|
|
|
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'; | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
I'm all for consistency. I may be wrong about this (being new to the project and all), but it seemed like the |
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. |
You can now pass a
Readablestream intorawBodyinsidethenCallback.Fixes #98