Skip to content

Conversation

@nathanhleung
Copy link

@nathanhleung nathanhleung commented May 16, 2025

Related issues/PRs: expressjs/express#2896, #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 expressjs/express#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 #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
Copy link
Author

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
Copy link
Author

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

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

@bjohansebas
Copy link
Member

yes, see #162

@nathanhleung
Copy link
Author

@UlisesGascon @wesleytodd @bjohansebas

thanks for taking a look at #169. now that it's closed + unblocking this, could we run the workflows here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants