Deprecate arity check in favor of explicit error handlers

[wesleytodd]

This is a first go at the idea discussed in expressjs/express#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]

Should there be an example for how to modify something like router.get('/foo', (req, res, next) => ..., (err, req, res, next) => ...) type patterns?

Good idea!

I think maybe that was because I pushed the change to remove the references to express in the readme. I will add that example as well, but queue the change to give you the opportunity to finish. But this sounds like a github bug :(

[dougwilson]

I was able to cancel it. I had to switch my phone from mobile to desktop and then the cancel actually canceled it.

[wesleytodd]

I have added an example for the conversion. Do you disagree on my thoughts about why it should be a separate module instead of bundled in here?

[dougwilson]

Sorry I didn't realize you had the reason listed in here. I left to go shopping now, but i will be back on later tonight to go back up and find and read it and let you know. I assume I didn't already respond to it?

[dougwilson]

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).

Wouldn't the same apply to the other classes in this module? Since it would just be an export any reason it couldn't be used by requiring this module? My thought is that we should either split out everything or not. Having this single one be split out feels weird, but maybe if you can show an example of how it would be used specifically that could help make sense?

[dougwilson]

My biggest concern to it being split is that we're going to have another path-to-regexp situation, where a core part of how this module works suddenly starts changing completely independently and it gets way out to sync.

[dougwilson]

