Deprecate arity check in favor of explicit error handlers

[wesleytodd]

This is a first go at the idea discussed in https://github.com/expressjs/express/issues/2896

It deprecates the router.use and route.<method> use cases with a 4 argument handler function in favor of explicitly having router.error and route.<method>Error.

Thoughts?

[dougwilson]

Nice! One thing I'm not sure about is the migration path for people with error handlers in the middle of a bunch of handlers. For example app.post('/login', [preLogin, login, postLogin]) where preLogin and postLogin are arrays of functions, some of which are error handers. Usually I see this come from the pattern of trying to build up pre-routes like preLogin = [urlencodedBody, sesson] and session = [loadSession, sessionErrorHandler], etc.

[wesleytodd]

Yeah, that would be broken with this approach for sure. Is that something we could be alright with? The change they would have to make is to break those apart into multiple calls:

app.use(loadSession)
app.error(sessionErrorHandler)

[dougwilson]

Maybe. Of course, your example would now mean that session is going to initiate for all the routes instead of only the necessary ones, meaning this will likely start making all middleware out there build in route white-lists to get it working back like it used to, right? No need to hit the database to load sessions when the session is not necessary, right?

[wesleytodd]

You could also do those route chains with router instances:

var r = new Router();
r.use(loadSession)
r.error(sessionErrorHandler)

app.use('/some-route', r)

[dougwilson]

So the idea is that every route in the app is a new router instance? That would work for sure, I guess :) Certainly need to somehow understand the impact of removing a feature if it has no replacement. Certainly removing app.use(4 args) for app.error(4 args) is obvious, but the others less so without a replacement, or a migration guide for how these apps should get rewritten.

[wesleytodd]

So the idea is that every route in the app is a new router instance?

Not desirable for sure. Technically it would only be necessary for the uses where you want to mix error handler middleware in the middle. Maybe we could document and improve the Route interface a bit more since really all you need is:

var r = Route('/')
r.all(/* ... */)
r.error(/* ... */)

app.use(function (req, res, next) {
  r.dispatch(req, res, next)
})

We could make it so you can just pass a route like app.use(route)?

migration guide for how these apps should get rewritten

If you like the general direction here, I can write an update guide for sure.

[dougwilson]

That could work, but it'll still work on all methods instead of just the POST method (and just the /login path) as the example. Also, router.route('/') would return what you're looking for without the extra app.use in your example.

[wesleytodd]

I was just meaning that it is lighter weight than a whole router instance and can still, with only small modifications, suit the needs of grouping a set of middleware which includes error handling middleware together. Sorry my example was only meant to show how close it already is.

[wesleytodd]

Oh, and I forgot to mention that this PR added a bunch of other methods for handling errors on only one method:

route.postError(function (err, req, res, next) {})
route.getError(function (err, req, res, next) {})

[wesleytodd]

How do we feel about these changes? It will remove some use cases which are possible currently, but I think the cost of the arity check is worth losing these. I think that us taking a more hard line to improve the past api mistakes is a good thing. And in this case, the proposed api seems better in the long run.

One other thing I noticed when re-going over this old PR. The example about making each route a new router instance is not what I intended. With this PR you could do:

var r = new Router();
r.route('*')
  .use(startSession)
  .postError(customHandlingWhenPOSTing)
  .error(sessionErrorHandler)

Which I think better solves your concerns. I don't know why I didn't realize that was the better solution to the above questions.

[czaarek99]

So what's the update on this issue? This not being merged is stalling the progress on #2896 in express.

Will this be included in express 5.0?

[wesleytodd]

Obviously as the author of this I think this is a good direction to move, but I have not received any further feedback from @dougwilson or any of the other core maintainers. I would like to push some of these bigger breaking changes forward so that people know about them and have time to update before 5.0 drops, but I need either the ability to merge/publish these or some feedback on what is holding this up.

[dougwilson]

I feel like the functionality you're removing is very useful, having seen many apps taking advantage of it. I think to move this forward maybe we need a solution that (1) doesn't remove so much functionality like inline error handlers and (2) passes the CI.

[wesleytodd]

The code in this PR provides an alternative (as shown above) with just a bit more code than previously. Is that not a satisfactory way?

And I can fix the ci for sure, just didn't want to do it if we didn't agree on the actual feature.

[wesleytodd]

I guess it does remove the version where you package them up in an array. So it is not exactly the same, but again, I argue that this loss is VERY worth it for the gain of not having the arity check.

[dougwilson]

Well, at the very least, we can't just remove something and not explain to folks how to migrate, or no one will know how to upgrade. I don't think it should be removed, but maybe if you can write up a migration guide on what folks are supposed to do instead that may help convince me otherwise.

Here is what I see a lot of, to give you an example to work off-of:

app.put('/foo/:id', authnMiddleware, authzMiddleware, parseMiddleware, loadFooMiddleware, putFooHandler)

// ... where the above is generally the following
authnMiddleware = [loadSessionMiddleware, sessionLoadErrorHandler]
authzMiddleware = [validatePermissions, permissionErroHandler]
parseMiddleware = [parsePayload, payloadErrorHandler]
loadFooMiddleware = [loadObjectMiddleware, loadErrorHandler]

[dougwilson]

Also, to be clear I'm not arguing against the removing the the arity check -- I'm arguing against removing the ability to have these inline, mixed error handlers.

[wesleytodd]

