Allow middleware after response has been sent #896

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
2 participants

For doing post-response processing, this seems useful since it allows easier separation of concern (see example below).

If not merged, would you have an idea on how to better solve this? Seems like it's actually an issue noted in the commit that added the check for sent headers: 91edd0b

Not sure if this breaks BC. I don't think so, since it should only allow to call next() after a response has been ended whereas before, calling next() would throw.

Example use case would be something like this:

var app = connect();

app.use(router);

app.use(function(req, res, next) {
    console.log('More things to do for each request/response.');
});
Contributor

jonathanong commented Sep 15, 2013

why? why not just listen to the res.on('finish') event or something?

This forces to mix the "after response sent middleware" with the middleware that sends the response, right?

Seems less readable to me also, since now, the logging middleware uses a different syntax than all the other middleware: it's called inside of another middleware instead of using app.use(), like:

app.use(function(req, res, next) {
    router(req, res, next);
    res.on('event', function() {
        log(req, res);
    });
});

// vs

app.use(router);
app.use(log);

Also, it's one more callback where if it throws, it is not handled by Connect's error handler, so re-implementing custom error handling code is necessary, no?

There seems to be only one available event on a http.ServerResponse but it doesn't seem appropriate: http://nodejs.org/api/http.html#http_event_close_1

Contributor

jonathanong commented Sep 15, 2013

well... first, having your logging logic affect the main app is terrible for a user experience. i would handle all those errors separately. users don't want to see a 500 Internal Server Error because you logged something incorrectly. also, if your logging logic fails, you can let it fail, whereas with the app, you can not.

for http.ServerResponse, it inherits from a readable stream, so it has those events in addition to the close event. you'll have the finish event when the response has been entirely written to the client. even better, you can add your own custom events!

while the way you wrote it is unreadable, if you do a logging middleware correctly, it will make much more sense and be totally separate from the main logic of your app.

app.use(function mycustomlogger(req, res, next) {
  var times = {
    start: Date.now()
  }

  req.on('get user', function () {
    times.user = Date.now()
  })

  req.on('get post', function () {
    times.post = Date.now()
  })

  res.on('header', function () {
    times.header = Date.now()
    times.responseTime = times.header - times.start
  })

  res.on('finish', function () {
    times.finish = Date.now()
    times.totalTime = times.finish - times.start
    db.insert(times, function (err) {
      if (err)
        console.error(err.stack)
    })
  })

  next()
})

app.use(function getuser(req, res, next) {
  User.get(req.session.userid, function (err, user) {
    if (err) 
      return next(err)

    req.user = user
    req.emit('get user', user)
    next()
  })
})

on the contrary, adding more middleware after the response has ended makes much less sense to me

Contributor

jonathanong commented Sep 15, 2013

if you don't think this way is better, then let me know. the problem with this pull request is that it changes the core functionality of connect without benefit. we may unintentionally break other people's apps.

That makes sense. Thanks for taking the time to reply.

jonathanong referenced this pull request in expressjs/express Oct 15, 2013

Closed

question regarding 404 response in connect proto #1778

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment