-
Notifications
You must be signed in to change notification settings - Fork 7.3k
Commit
This works for both ServerResponse and ClientRequest. Adds three new methods as a couple properties to to OutgoingMessage objects. Tests by Charlie Robbins. Change-Id: Ib6f3829798e8f11dd2b6136e61df254f1564807e
- Loading branch information
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -261,10 +261,59 @@ Example: | |
This method must only be called once on a message and it must | ||
be called before `response.end()` is called. | ||
|
||
If you call `response.write()` or `response.end()` before calling this, the | ||
implicit/mutable headers will be calculated and call this function for you. | ||
|
||
### response.statusCode | ||
|
||
When using implicit headers (not calling `response.writeHead()` explicitly), this property | ||
controlls the status code that will be send to the client when the headers get | ||
flushed. | ||
|
||
Example: | ||
|
||
response.statusCode = 404; | ||
|
||
### response.setHeader(name, value) | ||
|
||
Sets a single header value for implicit headers. If this header already exists | ||
in the to-be-sent headers, it's value will be replaced. Use an array of strings | ||
This comment has been minimized.
Sorry, something went wrong. |
||
here if you need to send multiple headers with the same name. | ||
|
||
Example: | ||
|
||
response.setHeader("Content-Type", "text/html"); | ||
|
||
or | ||
|
||
response.setHeader("Set-Cookie", ["type=ninja", "language=javascript"]); | ||
This comment has been minimized.
Sorry, something went wrong.
This comment has been minimized.
Sorry, something went wrong.
indexzero
|
||
|
||
|
||
### response.getHeader(name) | ||
|
||
Reads out a header that's already been queued but not sent to the client. Note | ||
that the name is case insensitive. This can only be called before headers get | ||
implicitly flushed. | ||
|
||
Example: | ||
|
||
var contentType = response.getHeader('content-type'); | ||
|
||
### response.removeHeader(name) | ||
|
||
Removes a header that's queued for implicit sending. | ||
|
||
Example: | ||
|
||
response.removeHeader("Content-Encoding"); | ||
|
||
|
||
### response.write(chunk, encoding='utf8') | ||
|
||
This method must be called after `writeHead` was | ||
called. It sends a chunk of the response body. This method may | ||
If this method is called and `response.writeHead()` has not been called, it will | ||
switch to implicit header mode and flush the implicit headers. | ||
|
||
This sends a chunk of the response body. This method may | ||
be called multiple times to provide successive parts of the body. | ||
|
||
`chunk` can be a string or a buffer. If `chunk` is a string, | ||
|
@@ -436,7 +485,10 @@ A queue of requests waiting to be sent to sockets. | |
## http.ClientRequest | ||
|
||
This object is created internally and returned from `http.request()`. It | ||
represents an _in-progress_ request whose header has already been sent. | ||
represents an _in-progress_ request whose header has already been queued. The | ||
header is still mutable using the `setHeader(name, value)`, `getHeader(name)`, | ||
`removeHeader(name)` API. The actual header will be sent along with the first | ||
data chunk or when closing the connection. | ||
|
||
To get the response, add a listener for `'response'` to the request object. | ||
`'response'` will be emitted from the request object when the response | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,119 @@ | ||
var common = require('../common'); | ||
var assert = require('assert'); | ||
var http = require('http'); | ||
|
||
// Simple test of Node's HTTP Client mutable headers | ||
// OutgoingMessage.prototype.setHeader(name, value) | ||
// OutgoingMessage.prototype.getHeader(name) | ||
// OutgoingMessage.prototype.removeHeader(name, value) | ||
// ServerResponse.prototype.statusCode | ||
// <ClientRequest>.method | ||
// <ClientRequest>.path | ||
|
||
var testsComplete = 0; | ||
var test = 'headers'; | ||
var content = 'hello world\n'; | ||
var cookies = [ | ||
'session_token=; path=/; expires=Sun, 15-Sep-2030 13:48:52 GMT', | ||
'prefers_open_id=; path=/; expires=Thu, 01-Jan-1970 00:00:00 GMT' | ||
]; | ||
|
||
var s = http.createServer(function(req, res) { | ||
switch (test) { | ||
case 'headers': | ||
assert.throws(function () { res.setHeader() }); | ||
assert.throws(function () { res.setHeader('someHeader') }); | ||
assert.throws(function () { res.getHeader() }); | ||
assert.throws(function () { res.removeHeader() }); | ||
|
||
res.setHeader('x-test-header', 'testing'); | ||
res.setHeader('X-TEST-HEADER2', 'testing'); | ||
res.setHeader('set-cookie', cookies); | ||
res.setHeader('x-test-array-header', [1, 2, 3]); | ||
|
||
var val1 = res.getHeader('x-test-header'); | ||
var val2 = res.getHeader('x-test-header2'); | ||
assert.equal(val1, 'testing'); | ||
assert.equal(val2, 'testing'); | ||
|
||
res.removeHeader('x-test-header2'); | ||
break; | ||
|
||
case 'contentLength': | ||
res.setHeader('content-length', content.length); | ||
assert.equal(content.length, res.getHeader('Content-Length')); | ||
break; | ||
|
||
case 'transferEncoding': | ||
res.setHeader('transfer-encoding', 'chunked'); | ||
assert.equal(res.getHeader('Transfer-Encoding'), 'chunked'); | ||
break; | ||
} | ||
|
||
res.statusCode = 201; | ||
res.end(content); | ||
}); | ||
|
||
s.listen(common.PORT, nextTest); | ||
|
||
|
||
function nextTest () { | ||
if (test === 'end') { | ||
return s.close(); | ||
} | ||
|
||
var bufferedResponse = ''; | ||
|
||
http.get({ port: common.PORT }, function(response) { | ||
console.log('TEST: ' + test); | ||
console.log('STATUS: ' + response.statusCode); | ||
console.log('HEADERS: '); | ||
console.dir(response.headers); | ||
|
||
switch (test) { | ||
case 'headers': | ||
assert.equal(response.statusCode, 201); | ||
assert.equal(response.headers['x-test-header'], | ||
'testing'); | ||
assert.equal(response.headers['x-test-array-header'], | ||
[1,2,3].join(', ')); | ||
assert.deepEqual(cookies, | ||
response.headers['set-cookie']); | ||
assert.equal(response.headers['x-test-header2'] !== undefined, false); | ||
// Make the next request | ||
test = 'contentLength'; | ||
console.log('foobar'); | ||
break; | ||
|
||
case 'contentLength': | ||
assert.equal(response.headers['content-length'], content.length); | ||
test = 'transferEncoding'; | ||
break; | ||
|
||
case 'transferEncoding': | ||
assert.equal(response.headers['transfer-encoding'], 'chunked'); | ||
test = 'end'; | ||
break; | ||
|
||
default: | ||
throw Error("?"); | ||
} | ||
|
||
response.setEncoding('utf8'); | ||
response.on('data', function(s) { | ||
bufferedResponse += s; | ||
}); | ||
|
||
response.on('end', function() { | ||
assert.equal(content, bufferedResponse); | ||
testsComplete++; | ||
nextTest(); | ||
}); | ||
}); | ||
} | ||
|
||
|
||
process.on('exit', function() { | ||
assert.equal(3, testsComplete); | ||
}); | ||
|
16 comments
on commit b09c588
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is actually kinda useless as-is right now. if writeHead() is called it should merge existing pre-defined headers, otherwise any middleware using these features will fail with connect (any many others who proxy/utilize writeHead). the veryyyy first thing i tried was res.setHeader(key,val); res.writeHead(200), if this doesnt work then its useless
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm ... looks like you're right. Do you want to put together a patch that fixes this issue? I can try my hand at it next week if you're too busy.
Now that this has landed in 0.4.0 seems logical a patch like that could go into 0.4.1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah ill whip something up
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great. Thanks TJ. The behavior imho should be to overwrite any predefined headers with the headers passed to writeHead. So
res.setHeader('x-foo', 'bar'); res.setHeader('x-bar', 'foo'); res.writeHead(200, { 'x-foo', 'bar2' }); // Writes // HTTP/1.1 200 OK // X-FOO: BAR2 // X-BAR: FOO
Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yup i agree, didnt realize you could still do writeHead(200, [[k,v]]) haha over complicates the implementation so bad, yuck
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://gist.github.com/821692
stupid arrays lol
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mmm. I didnt realize we weren't supporting the old writeHead with this. +1 on the patch. But honestly I feel like ideally writeHead would go away at some point. We would have this API to explicitly set headers, and then a sendHeaders method that took no args and just wrote the saved headers to the socket. I like the separation. And it could still be wrapped in a higher level writeHead that does what it does now. My push is usually towards explicit and useful but low level.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I purposely didn't support merging the headers from writeHead with the mutable headers because I was worried it would be too expensive. If someone proves it doesn't hurt the silly synthetic benchmarks, then it would be good to add that in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well its useless without this, but some of the internals really need a refactor, the array support in there would be far more expensive than merging but it really blows that you have to check for each case all over.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO either we need to drop writeHead() as public api all together, or we need the merging. im fine with either, but connect etc compatibility between the two (if writeHead() was removed) would be brutal
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One of the requisites for landing this in 0.4.0 was 100% backwards compatibility. So dropping writeHead() would be in the next stable release, so 0.6.0?
Hopefully the overhead of the merge doesn't affect the benchmark significantly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So how will the writeHead merge handle multiple headers? If it's an array then append, otherwise overwrite? There's some nuance here maybe.
res.setHeader('set-cookie', 'single-value');
res.writeHead(200, { 'set-cookie': ['more', 'cookies']});
Is single-value added to the array? Or does the array overwrite the single-value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah the array handling is pretty... lame, we need to address that at the setHeader() level as well. as we were talking about earlier it would probably be best to white-list common use-cases and then add a bool for the others
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've got more thoughts but let's wait until indexzero pushes this to the mailing list. Other folks can give opinions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
its already in node, my patch there adds the merging so it is at least somewhat useful for now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Distilled this down into a new thread on the mailing list: http://groups.google.com/group/nodejs-dev/browse_thread/thread/64469cc3aa9077
It's its, not it's, unless you mean 'it is', otherwise it's its.