This code seems fine if want to add this feature, but I question the if.

Per spec, headers are case-insensitive. And Node allows for multiple headers using array syntax.
So what is a valid use-case for wanting to allow invalid headers?

My perspective vaguely lines up with this comment from (#804):

It is certainly a bad practice to do so but the fact is that nock is used mostly to mock external libraries calls so it would be useful to be able to disable error throwing for such cases with some options.

I definitely agree that doing things according to the design spec is nice, but I personally find that that's not always the ideal option when taking into account all the considerations of the real world and, based on the comments and 👍 's on the two issues mentions, it seems like a good number of other people also feel similarly.

Additionally, I personally think the flexibility is valuable especially for a testing tool - if the user wants to validate that something is up to spec, i feel like it makes more sense to do it in the test rather than have the tool assert how your application/library should work. Similarly, since it's a mocking tool, and since it's trying to simulate what node will do, I feel like it makes sense for it to match node's behavior.

Regardless, I think it's a good point and we should be pushing developers to develop more accurately. As a result, I've updated this PR to throw a warning whenever it detects this improper header setup. If you don't feel like that's sufficient, I can also update it to take an option where it will be strict about this or whether it will ignore this.

What is the Node behavior we’re trying to emulate here? Could a test be added which invokes the http module directly?

I guess I agree with the principle that we should have parity with the real http behavior.

The part that is a bit tricky is that we don’t want people who are writing assertions about the headers they’re sending to be surprised.

I just meant that Node isn't strict about this behavior and so it's somewhat inconsistent to disallow something that node does allow. The test that I added should already verify that nock works with duplicate headers with the same value.

That second point makes a lot of sense to me, but I feel like that confusion already exists, since the header keys are being converted to lowercase in an invisible way anyways. For me, that lead to confusing occurrences of Failed to convert header keys to lower case due to field name conflict: ${key}. I think the warning does a decent amount to signal that something is not correct while preventing confusing errors, but I'm also open to other suggestions.

Sorry, what is Node’s behavior?

I meant that Node doesn't throw an error on duplicate headers of different cases despite the detail that it's not up to spec. It's closer to the node behavior to avoid throwing an error on duplicate headers of different cases when possible.

Yea, makes sense. Does it send the header twice or does it deduplicate them?

They deduplicate by converting to lower case. This line (https://github.com/nodejs/node/blob/90b5f1b1078961897082842b89d7bc9631ebf312/lib/_http_incoming.js#L246) avoids setting the header if the header is already set. I've updated the diff to do this instead. Since the JS object iterator loops over properties in the order they were inserted, this change should be able to infer which header was set first.

However, it looks like properly handling HTTP headers is much more complicated than I previously thought. It seems as though different clients can have slightly different behaviors. While the spec specifies both that header names should be case-insensitive and that duplicate headers can be used, the spec doesn't describe how case-insensitivity should be enforced and how duplicate headers should be combined. This discussion (nodejs/node#3591) highlights more of these frustrations.

There's a lot more nuance in the edge cases that Node handles here than what my diff tries to do. However, Node doesn't seem to throw an error in any of these cases.

Hmm, yes, this sure is complicated, isn't it? 😀

I wonder if we could delegate the resolution of at least request headers to Node's implementation. I'd hate for us to implement something that mimics Node but has subtle differences. I feel like that could lead to bad surprises and difficult-to-catch bugs, where e.g. Node concatenates duplicate header values but Nock drops one of them.

After reading #1192, which was never resolved, and #804, which was resolved in #852, I'm a bit lost about the use case we're trying to address here.

Is it that you want to treat request headers in a way that provides parity with Node (even though Node's is arguably ambiguous / confusing)? Is the intention to leave reply headers alone? It would help me a lot to have a code snippet that exhibits the desired behavior. That way I can more easily verify the disparity between Nock and Node.

The basic frustration to me is that any test involving these types of headers fails on an error while the actual request would succeed. That's the fundamental thing I'm trying to solve.

I was thinking that mimicking the way that node handles it was a reasonable way of making it more realistic, but I agree with your point that having it almost match node's behavior is more confusing than not matching it at all. I personally think the best approach is to avoid pretending to match behaviors and throw a warning instead of an error, but I'm open to other thoughts.

The basic frustration to me is that any test involving these types of headers fails on an error while the actual request would succeed. That's the fundamental thing I'm trying to solve.

For the sake of understanding, why is that important? In other words, why can't the library / application stop sending the ambiguous duplicate header?

There's no reason that the library/application can't - this is just a change of convenience. It's inconvenient for users to have to fix a "bug" that isn't real. That's especially true given the fact that duplicate headers are allowed in HTTP spec and used for headers that are arrays. Currently, applications that take advantage of this detail would fail for no good reason. I'm not sure what the benefit is of giving nock users unnecessary work.

The problem is that duplicate headers in HTTP need to have their values concatenated, not deduplicated, and in Node that involves these complicated heuristics.

To do what you’re asking, we need to completely match Node’s behavior. Otherwise we risk incorrectly making req header assertions, attaching the wrong headers to the objects that are passed to reply functions, and incorrectly recording headers when using the recorder.

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. We try to do our best, but nock is maintained by volunteers and there is only so much we can do at a time. Thank you for your contributions.