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

Second middleware stack #2255

Closed
rlidwka opened this issue Jul 21, 2014 · 19 comments
Closed

Second middleware stack #2255

rlidwka opened this issue Jul 21, 2014 · 19 comments

Comments

@rlidwka
Copy link
Member

rlidwka commented Jul 21, 2014

Just sharing my thoughts about res.send() mentioned in #2249 (comment)

This is just an idea, and it has a lot of flaws as is, but maybe somebody would find it useful.

So, right now res.send() terminates middleware stack. next() handler is not called, so no middlewares won't be executed. Well, except for those who monkey-patches res.send() of course, but people usually see it as a hack.

Currently all middlewares are supposed to be executed before result of the request is known.

Here is a typical express application with a few middlewares.

app.use(bodyParser)
app.use(etagify)
app.use(compress)
app.use(function convert_result_to_yaml(req, res, next) {
    // I'm typing this from memory and realizing that I
    // remember this pattern a lot better that I would like to
    var _send = res.send
    res.send = function(data) {
        res.send = _send
        data = YAML.stringify(data)
        res.send(data)
    }
})
app.get('/', function(req, res) {
    res.send({hello: 'world'})
})

Please note that etagify, compress and that weird custom formatter are usually only overriding res.send() and their main work is done after res.send is executed.

And because of the way how they work, etagify is specified before compress, but it is actually called after it.

Idea: res.send(data) could pass the data to a second middleware stack. This stack would know that the result of the request is ready and perform some operations over it.

app.use(bodyParser)
app.get('/', function(req, res) {
    res.send({hello: 'world'})
})
app.final(function convert_result_to_yaml(req, res, data, next) {
    // `data` would be an object/string/whatever res.send() is called with
    //
    // note changed signature of this and next functions
    next(null, YAML.stringify(data))
})
app.final(compress)
app.final(etagify)

Which some people might find more clear.

Note that real-world middlewares are much more complex because of the streaming, and I don't know how to deal with this yet. Well, maybe do res.send(Stream) and make use of transform streams.

@jonathanong
Copy link
Member

this is way too complicated and would require rewriting all the middleware. at this point, you might as well use another framework like koa that doesn't have this issue

@tlivings
Copy link

Another way to handle it could be through emitting more events. Hapi does something like this, allowing you to write hooks for postResponse for example.

@aredridel
Copy link
Contributor

The other alternative is to allow a middleware to replace req and res, letting them be made into transform streams or other interposed constructs.

@jonathanong
Copy link
Member

part of the beauty of express middleware (at least the ones we provide) is that you don't need express to use these middleware. you can create a pyramid of middleware(req, res, function (err) {})s if you'd like and avoid using express all together. or you can use them with connect, restify, etc. hell, use async.series() and bind all the middleware with .bind(this, req, res).

with both suggestions, you're creating a much more opinionated framework with less compatibility trying to solve a problem that shouldn't exist in the first place. the best solution is just to get rid of the problem: callbacks.

i'm also -1 on streams because streams are super broken and it's non-trivial to create a well-written stream utility/middleware.

@jonathanong
Copy link
Member

replacing req and res could be a possibility, but I can't think of an implementation that is fast and not prone to edge cases. the last thing you want is creating replacements in every single middleware.

@dougwilson
Copy link
Contributor

I am also generally -1 on this (especially since the example above is trivially implemented by a shared function), but am willing to hear a compelling argument for the added complexity.

@tlivings
Copy link

Although the interface for middleware is common I think it's too general an assumption to say all middleware is interoperable.

In any case, the intent of this thread, unless I am mistaken, is to call for control over the response pipeline in a fashion similar to that of middleware.

Personally I think this is a feature lacking in express / connect that is not simply an edge case and could benefit the community greatly.

@dougwilson
Copy link
Contributor

Another way to handle it could be through emitting more events. Hapi does something like this, allowing you to write hooks for postResponse for example.

Node.js core events do not stack, so it would not work for the use-case in the OP.

The other alternative is to allow a middleware to replace req and res, letting them be made into transform streams or other interposed constructs.

This will lead to issues caused by things like unity and Phusion Passenger: they'll implement res/req mostly, but not quite and things would just be broken. The only solution would be to not pass Node.js core req/res at all and instead a completely abstracted version.

I think it's too general an assumption to say all middleware is interoperable.

We are specifically talking about the middleware we provide. They don't even use express/connect in their tests. The current pattern is completely interoperable, though it doesn't mean people have to make their middleware that way (read: they are lazy).

@aredridel
Copy link
Contributor

Replacing req and res would be pretty easy to extend the interface to accommodate:

function (req, res, next) { 
    // do things
    next(req, new ProxyRequest(res).pipe(res));
}

res is a stream with some properties. It's really not hard to emulate, extend or wrap the interface, especially given a working one. I think the only reason Passenger broke them so badly was a poorly managed update to streams2 API. Now we have core modules that do the hard part of streams, the buffering.

@dougwilson
Copy link
Contributor

res is way more than stream just a stream. You have to emulate every single aspect of OutgoingMessage in Node.js HTTP core, like setHeader, getHeader, headersSent, etc.

@aredridel
Copy link
Contributor

Yeah, or delegate to the original. Certainly not the most elegant thing in the universe, but not a collossal hack.

@dougwilson
Copy link
Contributor

The following is the public interface of OutgoingMessage you would need to emulate:

writable property
chunkedEncoding property
sendDate property
finished property
socket property
connection property
statusCode property
headersSent property
setTimeout method
destroy method
setHeader method
getHeader method
removeHeader method
write method
end method
addTrailers method
writeContinue method
writeHead method
writeHeader method
close event
aborted event
error event
end event
timeout event
drain event

I'm sure I'm missing things. This also does not include all the possible signatures to the methods.

@dougwilson
Copy link
Contributor

Also, @aredridel I'm going to extract a utility as a module for the community where you can give a res and a transform stream and the transform stream would be injected into res directly, without changing the res reference and thus working 100% with the current middleware. Wouldn't this work here without all the caveats?

@dougwilson
Copy link
Contributor

The module also forwards back-pressure, etc.

@aredridel
Copy link
Contributor

That'd do the job. Ugly in the same ways, inside-out. Clever, too.

@dougwilson
Copy link
Contributor

@aredridel you're welcome you subscribe to expressjs/discussions#301 to know when/were it lands.

@tlivings
Copy link

That would be awesome.

@defunctzombie
Copy link
Contributor

I don't think this is something we should be pushing for right now.

@rlidwka
Copy link
Member Author

rlidwka commented Aug 22, 2014

Actually, thinking about it... We do have second middleware stack. That's error-handling middlewares.

Are there any side-effects for re-purposing that one for general usage, not just for errors?

I mean, the code above could be rewritten like this:

app.get('/', function(req, res, next) {
    next({hello: 'world'}) // this would be response, not an error
})

app.use(function(body, req, res, next) {
    if (body instanceof Error) return next(body)
    next(YAML.stringify(body))
})

app.use(function(body, req, res, next) {
    if (typeof(body) === 'string') {
        res.header('content-encoding', 'gzip')
        body = gzip(body)
    }
    next(body)
})

... and the funny thing is that it's already working perfectly fine. I wonder why I didn't notice that before. :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants