Get all headers 3992 #170
Get all headers 3992 #170
Conversation
|
Github doesn't automatically link to issues in the node.js repo. Here's a link to the issue in question: |
| var headers = response.getAllHeaders(); | ||
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
I think there might be two too many newlines here.
|
@chrisdickinson Fixed documentation formatting |
| if (!this._headers) | ||
| return; | ||
| else | ||
| return this._headers; |
There was a problem hiding this comment.
It would be nice to return a copy of headers instead of the full object -- something like util.extend({}, this._headers).
|
@chrisdickinson Fixed according to your advice. |
| @@ -380,6 +380,15 @@ Example: | |||
|
|
|||
| var contentType = response.getHeader('content-type'); | |||
|
|
|||
| ### response.getAllHeaders() | |||
|
|
|||
| Reads out all headers that are already been queued but not sent to the client. | |||
There was a problem hiding this comment.
Should probably be something like:
Returns the headers which have already been queued but not yet sent to the client.
Test a getAllHeaders function. It should return an object representing the headers in a request.
iankronquist
force-pushed
the
getAllHeaders-3992
branch
from
9a2367e
to
5bd1f5e
Compare
|
I made changes according to @phpnode's and @bnoordhuis' comments. I then rebased to keep up with v0.12. r? |
|
Apologies for dragging my feet on this a bit: this LGTM. If there are no objections I'll merge tomorrow afternoon (by 5PM PST). |
|
@chrisdickinson No worries, I'm glad I could pitch in. |
|
@chrisdickinson Can this be merged? |
|
ping @chrisdickinson |
| } | ||
| for (var field in expected) { | ||
| assert.equal(headers[field], expected[field]); | ||
| } |
There was a problem hiding this comment.
Sorry for the late comment, but wouldn't this omit the added-by-default headers Date and Connection?
|
@chrisdickinson updated according to your comments. |
|
ping @chrisdickinson |
| }); | ||
| response.resume(); | ||
| }); | ||
| } |
There was a problem hiding this comment.
var headers;
var rando = Math.random();
var expected = util._extend({}, {
'X-Random-Thing': rando,
/* other values */
});
var server = http.createServer(function(req, res) {
res.setHeader('X-Random-Thing', rando);
headers = res.getAllHeaders();
res.end('hello');
assert.strictEqual(res.getAllHeaders(), null);
});
server.listen(common.PORT, function() {
http.get({port: common.PORT}, function(resp) {
assert.deepEqual(response.headers, expected);
server.close();
});
});
|
@chrisdickinson do you require further action on this? Is your last comment a request for a test case along the lines of your code block? @iankronquist if this was to be merged it'd probably need to be a single commit so you might want to squash. The license block should also be removed from the test file and it now needs to go in to test/parallel/. |
|
Yes, that last comment is the test case that should be added and made to pass for this to be able to go in. Re: commits: I don't mind squashing those myself on merge. |
|
@rvagg I'll go ahead and do that. |
Users should not have to access private attributes like res._headers in order to get all the headers in a request. Define a method getAllHeaders to return an object which is a copy of re._headers Fixes issue nodejs#3992
iankronquist
force-pushed
the
getAllHeaders-3992
branch
from
8b9c94a
to
427fd00
Compare
|
... and squashed like a bug. Let me know if you need anything else cleaned up. |
Use TypedArray::kMaxByteLength instead.
Use TypedArray::kMaxByteLength instead.
Use TypedArray::kMaxByteLength instead.
Thanks @chrisdickinson for the pointer to this bug.