Skip to content

Loading…

Normalize `res.setHeader` capitalization #12

Merged
merged 1 commit into from

4 participants

@davidmurdoch

No description provided.

@ngryman
H5BP member

Hi @davidmurdoch.

You are right, we should normalize headers. But I'm not fery found of captitalizing them.
According to the RFC:

Each header field consists of a name followed by a colon (":") and the field value. Field names are case-insensitive.

Like isaacs I tend to prefer them to be lower-cased as it's easier to read, type and compare with.

Could you submit a lower-cased version of your PR?

Thanks!

@mathiasbynens
H5BP member

Could you submit a lower-cased version of your PR?

If you want lowercased headers, just don’t pull in the PR :) They’re lowercased already.

@davidmurdoch

@mathiasbynens, not all of them were, hence the normalization. :-p

@ngryman, this is what I'm hoping to "fix": http://bit.ly/YtxV0C. About half of the headers sent by node are not set by h5bp explicitly. Using lowercase for all headers set by h5bp would lead to further denormalization.

FYI: node uses uppercase (well, titlecase, really) when setting headers internally: https://github.com/joyent/node/blob/master/lib/http.js#L1334

@ngryman
H5BP member

Well, we are speaking about flavor here, not an issue AFAIK.
My flavor is lowercase :)

I feel confortable w/ mixing lower-cased header w/ title-cased ones.
In fact, this happens on many websites (https://www.amazon.com, http://www.nytimes.com, http://www.flickr.com, ...).

h5bp is meant to be loose coupled with other middlewares, if we choose lowercase and they choose titlecase, so be it!
nodejs API setHeader/getHeader makes working with headers case agnostic (is my sentence right in english? :)

So, until we spot a really annoying bug related to this, I think we will keep it that way.

@mathiasbynens you opinion about this?

@davidmurdoch

@ngryman, Your English is good enough for me. :-)

The point I was trying to make is that if you want to keep this in line with what node.js core actually does internally (in the http.js module), you'd use TitleCase header names. The image I attached in the previous comment was running the h5bp node server config as a static server with zero other dependencies or middleware (not even express.js).

I personally agree that lowercase does look better in the h5bp.js code; however, the nature of the project is such that a developer using this module should never have to look at the code. So, I feel that normalization to TitleCase would be better for developers who often do need to look at response headers; the mixed casing adds a bit of distraction and makes reading more difficult (at least for me).

</bikeshed>

Alternatively, you we could just do something like this:

/**
 * Patch http's `setHeader` method to force lowercase header names in its response.
 */
require('http').OutGoingMessage.prototype.setHeader = (function( old ) {
    return function( name, key ) {
        old.apply(this, arguments);
        this._headerNames[ key = name.toLowerCase() ] = key;
    };
}( require('http').OutGoingMessage.prototype.setHeader ));

kidding! :-p

@xonecas
H5BP member

If we want to do this, why not as an option to h5bp to normalize headers with possible values of lowercase, uppercase or titlecase?

@ngryman
H5BP member

@davidmurdoch You have some good points.
Some users may not understand why there are mixed cased headers and think this is bad (even if it's not really). So, as the majority is used to TitleCase, and I tend to be agree w/ that, it should reflect the common usage.

I have also read that some incorrectly configured cache proxies could fail on lowercased headers...

Merging :)

@ngryman ngryman merged commit 7482938 into h5bp:master

1 check passed

Details default The Travis build passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Mar 20, 2013
  1. @davidmurdoch
Showing with 7 additions and 7 deletions.
  1. +7 −7 lib/h5bp.js
View
14 lib/h5bp.js
@@ -125,7 +125,7 @@ h5bp = module.exports = function(options) {
// Rewrite "www.example.com -> example.com".
if (false === options.www && RE_WWW.test(host)) {
- res.setHeader('location', '//' + host.replace(RE_WWW, '') + url);
+ res.setHeader('Location', '//' + host.replace(RE_WWW, '') + url);
return respond(301);
}
@@ -137,7 +137,7 @@ h5bp = module.exports = function(options) {
// subdomains for certain parts of your website.
if (true === options.www && !RE_WWW.test(host)) {
- res.setHeader('location', '//www.' + host.replace(RE_WWW, '') + url);
+ res.setHeader('Location', '//www.' + host.replace(RE_WWW, '') + url);
return respond(301);
}
@@ -270,7 +270,7 @@ h5bp = module.exports = function(options) {
cc += (cc ? ',' : '') + 'no-transform';
// hack: send does not compute ETag if header is already set, this save us ETag generation
- res.setHeader('cache-control', '');
+ res.setHeader('Cache-Control', '');
/**
* ETag removal
@@ -281,14 +281,14 @@ h5bp = module.exports = function(options) {
// developer.yahoo.com/performance/rules.html#etags
// hack: send does not compute ETag if header is already set, this save us ETag generation
- res.setHeader('etag', '');
+ res.setHeader('ETag', '');
// handle headers correctly after express.static
res.on('header', function() {
- res.setHeader('cache-control', cc);
+ res.setHeader('Cache-Control', cc);
// remote empty etag header
- res.removeHeader('etag');
+ res.removeHeader('ETag');
});
/**
@@ -308,7 +308,7 @@ h5bp = module.exports = function(options) {
// TCP-expression. Be aware of possible disadvantages of this setting. Turn on
// if you serve a lot of static content.
- res.setHeader('connection', 'keep-alive');
+ res.setHeader('Connection', 'keep-alive');
/**
* Cookie setting from iframes
Something went wrong with that request. Please try again.