http: Add OutgoingMessage#addHeader

[awwright]

On occasion I've needed to append an HTTP header instead of set one. For example, Link and Set-Cookie often specify message headers in multiple places, all of which need to appear in the message.

Doing this with setHeader is awkward, requiring three different branches, depending on the current value of the header. Here is a simple polyfill:

if(!OutboundMessage.prototype.addHeader)
OutboundMessage.prototype.addHeader = function addHeader(n, v){
	const u = res.getHeader(n);
	if(u === undefined) res.setHeader(n, v);
	else if(typeof w === 'string') res.setHeader(n, [u, v] );
	else res.setHeader(n, u.concat([v]) );
}

It seems to me that userland code shouldn't have to concern itself with the current state of the headers if it doesn't need to, therefore, there should be a function that adds a new header (in addition to any existing).

I don't have documentation started yet, but this is a fairly straightforward feature; and this needs an http2 implementation. I can get started on those, if this feature looks reasonable.

Note this will re-use the capitalization of the existing header, if it exists.

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

[mscdex]

Perhaps appendHeader might be a slightly better name?

[awwright]

@mscdex appendHeader suggests to me that you're appending a string to an existing header line... is there something similar I've forgotten about?

[ronag]

I would prefer not to further expand on the http api. @mcollina wdyt?

[mcollina]

Overall I agree with @ronag. I would say this is handled by frameworks in most cases. What does fellow @nodejs/web-server-frameworks members think?

[ghermeto]

I also agree with @ronag. We should only expand the API if the use case is frequent enough and there are no reasonable alternatives. That is not the case here, as alternatives are already provided by most frameworks.

[awwright]

I would say this is handled by frameworks in most cases.

I would prefer not to monkey-patch Node.js core code; and I'm not sure how else you'd solve the problem I'm getting at here.

[ronag]

I would prefer not to monkey-patch Node.js core code; and I'm not sure how else you'd solve the problem I'm getting at here.

The web frameworks don't patch, they wrap in their own req/res objects.

[awwright]

I tend to use modular libraries in lieu of frameworks...

For example, I have this: https://github.com/awwright/node-http-transform/blob/master/lib/Pair.js — It creates a related pair of HTTP message streams, e.g. a writable ServerResponse connected to a readable IncomingMessage (that can be piped to yet another ServerResponse); the strength comes from the fact that the API is indistinguishable from the Node.js API (In theory... I'm knee deep in fixing breakage to the Node.js core test suite). I can add an addHeader function (actually I do have one), but if it's not in Node.js core, people can't rely on using it: the whole point is one side or the other could actually be a bare Node.js req/res object.

Even for frameworks though, why should userland ever have to worry about the current value of the header just to append a new one?

[awwright]

@mcollina It actually looks like there's an item on the docket for this problem, nodejs/web-server-frameworks#32

I'm curious why "Is this not handled well by any existing frameworks" ought to be the bar to pass? Do the other implicit header functions meet this test?

I would like to suggest a different test: Userspace shouldn't have to read the existing buffered headers before calling an implicit header function.

For example, headers that are only specified once (e.g. Content-Type) can simply call setHeader because it overrides any existing headers; suppose setHeader instead threw an error if you were setting a header already defined, it would be strange if userspace first had to test if the header exists, and remove it beforehand.

Likewise, for headers that may be specified multiple times, they shouldn't have to make a call to see if headers are already defined or not, they should just be able to make an appropriate implicit header call.

A separate function for adding cookies, as was proposed, would not pass this test; I agree this is appropriate for a module instead (except to the extent cookies don't follow the standard HTTP syntax, but Node.js already handles this much).

[ronag]

I think response.writeHead is the only api required in core in terms of response headers. How the headers option is built should be up to frameworks/users.

EDIT: Adding a spec compliant Headers class makes sense to me since it is decoupled from the response API, i.e. it is basically a builder for the headers option for writeHead.

[ronag]

Closing this as stale.