Do you have any ideas on how to keep this feature but also remove the arity check? That is the problem I am having, I cannot think of a clean way to have both.

[wesleytodd]

We could introduce a new api which wraps error handlers, something like:

app.put('/foo/:id', [authnMiddleware, app.errorHandler(authnErrorHandler)])

Or we could add a property:

authnErrorHandler.isErrorHandler = true
app.put('/foo/:id', [authnMiddleware, authnErrorHandler])

What other types of solutions are there?

[wesleytodd]

I had one other idea for this last night. What if we expose a new abstraction, a Stack, which would be a very light weight wrapper around a set of middleware. It would expose just .use and .error. It would provide the same ability as the array of middleware has now in that it allows for arbitrary grouping of middleware, including error handlers, but would not incur the cost of creating a sub-router for each reusable group. Here is an example usage:

var auth = new Stack([session, permissions], authError)
var body = new Stack(parseBody, parseBodyError)

// Or add them with use/error
// var auth = new Stack().use([session, permissions]).error(authError)

// Maybe expose it on router?
// var auth = router.stack(...)

app.put('/', auth, body)

This avoids the loss in features but does introduce a new abstraction (which has learning and support downsides). I am really trying to think of the best solution, because I agree we don't want to loose functionality, but this arity check really needs to go lol. What do you think of this idea?

[dougwilson]

Yea, I'm following. That does make sense: basically move away from a basic array with functions in it to an object that understands the difference. Not unlike a Promise object with .then and .catch actually.

[wesleytodd]

Exactly, that is what I was relating this idea to as well. Does this sound "promising" (pun intended) enough to build a PR around? IMO it is better than the other proposals I have above despite the added complexity of a new abstraction.

[wesleytodd]

Ok, I was thinking more about what this could look like, and this is what I came up with:

var flatten = require('array-flatten')
var slice = Array.prototype.slice

class Stack extends Array {
    constructor (handlers, errorHandlers) {
        super()
        if (handlers && handlers.length) {
            this.use(...handlers)
        }
        if (errorHandlers && errorHandlers.length) {
            this.error(...errorHandlers)
        }
    }

    use () {
        this.push(...flatten(slice.call(arguments)))
    }

    error () {
        this.push(...flatten(slice.call(arguments)).map((fnc) => {
            return ErrorHandler(fnc);
        }))
    }
}

function ErrorHandler (fnc) {
    Object.setPrototypeOf(fnc, ErrorHandler.prototype);
    return fnc;
}

var s = new Stack([() => {}], [function errH () {}]);
s.use(function bar () {});
s.error(function err () {});
s.forEach((f) => {
    if (f instanceof ErrorHandler) {
        console.log('err', f);
    } else {
        console.log('use', f);
    }
})

What do you think? The stack inherits from array, so all the other code could remain the same, and for error handlers all we need is to pass them to the ErrorHandler function which swaps in the prototype for the instance of check later. If we put this into the router I think we would want to expose it as Router.Stack but not expose ErrorHandler because that should be a transparent implementation detail.

[LinusU]

@wesleytodd that's nice, I was actually thinking of something similar just the other day, but using Symbols instead of the prototype.

const kErrorHandler = Symbol('error-handler')

function ErrorHandler (fnc) {
    fnc[kErrorHandler] = true
    return fnc
}

I guess that both would allow us to be backward compatible? That would be a really big plus in my opinion since we can then add this to Express.js 4.x.

We just need to change the arity check to also look for the symbol/prototype:

- if (handler.length === 4) {
+ if (handler.length === 4 || handler[kErrorHandler]) {

One con with the prototype is that there might already be another prototype on the passed function (highly unlikely though, right? 🤔)

One con with Symbol is that it might not work in all Node.js versions we support, so we would have to fall back to a non-enumerable property.

I'm happy with either one 👍

[wesleytodd]

One con with Symbol is that it might not work in all Node.js versions we support

It would be in what we plan to support in 5.x, it is supported since 0.12. If so, then I really like your idea better. I think now is the time to throw this into a PR. I will start on that this week/weekend!

[dougwilson]

I haven't read the above yet, but just wanted to chime in on the 5.x thing: there is pretty much no reason it would support anything below Node.js 4 at this point 🤣

[wesleytodd]

Ok, I pushed a version which is compatible with the idea above. The additions to support a new Stack abstraction were small because I kept the main implementation as a separate module. You can see that module here: https://github.com/wesleytodd/mwstack

It is late for me (past 12 lol, I need to sleep), so I didn't finish adding the polish to the docs and stuff, but I do have the tests running in node>=4 and will add the rest later. Welcoming any feedback on this!

[wesleytodd]

Ok, I updated things on that repo. All that is left is moving the repo. @dougwilson do you have any other changes you want on that repo? If not can you grant me publish rights on the @pilarjs npm org so I can publish the 1.0.0 version?

As for this PR, I added docs and the history entry and fixed all the deprecation warnings which were a result of the change (although the tests did pass before the changes).

[dougwilson]

Re module: shouldn't Stack just be another file in https://github.com/pillarjs/router/tree/master/lib ? I'm not sure I understand the separation of just that one piece.

[wesleytodd]

I am pretty confident that this abstraction would be applicable outside of just express. The other frameworks are one use case, as well as compatibility layers written against express (like in serverless platforms). Lastly, it would mean that express@4 users could start using it as well directly (because it is backwards compatible).