-
-
Notifications
You must be signed in to change notification settings - Fork 130
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
fix(ClientRequest): use ServerResponse
to build the HTTP response string
#596
Changes from 4 commits
58155de
dc400ce
fe8dbe6
11bd42d
ffffde7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,7 +4,7 @@ import { | |
type RequestHeadersCompleteCallback, | ||
type ResponseHeadersCompleteCallback, | ||
} from '_http_common' | ||
import { STATUS_CODES } from 'node:http' | ||
import { IncomingMessage, ServerResponse } from 'node:http' | ||
import { Readable } from 'node:stream' | ||
import { invariant } from 'outvariant' | ||
import { INTERNAL_REQUEST_ID_HEADER_NAME } from '../../Interceptor' | ||
|
@@ -272,36 +272,34 @@ export class MockHttpSocket extends MockSocket { | |
// if it hasn't been flushed already (e.g. someone started reading request stream). | ||
this.flushWriteBuffer() | ||
|
||
const httpHeaders: Array<Buffer> = [] | ||
// Create a `ServerResponse` instance to delegate HTTP message parsing, | ||
// Transfer-Encoding, and other things to Node.js internals. | ||
const serverResponse = new ServerResponse(new IncomingMessage(this)) | ||
|
||
httpHeaders.push( | ||
Buffer.from( | ||
`HTTP/1.1 ${response.status} ${ | ||
response.statusText || STATUS_CODES[response.status] | ||
}\r\n` | ||
) | ||
/** | ||
* Assign a mock socket instance to the server response to | ||
* spy on the response chunk writes. Push the transformed response chunks | ||
* to this `MockHttpSocket` instance to trigger the "data" event. | ||
* @note Providing the same `MockSocket` instance when creating `ServerResponse` | ||
* does not have the same effect. | ||
* @see https://github.com/nodejs/node/blob/10099bb3f7fd97bb9dd9667188426866b3098e07/test/parallel/test-http-server-response-standalone.js#L32 | ||
*/ | ||
serverResponse.assignSocket( | ||
new MockSocket({ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I used the |
||
write: (chunk, encoding, callback) => { | ||
this.push(chunk, encoding) | ||
callback?.() | ||
}, | ||
read() {}, | ||
}) | ||
) | ||
serverResponse.statusCode = response.status | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removing these server response headers before the developer-sent headers. They can always set these if they wish to. |
||
serverResponse.statusMessage = response.statusText | ||
|
||
// Get the raw headers stored behind the symbol to preserve name casing. | ||
const headers = getRawFetchHeaders(response.headers) || response.headers | ||
for (const [name, value] of headers) { | ||
httpHeaders.push(Buffer.from(`${name}: ${value}\r\n`)) | ||
} | ||
|
||
// An empty line separating headers from the body. | ||
httpHeaders.push(Buffer.from('\r\n')) | ||
|
||
const flushHeaders = (value?: Uint8Array) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I really like that we can drop this now. Fantastic! |
||
if (httpHeaders.length === 0) { | ||
return | ||
} | ||
|
||
if (typeof value !== 'undefined') { | ||
httpHeaders.push(Buffer.from(value)) | ||
} | ||
|
||
this.push(Buffer.concat(httpHeaders)) | ||
httpHeaders.length = 0 | ||
serverResponse.setHeader(name, value) | ||
} | ||
|
||
if (response.body) { | ||
|
@@ -312,35 +310,21 @@ export class MockHttpSocket extends MockSocket { | |
const { done, value } = await reader.read() | ||
|
||
if (done) { | ||
serverResponse.end() | ||
break | ||
} | ||
|
||
// Flush the headers upon the first chunk in the stream. | ||
// This ensures the consumer will start receiving the response | ||
// as it streams in (subsequent chunks are pushed). | ||
if (httpHeaders.length > 0) { | ||
flushHeaders(value) | ||
continue | ||
} | ||
|
||
// Subsequent body chukns are push to the stream. | ||
this.push(value) | ||
serverResponse.write(value) | ||
} | ||
} catch (error) { | ||
// Coerce response stream errors to 500 responses. | ||
// Don't flush the original response headers because | ||
// unhandled errors translate to 500 error responses forcefully. | ||
this.respondWith(createServerErrorResponse(error)) | ||
|
||
return | ||
} | ||
} else { | ||
serverResponse.end() | ||
} | ||
|
||
// If the headers were not flushed up to this point, | ||
// this means the response either had no body or had | ||
// an empty body stream. Flush the headers. | ||
flushHeaders() | ||
|
||
// Close the socket if the connection wasn't marked as keep-alive. | ||
if (!this.shouldKeepAlive) { | ||
this.emit('readable') | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,39 @@ | ||
/** | ||
* @vitest-environment node | ||
*/ | ||
import { it, expect, beforeAll, afterEach, afterAll } from 'vitest' | ||
import http from 'node:http' | ||
import { waitForClientRequest } from '../../../helpers' | ||
import { ClientRequestInterceptor } from '../../../../src/interceptors/ClientRequest' | ||
|
||
const interceptor = new ClientRequestInterceptor() | ||
|
||
beforeAll(() => { | ||
interceptor.apply() | ||
}) | ||
|
||
afterEach(() => { | ||
interceptor.removeAllListeners() | ||
}) | ||
|
||
afterAll(() => { | ||
interceptor.dispose() | ||
}) | ||
|
||
it('responds with a mocked "transfer-encoding: chunked" response', async () => { | ||
interceptor.on('request', ({ request }) => { | ||
request.respondWith( | ||
new Response('mock', { | ||
headers: { 'Transfer-Encoding': 'chunked' }, | ||
}) | ||
) | ||
}) | ||
|
||
const request = http.get('http://localhost') | ||
const { res, text } = await waitForClientRequest(request) | ||
|
||
expect(res.statusCode).toBe(200) | ||
expect(res.headers).toHaveProperty('transfer-encoding', 'chunked') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added a few assertions that the client also receives the right response headers. |
||
expect(res.rawHeaders).toContain('Transfer-Encoding') | ||
expect(await text()).toBe('mock') | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I passed
this
socket instance as the socket of theIncomingMessage
used here. I think it should be more error-proof in case this server response decides to terminate the socket (rightfully), for example.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used a new socket because I was afraid of infinity loops, but if it makes sense to you, I'm ok with this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't arrive at the infinite loops. This logic is only triggered on
respondWith()
, and that's called by the developer. If I spot any issues with this, will revert back to use an empty socket.