mswjs / interceptors

Low-level network interception library.
https://npm.im/@mswjs/interceptors
MIT License
535 stars 120 forks source link

fix(ClientRequest): preserve headers casing on mocked responses #463

Closed mikicho closed 10 months ago

mikicho commented 11 months ago

There are a few typing issues, but I would like to hear what you think about this solution first. Do you know how can we find the Symbol more elegantly? I'm not very familiar with Symbols and Symbol.for('headers list') returns undefined

closes #448


Some thoughts:

  1. While this solution probably will work for good, it relies on an "unofficial" behavior of Node.
  2. Perhaps we could add a function that allows users to set the 'rawHeaders', although it may be a bit clumsy.
  3. We can also solve it from Nock's side by sending a tempered version of the Response object which does not return lowercase headers. but IMO the solution should be in this lib.
mikicho commented 11 months ago

I dislike that the ref was a completely different instance

Agree!

This would be your primary way to access raw headers, as an example.

Do you mean that the response hook no longer would be read-only? If the response remains read-only, I believe that including this in the request hook would be more appropriate. This is because we must ensure that the headers are set correctly in the response instance that the user receives in the callback.

kettanaito commented 11 months ago

The response event remains read-only. What I meant is to expose another property in the listener to that event called httpMessage, which will be an abstraction over the originally sent HTTP message. That way the consumer can operate with the response using its Fetch API Response representation as well as the "raw" HTTP message representation (your use case, to access the raw response headers). Neither allow modifying the response.

mikicho commented 11 months ago

@kettanaito maybe I'm missing something, but this way, the IncomingMessage instance (the response ) will still have lowercased header names. In the rawHeaders property.

kettanaito commented 11 months ago

@mikicho, no because the HTTP message will be created from the original IncomingMessage not its fetch representation. That way, the HTTP message will contain the raw headers as they came, which seems to be the expected behavior here. That way we have a more low-level representation of responses (and even requests, if we want to) without exposing a variadic request instance reference, which would be different for each request client.

I've published the package to GitHub (ref), will publish to NPM soon. You can take a look to see if it'd work for this use case.

mikicho commented 11 months ago

What about mocked responses? I think I'm missing something.

The current flow is:

  1. someone calls the req.end() function
  2. Call to all request event listeners
  3. Check if someone called to responseWith in one of the listeners
  4. If yes, use the mocked response which is a Response instance with lowercased header names.

What do I miss?

kettanaito commented 11 months ago

@mikicho, since mock responses are declared using the Fetch API Response class, there is no concept of raw headers there. The response headers will be available by their raw name and by their normalized name (lowercase). Generally, response headers will be case-insensitive.

This will make your test pass in both cases:

// Assert normalized response headers.
const normalizedHeaders = Object.fromEntries(response.headers.entries())
expect(normalizedHeaders).toEqual(...)

// Assert raw response headers.
expect(response.headers.get('X-Custom-Header')).toBe('Expected Value')

But I think I understand the issue here. Since it's Response -> IncomingMessage translation, you also want to make sure that ClientRequest receives the IncomingMessage object with the right headers, which are case-sensitive (speaking of rawHeaders).

I will think about it and reply on this tomorrow.

kettanaito commented 11 months ago

Apologies for not understanding the requirements here sooner. I think I've finally wrapped my head around them. Yes, I think your original proposal to tap into Headers's symbols may be our best bet here given the constraints of Response.

Let me show you how we can access that symbol a bit better...

mikicho commented 11 months ago

@kettanaito LGTM! Thanks! ❤️ I still need to verify the support limit with Nock's maintainer(s?), but I think I can make a good workaround until 18.14 is widely used and even deprecated.

Just a quick note, I see the value of sticking with a common and familiar object like Response, but it can also restrict us in unforeseen ways. It might be beneficial to consider a solution that can help us avoid these limitations moving forward.

kettanaito commented 10 months ago

It might be beneficial to consider a solution that can help us avoid these limitations moving forward.

You may be right. But the way I see it—the Fetch API is extremely powerful and is the future of JavaScript. It's limitations are imposed only by our familiarity with, frankly, archaic ways of dealing with network requests in Node.js (looking at you, http.IncomingMessage). Things like header sensitivity have little practice use since most specs declare headers as case-insensitive. I'd even go as far as to say that writing tests for rawHeaders is testing Node's implementation details but I understand that the line between those is rather blurred (has been blurred for years when using Node).

kettanaito commented 10 months ago

Released: v0.25.7 🎉

This has been released in v0.25.7!

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.