fix(ClientRequest): use `ServerResponse` to build the HTTP response string #596

mikicho · 2024-07-06T00:18:11Z

Context: this allows us to support mocked responses with Transfer-Encoding and, potentially, other response transformations.

I still need to give meaningful names to some variables, but it works as expected.

The idea here is that MSW acts as a Node.js server and uses the ServerResponse class to build the final chunks that we later push to our socket.

The only caveat I found is that we are maybe TOO close to the Node.js implementation 😅. For example, Nock's tests found that ServerResponse automatically returns connection and date headers, which we now add to the response.
I think we can remove this default behavior, but I first want to know how you see it.
@kettanaito

kettanaito · 2024-07-06T10:12:31Z

src/interceptors/ClientRequest/MockHttpSocket.ts

+        callback()
+      },
+    })
+    const fakeResponse = new ServerResponse(new IncomingMessage(new net.Socket()))


Any chance this can also handle compression for us?

Hmm.. good point.
I thought we would get this OOTB from Node.js when the response goes back to the client.

I hope so! I think we have some tests for compression but I got a new issue about it in MSW recently, which made me think something was still missing. Anyways, we'll see.

kettanaito · 2024-07-06T10:48:09Z

src/interceptors/ClientRequest/MockHttpSocket.ts

-    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))


I passed this socket instance as the socket of the IncomingMessage used here. I think it should be more error-proof in case this server response decides to terminate the socket (rightfully), for example.

I used a new socket because I was afraid of infinity loops, but if it makes sense to you, I'm ok with this

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.

kettanaito · 2024-07-06T10:49:04Z

src/interceptors/ClientRequest/MockHttpSocket.ts

