expressjs / generator

Express' application generator
MIT License
1.83k stars 548 forks source link

npx express-generator does not follow error recommendations #254

Open sdavids opened 4 years ago

sdavids commented 4 years ago

https://expressjs.com/en/guide/error-handling.html#the-default-error-handler

So when you add a custom error handler, you must delegate to the default Express error handler, when the headers have already been sent to the client

function errorHandler (err, req, res, next) {
  if (res.headersSent) {
    return next(err)
  }
  res.status(500)
  res.render('error', { error: err })
}

The express-generator does not follow the recommendation:

$ npx express-generator
$ grep -A 10 '// error handler' app.js
// error handler
app.use(function(err, req, res, next) {
  // set locals, only providing error in development
  res.locals.message = err.message;
  res.locals.error = req.app.get('env') === 'development' ? err : {};

  // render the error page
  res.status(err.status || 500);
  res.render('error');
});
dougwilson commented 4 years ago

I think this may be a documentation issue on the website; there is no requirement to delegate to the default handler if the headers have already been sent; users can do whatever logic they would like to do.

barraponto commented 4 years ago

from the docs:

If you call next() with an error after you have started writing the response (for example, if you encounter an error while streaming the response to the client) the Express default error handler closes the connection and fails the request.

oviecodes commented 4 years ago

@sdavids Did you create the error handler after all the other routes. That could cause some issues if you didn't.