(That's not my only concern, at least jist one i typed really wuick while walking lol)

[dougwilson]

Looking at the readme of mwstack it doesn't seem like you can do anything other than ultimately pass it to express. There doesn't seem to be any kind of interface to make use of the stack after building it. I assume this module would then use undocumented interfaces to make use of the object from that module. So that's partly why I'm not sure how anyone else would be able to make use of it outside of this module, as the readme of mwstack makes it read like that is the case.

[wesleytodd]

Wouldn't the same apply to the other classes in this module? Since it would just be an export any reason it couldn't be used by requiring this module? My thought is that we should either split out everything or not.

It might, and I see no issue with that where it makes sense.

My biggest concern to it being split is that we're going to have another path-to-regexp situation, where a core part of how this module works suddenly starts changing completely independently and it gets way out to sync.

There are pros & cons here. While I agree that path-to-regexp has caused some issues lately because of it has outpaced the rest of the project, I do not think the solution is to slow other parts down. I guess I think there is a balance, path-to-regexp went too far maybe, but the benefit of having reusable components we can share is worth it.

Looking at the readme of mwstack it doesn't seem like you can do anything other than ultimately pass it to express.

Another router could also use it, which is one of the first things I was planning on doing with it. The hope would be that my experimentation would help direct the future of this module.

maybe if you can show an example of how it would be used specifically that could help make sense?

It would be the same way we use it, just allowing for other implementations to directly share our implementation. I was going to mess with integrating it to my experimental router, so if you want I can prioritize that work, but it probably wont show much.

[dougwilson]

Yes, there are definately pros and cons. But my opinion, based just on the mwstack repot itself, is that it is very tightly-coupled with this module. There is no reason it cannot just land here first and then be extracted after the fact once there is a need to share it without the baggage here. Right now that module has no documentation on how it would be possible to use without this module, so to me, seems like it hasn't even graduated to a point where it should be a separate module vs just an included class of this module.

[wesleytodd]

It is coupled to the API, but not the implementation details. My usage of it outside of router would be because I am going to do some things similar in API but different in implementation details. I would just have to use my separate version of it, which seems pointless to me 🤷‍♂️.

[dougwilson]

What I mean, though, is that we want to treat separate modules as separate, and utilize the public APIs of each one, so the semver makes sense and we can manage them. This PR seems to be using undocumented APIs from the mwstack module currently. How can we manage something like that over time? If it's just a class within this module then it doesn't matter, but being a separate module we need to get everything in order if that is what we want to do, and that is something at the top of my mind for what is missing here.

[wesleytodd]

This PR seems to be using undocumented APIs

Which ones? If I missed one then it was an accident.

[wesleytodd]

Is it that I only say array like and not explicitly that it extends array?

[dougwilson]

This .concat seems to be undocumented in the mwstack repo.

[wesleytodd]

Ok, yeah so that is just because it extends array. Would it satisfy if I added a section at the top of the api docs which linked to the array documentation and said "Stack and FlatStack extend from array"?

[dougwilson]

I get that it extends an array, but there is no documentation for what that array actually contains. Is it an array of strings, numbers, objects of some kind, functions... ? I cannot tell if the result of createLayers here results in something that should be in that array.

[wesleytodd]

It is an array of functions, but I guess I can see how this line abstracts that in such a way where it is hard to know. Let me see if there is a way I can make this more clear in the documentation. createLayers returns the result of callbacks.map, so it returns an array of middleware functions which are just passed to Array.prototype.concat.

[dougwilson]

So if .concat should be given an array of function, then this API is used wrong here. The createLayers function is returning an array of Layer objects, not an array of functions.

[wesleytodd]

Yeah, Now that I read this again I think it is better to keep them separate. So I changed router.stack back to a plain array. I will push once we have any other changes worked out (so I don't break any reviews in progress).

[dougwilson]

I haven't dug into the PR deeply to comment on what makes sense or not; currently my comments are just about if we're splitting things apart into different modules, then we're careful to use the external module as is documented/support by said module so we don't end up breaking ourselves on accident with mysterious API usage is all.

[wesleytodd]

Perfectly understandable. I will make sure that mwstack has a more clear documentation on the fact that it extends Array. In this case, now that I am removing it's usage as router.stack we are only importing it to expose it and use the one documented method mwStack.isError. Seems like that solves the concern?

[dougwilson]

Perhaps. I'm not sure without seeing it to honestly say for sure 😆 . To be clear: my concern is just that we're using a module as that module documents being used. The isError seems to say the following from the mwstack docs:

Returns true when the middleware was added to a stack as an error middleware.

It seems to imply, at least to me, that it is to be used on a func that was added to the Stack of this module, is that right? And it will tell you yes/no if it was added to Stack as an error middleware or not?

[wesleytodd]

If it is used on a function which was never added to a stack as an error it will always return false (so just any random mw function). But yes, it is intended to be passed any middleware function and will return true/false accordingly.

[dougwilson]

I will work to comment on them inline in the code, since I will need to review through all the code to note which ones they are.

[dougwilson]

I think this is an issue here, as it is modifying opts but that is an object used for all callbacks, yet the modification seems to be only for the given callback being mapped over at the time.

It would seem to reason that if callbacks was an array of "middleware", "error handler", "handler" then this would end up marking that last "hander" as an error handler incorrectly?

[wesleytodd]

Good catch! I will write a test which covers this and then make sure that the opts are only modified locally.

[wesleytodd]

Also, it is quite buried in this thread, but the goal of exposing the Stack is that you will no longer be a able to construct arbitrary arrays of middlewares with error handlers in the middle. This makes it so there is a replacement for this functionality router.use(stack.infer([ ... ])).

[dougwilson]

Also, it is quite buried in this thread, but the goal of exposing the Stack is that you will no longer be a able to construct arbitrary arrays of middlewares with error handlers in the middle. This makes it so there is a replacement for this functionality router.use(stack.infer([ ... ])).

Yep, understood 👍 I recall that aspect of these changes and have no issue with it, being an edge-use-case.

[wesleytodd]

@dougwilson do you consider this mwstack concern as blocking this PR? If so I will pull it into the repo to get it published. That will just mean that I will continue to support my separate module independently 🤷‍♂

[dougwilson]

Sorry, please remind me which issue in particular, as there were a couple conversations going on above in parallel. I know you said you were going to push up some change around isError or something so I was mostly just waiting to look when new changes appear, but are you asking to no longer make those changes?

[wesleytodd]

Sorry, I should have written more to be more clear. Yes there were three points:

1. Should it be a separate package
2. If it is a separate package, is everything we are using well documented
3. General fixes on the PR and mwstack module

I think I can resolve numbers 2 & 3 with the conversation we had above. If that is resolved, does it also resolve number 1 for you? I clearly think this belongs in a separate package, but if you disagree strongly enough to block this I would rather pull it in and get the ball rolling forward on fixing this than be pedantic on where the code lives.

[wesleytodd]

ping @dougwilson, I would love to land this because I think it is one of the more impactful feature changes, and if we dont land it in 5 we have to deprecate in 6 and remove in 7. Can you take a look so I can either pull the mwstack code in or publish it in the next few days?

[dougwilson]

If that is resolved, does it also resolve number 1 for you?

It sounds like it would, but only after seeing the changes could I truly say. I've been waiting to see what they look like...

I clearly think this belongs in a separate package, but if you disagree strongly enough to block this I would rather pull it in and get the ball rolling forward on fixing this than be pedantic on where the code lives.

It is not being "pedantic on where the code lives"; if something is split into it's own module it need to be treated well. It is extremely straight forward to pull things out at a later time and so it doesn't make sense to start with a weird module where we are immediately using undocumented things from it.

[wesleytodd]

It sounds like it would, but only after seeing the changes could I truly say. I've been waiting to see what they look like...

I didn't realize you were waiting on that. I will for sure make the updates then. Including moving the code into this repo. I was saying that I did not want to be pedantic, so I am fine moving it into here if that helps this move forward.

[dougwilson]

Gotcha. If you're ok with that, it seems like it would be pretty straight forward, and if it would help, I could add a commit that moves it (separate commit so you could review / remove if incorrect) if that would help speed things along :) If your next window is this weekend, I could get to it prior to then.

[wesleytodd]

Yep! I will do that as soon as I can carve out the time (hopefully tonight)!