Express: Define error-handling middleware functions explicitly (without arity detection)

[feross]

I propose adding an explicit app.error method for defining error-handling middleware functions:

app.error(function(err, req, res, next) {
  res.status(500).send('Something broke!');
});

Instead of:

app.use(function(err, req, res, next) {
  res.status(500).send('Something broke!');
});

It's so easy to forget the next argument when it's not being used in the body of the function, and that changes the whole meaning of the middleware. express is one of the only packages that has this pattern. Others that used this pattern in the past (i.e. superagent) have since removed it.

Furthermore, this clashes with popular linting rules like ESLint's no-unused-vars rule which enforce that all named arguments must be used in the function body. Users who see this rule and remove the un-unsed next parameter will be unwittingly changing the behavior of their program.

No one expects removing an unused parameter to change the behavior of a program.

app.use(function(err, req, res, next /* <-- unused, guess I'll remove this... */) {
  res.status(500).send('Something broke!');
});

The four argument middleware convention should be deprecated in favor of app.error, but support for it could remain for a long time, or even indefinitely. I just want to be able to recommend that folks use app.error going forward.

[wesleytodd]

This, this this! I have thought this for a very long time. The implicit behavior of the error handlers is confusing to beginners and advanced users alike. I would support making this to 5.0 and explicitly deprecating the old way, for future removal. And possibly even adding as a new feature in 4.0 as a point release. Thanks @feross for bringing this up.

[Fishrock123]

I think this is probably a Good Idea™.

Maybe we should do some assessment of how much app.use(function(err, req, res, next) {}) is used in the wild and see how viable deprecation would be for 5.0?

[wesleytodd]

My guess is that since it is the only way to handle errors it is used in EVERY express app :) Right?

I know for sure that I use it in all of my apps.

[jordonias]

It would have to be changed in the errorHandler middleware as well. If deprecated, new versions of the middleware would not work with Express 4. There may be some other modules as well.

[wesleytodd]

Do you have a link to the code that you are saying would need to change?

[jordonias]

https://github.com/expressjs/errorhandler/blob/master/index.js#L83

This is just an example. There is going to be other third-party modules that are going to have to be changed as well, and I'm not sure if it's possible to handle carefully.

[tunniclm]

This looks like a nice idea. The implicit error handler behaviour is one of the more surprising parts of the api imo.

[hacksparrow]

The interface looks lovely. I am assuming the extended example looks something like this:

app.error(function(err, req, res, next) {
  if (err.code === 404) res.status(err.code).render('404');
  else res.status(err.code).send(err.string);
});

[billinghamj]

Major :+1: to this - my linter complains about the unused parameter all the time even though I can't remove it.

[wesleytodd]

@billinghamj I use // es-lint-disable a bunch for this, but I think you should be able to do something like // eslint no-unused-vars: [2, { "args": "none" }] to specifically do just that one rule. I just hammer it with a full disable because I cannot remember that long argument line :)

[sjanuary]

This API is really nice :+1:

[dlinx]

Nice option,
There is another option,

/* error parameter will be at end. To maintain standards*/
app.use(function(req, res, next, err) { 
  res.status(500).send('Something broke!');
});

Another one option. Using chaining to handle error. This will give power to handle error at all levels.

app.get('/',function(req, res, next) { 
  res.status(500).send('Something broke!');
}).error(function(err){
    /* Error Handler*/
});

[Zertz]

This is a neat idea, but it would have to be a feature because this is a gigantic breaking change. There are a lot of Express dependents out there and this breaks all of them, no exceptions.

Personally, I don't think deprecation is even appropriate.

[billinghamj]

There's no reason not to continue supporting the old method, but just outputting a warning to the console explaining what the new way is.

The old way can then be removed in the next major version, or perhaps the one after.

[dougwilson]

There's no reason not to continue supporting the old method, but just outputting a warning to the console explaining what the new way is.

There absolutely is. How does app.error provide a method to only handle errors for specific routes? Specific methods? In the middle of a bunch of handlers? Specifically part of just a single Route? The currently proposed app.error looks good, but it does not capture how versatile the current error-handling possibilities actually are, only capturing the very basic global handler case.

[billinghamj]

Imo, it would be best not to change the behavior of error handlers at the same time as moving away from the arity detection. If they are independent changes, and the removal of the arity detection makes no difference to the functional behavior, there isn't much to worry about from what I can see.

Why would .error be different than any other kind of middleware? (Other than when it is triggered.)

[dougwilson]

@billinghamj, error handlers can be used in more than just app.use.

[dougwilson]

Regardless to various things to work out, what I hear loud and clear: Express should not use argument arity to differentiate between error and non-error handlers, and I 100% agree.

[adamreisnz]

+1 for this. I've stumbled on this so many times, because jshint always complains about the unused next parameter.

Current workaround to stop jshint from complaining in middleware that doesn't use next is (without turning off the jshint flag that is):

function(error, req, res, next) {
  next = next || null;
  //do stuff
}

But that's just... meh

[coakleypa]

Its been said a lot already but to add I faced this multiple times also with eslint, reverted to using:
/* eslint-disable no-unused-vars */

[wesleytodd]

If anyone is interested, I opened a PR in the router package for this idea. Would appreciate any feedback. https://github.com/pillarjs/router/pull/59

[golopot]

This issue can be solved if a new approach is devised that allows express to differentiate between error and non-error handlers. For example a property fn.isExpressErrorHandler can be assigned and checked. Or a wrapper object {isErrorHandler: true, fn: handler} can be used to take place of a handler.

User-facing API

Users creates an error handler by calling express.createErrorHandler(fn):

const errorHandler = (err, req, res, next) => {...}

app.use(foo, [bar, express.createErrorHandler(errorHandler)])

Implementation

Approach 1: Assign and check handler.isExpressErrorHandler

express.isErrorHandler = (fn) => {
  return Boolean(fn.isExpressErrorHandler);
}

// assign the property `isExpressErrorHandler = true`
express.createErrorHandler = (fn) => {
  const clonedFn = fn.bind({})
  clonedFn.isExpressErrorHandler = true;
  return clonedFn;
}

Approach 2: Assign and check a wrapper object

express.createErrorHandler = (fn) => {
  return {
    isExpressErrorHandler: true,
    handler: fn,
  }
}

express.isErrorHandler = (handler) => {
  return Boolean(handler.isExpressErrorHandler);
}

[wesleytodd]

@golopot, can you check out the open PR for this? https://github.com/pillarjs/router/pull/59

Your thoughts would be appreciated there, thanks!