Request headers have array values when using node-fetch

[fluggo]

What is the expected behavior?
The TypeScript type for req.headers says that each value is a string.

What is the actual behavior?
When calling with node-fetch, most values appear to be an array of strings. This is surprising to me because nock should be operating on a lower level and shouldn't care which library is making the request.

Possible solution
Don't know, haven't traced the code. If header value arrays are possible in any case, the type should at least be changed to string | string[].

How to reproduce the issue

const nock = require('nock@13.0.11')
const fetch = require('node-fetch@2.6.1')

const data = {foo: 'bar'}
nock('http://example.com')
  .post("/baz", {foo: 'bar'})
  .reply(function() {
    console.log(this.req.headers);
    return [200, 'blah'];
  })

fetch('http://example.com/baz', { method: 'POST', body: JSON.stringify(data) }).catch(console.error);

// Logs headers object with arrays

Runkit: https://runkit.com/fluggo/nock-nock-2197

Does the bug have a test case?
Don't know

Versions

Software	Version(s)
Nock	13.0.11
Node	14.15.4
Node-fetch	2.6.1

[gr2m]

I agree that something is fishy, but someone would need to dig into the code and find out why the current behavior is the way it is. Any help welcome!

[fredrik-w]

From a quick glance I think this is to support multiple values for a single header, RFC2616, section 4.2

Multiple message-header fields with the same field-name MAY be present in a message if and only if the entire field-value for that header field is defined as a comma-separated list [i.e., #(values)]. It MUST be possible to combine the multiple header fields into one "field-name: field-value" pair, without changing the semantics of the message, by appending each subsequent field-value to the first, each separated by a comma. The order in which header fields with the same field-name are received is therefore significant to the interpretation of the combined field value, and thus a proxy MUST NOT change the order of these field values when a message is forwarded.

And looking at node-fetch we can see that the internal representation is indeed an array:
https://github.com/node-fetch/node-fetch/blob/main/src/headers.js#L36

And looking at the nodejs api docs, it's perfectly valid for headers to be a list: https://nodejs.org/docs/latest/api/http.html#requestgetheadername

So I guess an easy fix would be to check if the value is an array, do a value.join(',')?

Not had time to debug it to actually verify my claims, but this is what I've quickly looked up as I just hit the issue myself.

[fredrik-w]

Just created a test file given the supplied example and when debugging where node-fetch drops of the request it's clearly visible that the headers are passed as arrays:

With send being declared as either http.request or https.request: https://github.com/node-fetch/node-fetch/blob/main/src/index.js#L55

[gr2m]

From a quick glance I think this is to support multiple values for a single header, RFC2616, section 4.2

I think you are right, thank you for digging into it.

I think we also pass raw headers, which is entirely an array, so we might use comma-separated header values in this.req.headers, but changing it would be a breaking change.

For now I'd close this issue as "works as expected". But feel free to continue to comment

[mastermatt]

Ahhh headers! This takes me back lol. It was headers that originally started my contributing to Nock.
The description of this PR covers the topic a bit #1564, but tl;dr HTTP headers in Node vary. Importantly for this topic, headers on ClientRequest instances are the pre-coerced values, whatever is provided by the caller.

const http = require('http')
const req = http.request('http://example.com')
req.setHeader('X-Foo', false)
req.getHeaders() // this is how Nock sets `req.headers` for the Interceptor.
// [Object: null prototype] { host: 'example.com', 'x-foo': false }

So, technically the types of the headers should be Record<string, unknown> or worse Record<string, any>. However, we (maybe just I) decided to use Record<string, string> as is provided the user experience with TS the majority of the time. Admittedly, that does not hold true for node-fetch since it always uses arrays.

A decent argument would be that the header types should be updated to use OutgoingHttpHeaders, which does specify string arrays. But it's still not technically correct, because while OutgoingHttpHeaders denotes number | string | string[], it's still possible that the values of the object are other types.
At this point, though, it would require callers to type check every time, which adds little value over any IMO.
Either way, as @gr2m mentioned, it would be a breaking change.