fix: support Readable as the request body #522
Conversation
| @@ -34,7 +34,7 @@ export class MockSocket extends net.Socket { | |||
| public write(...args: Array<unknown>): boolean { | |||
| const [chunk, encoding, callback] = normalizeWriteArgs(args as WriteArgs) | |||
| this.options.write(chunk, encoding, callback) | |||
| return false | |||
| return true | |||
There was a problem hiding this comment.
How does this affect the fix?
I'm hesitant to return true because it means that the chunk has been sent. However, this method will be called for request bodies that we forward to the interceptor first. Semantically, we are buffering the request chunks so returning false looks more appropriate.
There was a problem hiding this comment.
We can emit drain instead in onRequestBody/End but this would be unnatural as well.
The more "right" thing to do is to return true/false according to the this.requestStream.push(chunk) in the onRequestBody
There was a problem hiding this comment.
My concern is that this is public facing .write() while this.requestStream is an internal stream we use to feed to the HTTP parser. Returning true will notify the consumer (the user/test) that the chunk has been written while it hasn't been. It really gets buffered in the most direct sense of the word. I don't think returning true is correct here.
This can be resolved rather easily:
- Is this change necessary for the
Readablesupport to work?
If not, let's revert it and discuss it as a separate thing.
There was a problem hiding this comment.
We need to either return true or emit a drain event. I have an implementation with the drain event in my local machine.
There was a problem hiding this comment.
Got it. Okay, so let's return true since it's the path of least resistance. If it ever causes issues, we iterate. Thanks for pointing this out.
| @@ -93,6 +94,31 @@ it('writes Buffer request body', async () => { | |||
| expect(await text()).toEqual(expectedBody) | |||
| }) | |||
|
|
|||
| it('writes Readable request body', async () => { | |||
There was a problem hiding this comment.
| it('writes Readable request body', async () => { | |
| it('supports Readable as the request body', async () => { |
| @@ -6,7 +6,8 @@ import http from 'node:http' | |||
| import express from 'express' | |||
| import { HttpServer } from '@open-draft/test-server/http' | |||
| import { ClientRequestInterceptor } from '../../../../src/interceptors/ClientRequest' | |||
| import { waitForClientRequest } from '../../../helpers' | |||
| import { sleep, waitForClientRequest } from '../../../helpers' | |||
| import { Readable } from 'node:stream' | |||
There was a problem hiding this comment.
Nit: could you please move this import to the top of the file? Thanks!
|
I've opened #527 as a feature branch from the socket interceptor branch (yours points to the same branch so it's a bit problematic to iterate on). |
No description provided.