+     * @see https://github.com/nodejs/node/blob/10099bb3f7fd97bb9dd9667188426866b3098e07/test/parallel/test-http-server-response-standalone.js#L32
+     */
+    serverResponse.assignSocket(
+      new MockSocket({


I used the MockSocket we already have instead of Writable. While sockets extend streams, they implement additional methods on top. I know Node.js uses a simple Writable in their test but we have to acknowledge that test case is controlled. Our use cases are going to be everything out there. Using an actual socket-compatible instance is crucial here.

kettanaito · 2024-07-06T10:49:21Z

src/interceptors/ClientRequest/MockHttpSocket.ts

-    // An empty line separating headers from the body.
-    httpHeaders.push(Buffer.from('\r\n'))
-
-    const flushHeaders = (value?: Uint8Array) => {


I really like that we can drop this now. Fantastic!

kettanaito · 2024-07-06T10:49:36Z

test/modules/http/response/http-response-transfer-encoding.test.ts

+  const { res, text } = await waitForClientRequest(request)
+
+  expect(res.statusCode).toBe(200)
+  expect(res.headers).toHaveProperty('transfer-encoding', 'chunked')


Added a few assertions that the client also receives the right response headers.

kettanaito

This is nothing short of fantastic. Great job, @mikicho 👏

kettanaito · 2024-07-06T10:51:02Z

Regarding the Connection and Date headers being added to the mocked response, I'm not sure. I'm leaning toward removing them. Only the headers the developer themselves provided in the mocked response instance should be set.

I've noticed we don't have any tests for strict response header equality either. Perhaps this is the time we added them.

@mikicho, what do you think about this?

kettanaito · 2024-07-06T10:57:07Z

From the RFC9110, the Date section:

An origin server with a clock (as defined in Section 5.6.7) MUST generate a Date header field ...
An origin server without a clock MUST NOT generate a Date header field.

So the addition of the Date (and I assume by extension the Connection header) seems to be dictated by the specification. Node.js just follows it.

Now, it's debatable who should be in charge of this. I think it's may be okay for us to leave those headers if they mean better spec compatibility out of the box. It'd even be good, in fact. As long as we are sure that the developer can override (or remove) those headers if they want to.

kettanaito · 2024-07-06T11:02:18Z

My opinion on this jumps back and forth every time I think about it.

The connection and date headers make sense from the server's perspective. Request listeners are not technically servers. You can emulate one if you want, but you are not required to.

Moreover, new Response() doesn't attach those headers by default. It may be confusing to see them added by the interceptor when the developer had no intention of having them added.

Let's remove those server headers. Keep the existing behavior, make the mocked response a WYSIWYG.

mikicho · 2024-07-06T11:03:24Z

I'm leaning toward removing them.

Me, too. I think we are a mocking tool and the DX > compatibility. It may be confusing because, as a mocking tool, compatibility IS better than DX (it prevents terrible production errors).
Anyway, in this specific case, I think we should remove these default headers.

kettanaito · 2024-07-06T11:05:57Z

@mikicho, agreed. Pushed the fix and also adjusted the test for the empty mocked response to assert that the received IncomingMessage headers are empty.

kettanaito · 2024-07-06T11:06:47Z

src/interceptors/ClientRequest/MockHttpSocket.ts

    )
+    serverResponse.statusCode = response.status


Removing these server response headers before the developer-sent headers. They can always set these if they wish to.

kettanaito · 2024-07-06T23:16:52Z

Released: v0.32.1 🎉

This has been released in v0.32.1!

📄 Release notes
📦 npm package

Make sure to always update to the latest version (npm i @mswjs/interceptors@latest) to get the newest features and bug fixes.

Predictable release automation by @ossjs/release.

before cleanup

58155de

kettanaito reviewed Jul 6, 2024

View reviewed changes

kettanaito added 3 commits July 6, 2024 12:42

chore: create a new test suite

dc400ce

fix(MockHttpSocket): use MockSocket vs raw Writable

fe8dbe6

fix(MockHttpSocket): improve naming

11bd42d

kettanaito reviewed Jul 6, 2024

View reviewed changes

kettanaito approved these changes Jul 6, 2024

View reviewed changes

kettanaito changed the title ~~use ServerResponse to build the HTTP response string~~ fix(ClientRequest): use ServerResponse to build the HTTP response string Jul 6, 2024

fix(MockHttpSocket): remove server "connection" and "date" headers

ffffde7

kettanaito reviewed Jul 6, 2024

View reviewed changes

kettanaito merged commit fedac45 into main Jul 6, 2024
2 checks passed

kettanaito deleted the Michael/nodejs-support-transfer-encoding branch July 6, 2024 11:11

kettanaito mentioned this pull request Jul 6, 2024

Nock compatibility #575

Open

19 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(ClientRequest): use `ServerResponse` to build the HTTP response string #596

fix(ClientRequest): use `ServerResponse` to build the HTTP response string #596

mikicho commented Jul 6, 2024 •

edited by kettanaito

Loading

kettanaito Jul 6, 2024

mikicho Jul 6, 2024

kettanaito Jul 6, 2024

kettanaito Jul 6, 2024

mikicho Jul 6, 2024 •

edited

Loading

kettanaito Jul 6, 2024 •

edited

Loading

kettanaito Jul 6, 2024

kettanaito Jul 6, 2024

kettanaito Jul 6, 2024

kettanaito left a comment

kettanaito commented Jul 6, 2024 •

edited

Loading

kettanaito commented Jul 6, 2024

kettanaito commented Jul 6, 2024

mikicho commented Jul 6, 2024 •

edited

Loading

kettanaito commented Jul 6, 2024

kettanaito Jul 6, 2024

kettanaito commented Jul 6, 2024

fix(ClientRequest): use ServerResponse to build the HTTP response string #596

fix(ClientRequest): use ServerResponse to build the HTTP response string #596

Conversation

mikicho commented Jul 6, 2024 • edited by kettanaito Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikicho Jul 6, 2024 • edited Loading

Choose a reason for hiding this comment

kettanaito Jul 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kettanaito left a comment

Choose a reason for hiding this comment

kettanaito commented Jul 6, 2024 • edited Loading

kettanaito commented Jul 6, 2024

kettanaito commented Jul 6, 2024

mikicho commented Jul 6, 2024 • edited Loading

kettanaito commented Jul 6, 2024

Choose a reason for hiding this comment

kettanaito commented Jul 6, 2024

Released: v0.32.1 🎉

fix(ClientRequest): use `ServerResponse` to build the HTTP response string #596

fix(ClientRequest): use `ServerResponse` to build the HTTP response string #596

mikicho commented Jul 6, 2024 •

edited by kettanaito

Loading

mikicho Jul 6, 2024 •

edited

Loading

kettanaito Jul 6, 2024 •

edited

Loading

kettanaito commented Jul 6, 2024 •

edited

Loading

mikicho commented Jul 6, 2024 •

edited

Loading