Inconsistent argument order for error event listeners

[vickvu]

Since the pull request at #741, error event listeners will be called with either (request, response, cb) or (request, response, error, cb).
For e.g. given the following code

server.on('NotFound', function(request, response, done) {
  console.log('Not found');
  done();
});

The code works fine if the route is not found and the server throw a ResourceNotFoundError. However, if a ResourceNotFoundError is returned by a controller

function handleRoute(request, response, next) {
  next(new restify.NotFoundError());
}

then the above listener will be triggered with (request, response, error, callback) (https://github.com/mcavage/node-restify/blob/master/lib/server.js#L666)

[pfmooney]

While it means breaking the added error event API in 2.8.5, I'm inclined to swap the order of the cb and err arguments dispatched to error listeners. This would restore the correct behavior for router-generated events while still enabling handlers similar to what @yunong originally proposed.

I've staged a5c5d7b for review.

Thoughts?

[vickvu]

We can also put err into request or response object and keep the original order request,response,cb

[yunong]

@pfmooney Thanks for staging a potential fix. I'd rather not buck node's convention of the last argument being the cb and swap the cb and err args. In this case since we're listening for an error event, the error object should be a first class object. Hijacking it onto either the res or req object doesn't seem like a good fit. We could change the API to one that returns a context object, and a cb like so:

({req: req, res: res, err: err, someFutureObj: foo}, fb)

But this seems like an even bigger breaking change. I'm inclined to leave the API is and just make this breaking change to the API.

@vickvu my apologies for not realizing this was an issue before.

[pfmooney]

@yunong I agree that we shouldn't be hijacking req/res to pass the err object.

If we're accepting the break in API, I suggest that we pass null err arguments when emitting errors from the router so that the function signature is the same. It makes the behavior more consistent and we can call it out specifically as a breaking change.

[yunong]

@pfmooney That's a great idea.

[pfmooney]

I'll refactor my branch to that effect.

@mcavage Care to weigh in?

[yunong]

@pfmooney Actually we should probably just pass back a new error from the router.

[pfmooney]

@yunong We can just use the err object passed to emitRouteError.

[vickvu]

thanks @pfmooney and @yunong for fixing this 👍

[mcavage]

@pfmooney +1 for passing a standard err object and just nulling it when things are good. That's what everyone would expect - this was a new feature that I doubt many people are using so now's the time to break it.