router icon indicating copy to clipboard operation
router copied to clipboard

feat: create `router.error` method to allow explicit definition of error-handling middleware

Open nathanhleung opened this issue 7 months ago • 4 comments

Related issues/PRs: https://github.com/expressjs/express/issues/2896, https://github.com/pillarjs/router/pull/59

Summary

This PR creates a router.error method which enables the explicit definition of error-handling middleware (first proposed by @feross ~10 years ago in https://github.com/expressjs/express/issues/2896) as opposed to implicit definition by passing an arity-4 function to router.use.

The new router.error method supports error handling middleware defined with either 3 or 4 arguments. The first argument to the middleware will always be the error err (thus avoiding the arity gotcha with router.use).

// This works, even though the arity of the handler is 3
router.error(function (err, req, res) {
  res.statusCode = 500
  res.setHeader('Content-Type', 'text/plain')
  res.end('ouch on ' + req.method + ' ' + req.url + ': ' + err.message)
})

// The usual signature also works
router.error(function (err, req, res, next) {
  res.statusCode = 500
  res.setHeader('Content-Type', 'text/plain')
  res.end('ouch on ' + req.method + ' ' + req.url + ': ' + err.message)
})

// It's also possible to set error handlers on specific paths
router.error('/foo', function (err, req, res) {
  res.statusCode = 500
  res.setHeader('Content-Type', 'text/plain')
  res.end('ouch on ' + req.method + ' ' + req.url + ': ' + err.message)
})

The PR is backwards-compatible and makes no functional changes to any other router methods, including router.use; it is purely additive. All tests pass locally for me, at least (macOS Sequoia, Node v22).

Let me know your thoughts/feedback on the current approach and can revise.

Changes

The diff is ~80 source lines (plus ~40 lines of doc comments) and ~600 test lines. The new router.error method mostly falls back on the existing logic in router.use when possible. Effectively, it wraps its argument in another wrapper function with arity 4 to trigger the existing error handling behavior in use, and then the wrapped function is passed to use. Since it falls back on the existing logic in use, the same path behavior also works (i.e., error can be set to work with regular paths, parametrized paths, regexes, etc.)

The PR differs from https://github.com/pillarjs/router/pull/59 in that the changes are isolated to the router itself, rather than being on the individual route objects.

Demo

Live demo on CodeSandbox (using my forked version of router): https://codesandbox.io/p/devbox/y63xfg?file=%2Findex.js

function logErrorToConsoleMiddleware(err, req, res, next) {
  console.error(err)
  next(err)
}

function logErrorToFileMiddleware(err, req, res, next) {
  fs.appendFile('errors.txt', err.toString(), function() {
    next(err)
  })
}

// Note that the arity of the error handler can be 3 when passed to
// `router.error` (it must be 4 when passed to to `router.use`).
function sendErrorResponseMiddleware(err, req, res) {
  res.statusCode = 500
  res.setHeader('Content-Type', 'text/plain')
  res.end('ouch on ' + req.method + ' ' + req.url + ': ' + err.message)
}

router
  .error([logErrorToConsoleMiddleware, logErrorToFileMiddleware])
  .error(sendErrorResponseMiddleware)

nathanhleung avatar May 16 '25 00:05 nathanhleung

Friendly ping @UlisesGascon @wesleytodd @bjohansebas — anything I can help out with here in terms of being able to get this merged? Thanks for your work as maintainers 🙏

nathanhleung avatar May 19 '25 22:05 nathanhleung

Thanks for running CI — seems like the failing test is unrelated to this PR?

(Seeing same failure on https://github.com/pillarjs/router/pull/161 in https://github.com/pillarjs/router/actions/runs/15097652965/job/42434464916?pr=161)

nathanhleung avatar May 21 '25 04:05 nathanhleung

yes, see https://github.com/pillarjs/router/issues/162

bjohansebas avatar May 21 '25 14:05 bjohansebas

@UlisesGascon @wesleytodd @bjohansebas

thanks for taking a look at https://github.com/pillarjs/router/pull/169. now that it's closed + unblocking this, could we run the workflows here?

nathanhleung avatar Aug 15 '25 15:08 nathanhleung