Get all headers 3992#170
Get all headers 3992 #170
Uh oh!
There was an error while loading. Please reload this page.
Conversation
iankronquist commented Dec 16, 2014
Github doesn't automatically link to issues in the node.js repo. Here's a link to the issue in question: |
doc/api/http.markdown Outdated
There was a problem hiding this comment.
I think there might be two too many newlines here.
iankronquist commented Dec 16, 2014
@chrisdickinson Fixed documentation formatting |
lib/_http_outgoing.js Outdated
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).
iankronquist commented Dec 20, 2014
@chrisdickinson Fixed according to your advice. |
doc/api/http.markdown Outdated
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.
iankronquistforce-pushed the getAllHeaders-3992 branch from 9a2367e to 5bd1f5eCompareiankronquist commented Dec 21, 2014
I made changes according to @phpnode's and @bnoordhuis' comments. I then rebased to keep up with v0.12. r? |
chrisdickinson commented Dec 21, 2014
Apologies for dragging my feet on this a bit: this LGTM. If there are no objections I'll merge tomorrow afternoon (by 5PM PST). |
iankronquist commented Dec 21, 2014
@chrisdickinson No worries, I'm glad I could pitch in. |
iankronquist commented Dec 27, 2014
@chrisdickinson Can this be merged? |
iankronquist commented Dec 30, 2014
ping @chrisdickinson |
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?
iankronquist commented Jan 6, 2015
@chrisdickinson updated according to your comments. |
iankronquist commented Jan 14, 2015
ping @chrisdickinson |
There was a problem hiding this comment.
varheaders;varrando=Math.random();varexpected=util._extend({},{'X-Random-Thing': rando,/* other values */});varserver=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();});});
rvagg commented Jan 27, 2015
@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/. |
chrisdickinson commented Jan 27, 2015
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. |
iankronquist commented Jan 27, 2015
@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
iankronquistforce-pushed the getAllHeaders-3992 branch from 8b9c94a to 427fd00Compareiankronquist commented Jan 27, 2015
... 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.
Use TypedArray::kMaxByteLength instead.
9 participants
Thanks @chrisdickinson for the pointer to this bug.