-
-
Notifications
You must be signed in to change notification settings - Fork 120
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): preserve headers casing on mocked responses #463
Conversation
@@ -315,3 +315,36 @@ it('does not send request body to the original server given mocked response', as | |||
const text = await getIncomingMessageBody(response) | |||
expect(text).toBe('mock created!') | |||
}) | |||
|
|||
it.only('does not lowercase the rawHeaders', async () => { |
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.
This test should be unmarked as .only
.
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.
While this approach may work, I recommend against it (see my reasoning in the comments).
Instead, I propose we find some middle-ground in exposing the original response to the consumer (the response listener). Historically, Interceptors used to expose the original request instance in the request listener, which looked like this:
type RequestListener = (
request: Request,
ref: ClientRequest | XMLHttpRequest
) => void
This was to give the consumer the means to access and operate on that original instance, need be. I've then moved away from this call signature, finding it too dynamic and confusing for the consumer.
But I think we can bring it back, although partially. I dislike that the ref
was a completely different instance based on the request-issuing module. I think Interceptors must expose requests/responses in a unified way. I think the next unified way to represent requests/responses is HTTP message.
What do you say if we create and expose an HTTP message based on the request/response in the respective listeners?
// Approximate object shape.
interface HttpMessage {
status: number
statusText: string
headers: Record<string, string[] | string>
body?: string
}
interceptor.on('response', ({ response, httpMessage }) => {
httpMessage.headers // raw headers
})
The httpMessage
will be a custom abstraction respecting the HTTP message specification and describing the request/response as it was sent/received. This would be your primary way to access raw headers, as an example.
I'm currently working on something very closely related, and I can abstract the HttpMessage
class into a separate package and we can consume it in Interceptors then.
Having access to the HTTP message also provides more data to the consumer, as they are now able to do thing like calculating the headers/body size.
Agree!
Do you mean that the |
The |
@kettanaito maybe I'm missing something, but this way, the IncomingMessage instance (the response ) will still have lowercased header names. In the rawHeaders property. |
@mikicho, no because the HTTP message will be created from the original 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. |
What about mocked responses? The current flow is:
What do I miss? |
@mikicho, since mock responses are declared using the Fetch API 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 I will think about it and reply on this tomorrow. |
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 Let me show you how we can access that symbol a bit better... |
if (headers) { | ||
// Try extracting the raw headers from the headers instance. | ||
// If not possible, fallback to the headers instance as-is. | ||
const rawHeaders = getRawFetchHeaders(headers) || headers |
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.
We will try to read the raw headers from the internal Headers
symbols but if those are not present for any reason, fallback to the headers
instance as-is.
|
||
const headersMap = getValueBySymbol< | ||
Map<string, { name: string; value: string }> | ||
>('headers map', headersList) |
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've noticed the raw headers map is buried a bit deeper in the symbols:
Headers
- Symbol(headers list)
- Symbol(headers map)
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.
This seems to be specific to a Node.js version. This is where the raw headers map is for me on 18.14.0 but, apparently, it wasn't there on 18.8.0 used in the CI. I've updated the Node version on CI, let's see how it fares.
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 confirm that's due to the Node.js difference in versions:
- On 18.14.0,
headers map
isMap<normalizedName, { name: rawName, value: string }>
- On 18.8.0, the same
headers map
symbol isMap<normalizedName, value>
. Looks like older versions of Node doesn't even expose the raw header name in any way.
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 think they do in the Symbol(headers list).entires
(not the function entries
)
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 fixed this by checking what values headersMap
actually has. If it's not an object with a name
property, return undefined.
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.
@mikicho, unfortunately, .entries()
on headers list
contains already normalized header names in Node.js 18.8.0:
const h = new Headers([['aBc, 'value']])
getValueBySymbol('headers list', h).entries()
// [ ['abc', 'value'] ]
const ownSymbols = Object.getOwnPropertySymbols(source) | ||
|
||
const symbol = ownSymbols.find((symbol) => { | ||
return symbol.description === symbolName |
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 took your original lookup logic just wrapped it in a utility function.
interceptor.dispose() | ||
}) | ||
|
||
it('preserves the original mocked response headers casing in "rawHeaders"', async () => { |
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've also moved the raw headers test from NodeClientRequest.test.ts
to a new compliance test. I think it makes sense to be here.
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.
This looks good from my side now. @mikicho, let me know if we should merge this.
@kettanaito LGTM! Thanks! ❤️ Just a quick note, I see the value of sticking with a common and familiar object like |
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, |
Released: v0.25.7 🎉This has been released in v0.25.7! Make sure to always update to the latest version ( Predictable release automation by @ossjs/release. |
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')
returnsundefined
closes #448
Some thoughts:
Response
object which does not return lowercase headers. but IMO the solution should be in this lib.