Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

http: introduce getAllHeaders() #772

Closed
wants to merge 2 commits into from

Conversation

Fishrock123
Copy link
Contributor

Succeeds the patch proposed in #170

R=@chrisdickinson, cc @iankronquist

iankronquist and others added 2 commits February 9, 2015 13:01
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 res._headers

Fixes: nodejs/node-v0.x-archive#3992
PR-URL: nodejs#170
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Chris Dickinson <[email protected]>
This replaces the faulty test with a modified version of
test-http-header-read.
@Fishrock123 Fishrock123 added semver-minor PRs that contain new features and should be released in the next minor version. http Issues or PRs related to the http subsystem. labels Feb 9, 2015
@@ -381,6 +381,15 @@ Example:

var contentType = response.getHeader('content-type');

### response.getAllHeaders()

Reads out all headers that are already been queued but not yet sent to the
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that are should be that have

@chrisdickinson
Copy link
Contributor

The test doesn't quite cover what I'd like it to cover:

var util = require('util');
var http = require('http');
var common = require('../common');
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();
  });
});

Specifically. I'm looking for it to be able to pick up on the automatic headers as well as the explicitly set ones.

@Fishrock123
Copy link
Contributor Author

@chrisdickinson According to the previous test assert.strictEqual(res.getAllHeaders(), null); where you have it should be incorrect?

I'll update the rest though,

@iankronquist
Copy link
Contributor

@Fishrock123 Thanks for doing this. I haven't had the time recently to finish this test.

@Fishrock123
Copy link
Contributor Author

I talked extensively about this on irc with @chrisdickinson today on if and how this should return the implicit headers, and now I'm actually sure how much I like the idea of this.

I think the only way to keep it sane would be:

  • implicit headers that can be set at the start of the request are set on _headers at the start.
  • after write(), the implicit headers are also written to _headers.
  • this PR's function returns a read-only copy of just _headers
  • rename this PR's function to getAvaliableHeaders() (or anything better)

@vkurchatkin vkurchatkin mentioned this pull request Feb 21, 2015
@Fishrock123
Copy link
Contributor Author

ping @chrisdickinson, how do you feel about my proposed changes as per the above?

@chrisdickinson
Copy link
Contributor

implicit headers that can be set at the start of the request are set on _headers at the start.

Would it be possible to do a Object.defineProperty(this._headers, 'date', {get/set}) style thing at the outset, overriding it if setHeaders changes the property? If so that might solve a lot of our problems: then automatic headers would just go out as soon as they were referenced from the _headers object.

@Fishrock123
Copy link
Contributor Author

Would it be possible to do a Object.defineProperty(this._headers, 'date', {get/set}) style thing at the outset, overriding it if setHeaders changes the property? If so that might solve a lot of our problems: then automatic headers would just go out as soon as they were referenced from the _headers object.

I'm not sure I understand?

Do you mean just generate on getter? That seems less reliable.

@Fishrock123
Copy link
Contributor Author

Going to close (for now?). I'm not too confident about changing _headers-related things. Reading/Writing _headers has already been around so long it's probably here to stay.

@niftylettuce
Copy link

How can we get the Date among other automatic headers set in the response? The method res.getHeaders() does not return Date. It's been 4 years and still doesn't seem like we have an option.

@niftylettuce
Copy link

OK I have a working solution for now. See my comment here #28302 (comment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http Issues or PRs related to the http subsystem. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants