ctx.response.get - use .getHeader

[tinovyatkin]

response.header fails back to standard method response.getHeaders on Node >= 7.7 (i.e. on all live Node versions today), so, it overkill to to get all headers first, then do field.toLowercase (specially without typechecking of field).
response.getHeader is a standard method since Node 0.x and it's doing all lowercasing internally.

Additionally, coercing to empty string is invalid here, as headers may contain non-string values before being sent to network, for example, Content-Length may be a number value of 0.

[codecov]

Codecov Report

Merging #1392 (a3b8ffb) into master (6cf6a95) will decrease coverage by 0.23%.
The diff coverage is 100.00%.

❗ Current head a3b8ffb differs from pull request most recent head fe573a7. Consider uploading reports for the commit fe573a7 to get more accurate results

@@            Coverage Diff             @@
##           master    #1392      +/-   ##
==========================================
- Coverage   99.60%   99.37%   -0.24%     
==========================================
  Files           5        4       -1     
  Lines         507      479      -28     
  Branches      143      127      -16     
==========================================
- Hits          505      476      -29     
+ Misses          2        0       -2     
- Partials        0        3       +3     

Impacted Files	Coverage Δ
lib/response.js	99.36% <100.00%> (-0.64%)	⬇️
lib/application.js	98.23% <0.00%> (-0.02%)	⬇️
lib/context.js	100.00% <0.00%> (ø)
lib/request.js	100.00% <0.00%> (ø)
test-helpers/context.js

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6cf6a95...fe573a7. Read the comment docs.

[fl0w]

LGTM
Note though, defaulting to string could be a behaviour that is relied on. I'd consider this a breaking change.

[dead-horse]

it is related with #1394

Because we change all headers to string when set, so get header default to empty string is reasonable.

I'm -0 on both change even in the next major version, the current implementation is also reasonable, the new implementation does not have much benefit but introduces some incompatible changes.

[tinovyatkin]

@dead-horse new implementation do have benefits, indeed:

1. Empty string is a valid value for HTTP header (see Support empty HTTP header values jetty/jetty.project#1116 for example), and return it in case where header wasn’t set at all is an error.

2. It’s rare and nowhere documented that ctx.response.set(‘foo’, 5); 5 !== ctx.response.get(‘foo’). It’s even more confusing taking in account that ctx.res.setHeader(‘foo’, 5); 5 === ctx.res.getHeader(‘foo’). Or even:

ctx.res.setHeader('foo', 5);
ctx.response.get('foo') === 5; // true
ctx.response.set('foo', 5);
ctx.res.getHeader('foo') === 5; // false

3. Node built in response.getHeader documentation directly says that return type is the same as response.setHeader. Koa documentation says nothing about conversion to string. So, any developer relying on a fact that ctx.response.get must always return a string is relying on a bug, that may be and should be fixed in patch/minor version.

4. It's hacky, that the same Koa sourcebase uses semantic like this.header['content-length'] to avoid this coercion to empty string by .get

[tinovyatkin]

More with current implementation:

ctx.set('Content-Length', 10);
ctx.response.header['content-length'] = 0; // or ctx.res.setHeader('Content-Length', 0)
ctx.response.get('Content-Length); // => ''

[fl0w]

FYI, I think it's better to rebase the branch rather than to merge in changes (though if you rebase and squash the result will be equivalent). Just to omit some commit bloat I tend to stay clear from merge-commits if possible or squash. Others might disagree.

[tinovyatkin]

@fl0w that's the GitHub suggested way to Update branch, but anyway you can squash at merging, right?

[anlexN]

@tinovyatkin koa team seems don't like merge, can we together fork koa and fix many problem? i think you are right.