Header-case and duplicate headers about xhook HOT 6 CLOSED

Thanks for the write-up, @jpillora. Sorry, @peterjosling that you ran into an issue due to header casing. Since HTTP headers should be treated case insensitive per spec, I believe we’ll have to satisfy two constraints:

• Preserve case of all headers set in XHook.
• Allow retrieval of headers (getResponseHeader) using any case, e.g. getResponseHeader('foo'), getResponseHeader('FOO'), getResponseHeader('fOo') should all return the same.

One idea is to save a map of the original headers for sending it to the server and one for keeping a normalized map (e.g. lowercase) for case insensitive retrieval. One edge case is allowing multi-value headers where the header name is set using different cases. Maybe the last one wins then, e.g. setRequestHeader('foo', 2), setRequestHeader('FOO', 3) will be FOO: 2, 3. What do you think?

from xhook.

Yep, looks good.

The hard part is making this work with the XHook headers object API (https://github.com/jpillora/xhook#request-object). Maybe multiple setRequestHeader calls could:

• convert name:string into name:[string,string], or it could
• arr = string.split(','); arr.push(newstring); arr.join(',')

And also if we go with the last key, the prior key would need to be deleted.

I think I'll go with the latter, since I'm hesitant about changing the existing name:string API.

An alternative to a second map, would to simply scan the existing map on all setRequestHeader calls and on match, perform the split/join. This shouldn't be much of a performance hit since people generally only set 1-5 headers per request.

from xhook.

Unless there any objections, will try to get this in tonight

from xhook.

@jpillora Just to clarify, are you talking about how to support multi-value headers with setRequestHeader, e.g. setRequestHeader('foo', 1); setRequestHeader('foo', 2) should equal foo: 1, 2? I’d vote for keeping an array in case of multiple values to make the API cleaner. What does the code currently do?

Documentation: https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest#setRequestHeader()

Re: preserving case for setRequestHeader while allowing case insensitive lookup using getResponseHeader. You’re right that if we assume a small number of headers, an object traversal with normalization might be sufficient, e.g. (untested)

getResponseHeader = (name) ->
  values = (value for header, value of headers when name.toLowerCase() is header.toLowerCase())
  values.join ','

from xhook.

@gasi Yep, that will be the new getResponseHeader. The code currently:

1. Initialises underlying XHR, the facade xhr, and blank headers = {}
2. Calls to xhr.setRequestHeader just modify headers (Here is where we need the new setRequestHeader, which would find the value with a case-insensitive scan and perform the split join - instead of using an array)
3. before hooks run, passing the user the headers object for optional modification
4. Call to xhr.send causes each of headers to be set on the underlying XHR, then it is sent

The before hooks receive headers as name:value (String), since what is actually sent is just a comma-delimited string, I think we should just pass that string to the user and they can modify it as they wish. If we used an array, then all users must do if headers instanceof Array else ... first, whereas it's always a string, they can optionally split it again if they want an array.

from xhook.

Thanks for the outline—LGTM 👍 Not 100% sure about string vs array though. I can see that if headers instanceof Array else ... (BTW, I believe Array.isArray headers is safer: http://stackoverflow.com/a/4029057/125305) is cumbersome but at least the user will quickly notice if it’s a multi-valued header. Just doing a comma delimited string hides the fact that the header returned multiple values.

I know this isn’t Java, but I found Netty’s HTTP message implementation quite elegant:
http://docs.jboss.org/netty/3.2/api/org/jboss/netty/handler/codec/http/HttpMessage.html#getHeader(java.lang.String)

• setHeader to overwrite existing header (not sure this jives with W3C spec in XHR)
• addHeader to support multi-value headers.
• getHeader return first value of header.
• getHeaders return all header names and values.

Just 🍔 for 💭 😄

from xhook.