http: deprecate legacy array header notation

[brendanashworth]

Ever since commit 316e283, flat object header notation has been
preferred over array-in-array notation, ex:

[['Content-Type', 'text/plain'], ['X-Data', 'hello world']]

This commit officially deprecates this functionality. This frees us up
for further optimizations and cleaner code, removing the spaghetti-like
residue of this five-year-old feature.

[kesla]

👍 This is great!

[kesla]

Perhaps we should take this one step further, officially deprecating this feature now and then remove it in the next major release? Since the array-in-array notation is undocumented it should be safe...

[rlidwka]

How would someone generate multiple HTTP headers with object syntax?

[brendanashworth]

@rlidwka this commit only forces the top level of the headers to be an object. You can still do multiple headers like so:

req.addTrailers({
  'Content-Type': [
    'text/plain',
    'text/html'
  ],
  'Set-Cookie': 'xyz'
});

[brendanashworth]

@kesla yes, definitely! It could be removed when 2.0.0 comes around, but it may be noted that before the commit I noted, this was the preferred (only?) way to do this, so it could potentially break many HTTP servers written before then. That was a long time ago, though, so I'm unsure whether we should support it or whether it even works like it once did.

[domenic]

Hmm, -1, the existing syntax matches ES6 Map constructor; why wouldn't you support it?

[brendanashworth]

@domenic The resemblance to the Map constructor is kind of moot, because [['Content-Type', 'text/plain']] is neither a Map when passed nor could a Map be passed. Implementing Maps into the API could be a valid discussion, but it isn't really related to the deprecation -- we'd just be lying to the user to keep it in because it "looks like a Map" but isn't.

Plus, the smaller the API the faster.

[domenic]

It seems nice to be able to construct headers in io.js the same way you construct them in the Fetch API in browsers (which uses the Map constructor signature).

[brendanashworth]

I personally don't think that the resemblance is too important but if others agree than I can close this.

[brendanashworth]

Perhaps there isn't enough support behind this to get this merged. I'll leave it open for a few more days, but then I'll close it pending no discussion.

[Fishrock123]

-0, maybe it would be nice to support map in the future? I'm not really too sure. I've never used this form but there seems little reason to deprecate it in code yet.

(Also, please check discussions for a formal deprecation policy: #1704)

[brendanashworth]

Yeah, I can't see this ever getting merged. Thanks everyone.

[ronkorving]

+1 but a bit late I fear

[ronkorving]

I will say this though. Keeping unused pre-legacy code just because some day maybe people might like it promotes spaghetti imho. And it's still not publically documented, so it's not like anyone will actually use this "feature".

[ronkorving]

I would also add to this that it seems the array support is completely broken. Look at the code and notice how this entire block of logic is ignored and this block seems to be executed without purpose.

Actually, the latter has a single purpose:
It allows duplicate header names to be sent (even when not HTTP compliant) by avoiding the setHeader function (an undocumented hack, more than a feature if you ask me). It's the HTTP server that is tasked with parsing it and merging duplicate headers into comma-separated values. The client's job if you ask me (although technically valid). This implementation also ignores options.host and options.auth completely (this is a bug).

I doubt all this is really by design, is it? It just seems like unused broken legacy code (that has become hard to maintain).

So imho, we should either fix it and make this a real and documented feature, or get it out of the codebase.

[domenic]

And it's still not publically documented, so it's not like anyone will actually use this "feature".

HAHAHAHAHAHAHAHAHAHAHAHAHAHAHA.

OK, sorry, but let's not kid ourselves here :). Removing code that has worked since 0.2.x (guessing) is going to break a lot of packages.

[ronkorving]

I'll just take that as you being on the "let's fix it" side. As I pointed out, it is broken. Or rather, it breaks options.

[domenic]

As long as it's done in a backward-compatible way, sure. But I'm not sure that's really worth the effort. I'd wait for someone to actually report the bug before we do so, personally.