Skip to content
This repository

improved http.ServerResponse API #1448

Open
mikeal opened this Issue · 21 comments

6 participants

Mikeal Rogers Isaac Z. Schlueter Christopher Jeffrey Marco Rogers TJ Holowaychuk Mark Nottingham
Mikeal Rogers

We have a long thread about this here:

http://groups.google.com/group/nodejs-dev/browse_thread/thread/f9ddf97342f3d5ff#

The gist of it is:

Right now we kind of have two totally seperate APIs for dealing with setting the status code and headers and sending them.

One is to do

response.statusCode = 200
response.setHeader('content-type', 'text/plain')
response.end('hello world') || fs.createReadStream('hello.txt').pipe(response)

The other is:

response.writeHead(200, headers)
response.end('hello world') || fs.createReadStream('hello.txt').pipe(response)

In the first example we don't actually send the statusCode and headers until the first response.write() call.

In the second example, which is the older API, we send them immediately.

writeHead() also supports a 2d array of headers, which bails out of the other API entirely causing some inconsistency problems.

Here is my proposal for a new API.

http.ServerResponse ::

.statusCode // integer
.headers // 2d array
.setHeader(key, value)
.getHeader(key)
.setHeaders(object)
.headersSent // boolean
.sendHeaders() // sends StatusCode and headers
.writeHead(status, headers) // reverse compat, sets StatusCode, calls setHeaders and sendHeaders().

This way, writeHead just becomes a shorthand and maintains the same API it does now, but it's no longer inconsistent with the rest of the API.

This is also better support for 2d arrays as headers. You can set .headers to whatever you want because it's what getHeader/setHeader actually operate on and use. You can also just modify it in place if that's simpler. And now when you do use 2d arrays you don't bail out of the normal API flow.

We can also replace the instance method of setHeader/setHeaders/writeHead/sendHeaders after sendHeaders is called to a method that throws.

Christopher Jeffrey
chjj commented

In the second example, which is the older API, we send them immediately.

I think .writeHead actually does buffer the headers until the first .write. The only difference is, they're rendered (or compiled) using ._renderHeaders before the first .write/.end call. I remember this was a major problem when writing a sendfile implementation for http, there is no way to forcefully send the headers. Not even .writeHead does it.

Mikeal Rogers

well then, i guess whether or not we call sendHeaders() in writeHead() is discretionary. it certainly won't break backwards compatibility.

Marco Rogers

I'm still +1 on this.

TJ Holowaychuk

pluralization of header kinda bugs me a bit but other than that <3

Mikeal Rogers

actually,@ry expressed concern with using "send"

maybe:

.writeHeadNow()
.headWritten

??

TJ Holowaychuk

hmMmM .flushHead()? not sure

Mikeal Rogers

flush means something else when we're talking about socket writes so i'd rather not step on the verbiage there either.

TJ Holowaychuk

yeah i guess it's not really a flush. what didn't ryan like about send?

Mikeal Rogers

something about getting "send" out of the API and only calling things "write"

TJ Holowaychuk

only my personal opinion since I'm not sure of all the internals 100% but I would expect writeHead to be immediate while the progressive res.setHeader() etc obviously wouldn't but maybe writeHead() without args could force that, not sure I can't think of anything else off hand

Marco Rogers

@visionmedia I think writeHead would still be immediate. It calls sendHeaders at the end. I'm not sure what the question is. Just the naming of sendHeaders? I think that was a bikeshed point a long time ago when this came up. Why not writeHeaders? It fits nicely, it's just too close to writeHead. But aren't we going to deprecate and remove writeHead eventually?

TJ Holowaychuk

@polotek oh maybe, I thought it buffered / deferred or something. I find writeHead pretty tough to read especially if we add res.setHeaders(obj) it becomes a lot less necessary, but I imagine it will be around for quite some time if not forever, but we it could use the progressive api internally instead of the wacky combo we have internally right now.

Marco Rogers

Yes it's old and venerable in terms of node's history. But we're talking about pretty much obsoleting it with these changes. And we want people to use the new API. It should be back compat for a while, and deprecated for longer probably, but IMO writeHead should eventually go away. But that's not my call obviously.

Mikeal Rogers

let's get on the same page about what we're trying to do.

i think the place we all want to get is a place where people use setHeader() and set the statusCode property.

the point of this change is to get the legacy API that we need to continue to support to the point where it's just a simple shim and doesn't break entirely from the newer API.

so, ideally, @visionmedia will never call writeHead(). he'll have his framework setting headers and the statusCode and he'll call writeHeadNow() [or whatever we name it] or just wait for a write() or end() call to happen some time in the future.

Marco Rogers

Yes, my understanding is we're reimplementing writeHead with the new API to provide back compat. Then we can start weening people off of writeHead. Ideally you wouldn't even worry about when the headers were sent, you would just start writing and it'll get handled for you. Otherwise you should be able to replace all calls to write head with something like the following.

// writeHead(status, headers)
res.statusCode = status;
res.setHeaders(headers);
res.sendHeaders();

And once this works as expected and passes all tests, it's simple to kill writeHead because third party devs can easily write their own version. The only thing missing is optionally setting a custom reason phrase on the status header. Did that get addressed at all?

Mark Nottingham

What is the 2d array of?

E.g., what's the JSON for this set of headers:

Foo: 1
Foo: 2
Bar: a, b
Bar: c
Baz: m, n, o
TJ Holowaychuk

I dont mind our current Bar: ['a', 'b', 'c'] / Baz: 'm' we just have way too many inconsistencies, that part can be easily hidden in the public API and even with it being sometimes an array I think it's easier to work with than an array at all times

Mikeal Rogers

as much as I love the "purity" of the 2d array I have a suspicion that everywhere the public API needs to manipulate the headers will be much slower iterating over a 2d array rather than the Bar: ['a', 'b', 'c'] object we have now.

what is our API for appending to headers rather than setting them outright?

TJ Holowaychuk

yeah tough call there, I was only special-casing set-cookie

Isaac Z. Schlueter
Collaborator

The focus of 0.12 will be http refactoring. Let's revisit then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.