Default responder should not console.error 4xx errors

[dougwilson]

Right now the default handler (last step) will console.error when it gets an err, otherwise 404. I think that it probably shouldn't console.error when the err it got was a 4xx error, mainly because a 4xx is supposed to be a client error, of which the server admin probably cannot do anything about. Adding a custom error handler could, of course, let the user log 4xx error all they wanted--this is just about the default behavior. It could probably wait until express 5, though.

[Fishrock123]

+1

[aredridel]

👍 5.0

[ritch]

I agree this should be removed. In a hello world express app, you'll sometimes get a nasty error printed when the browser tries to grab /favicon.ico which is pretty annoying.

It could probably wait until express 5, though.

We should avoid removing behavior (however lame it might be) in a minor version. Anything landing in 4.x should be additive or fixes until 5. This change may actually be breaking, even though it seems like it wouldn't. Eg. someone might have a log parser that relies on this error, who knows.

[dougwilson]

Anything landing in 4.x should be additive or fixes until 5. This change may actually be breaking, even though it seems like it wouldn't. Eg. someone might have a log parser that relies on this error, who knows.

Thus the reason I said it should land in 5 :)

[dougwilson]

The reason I only suggested it wait for 5 is because there is a bunch of work around it. If it were to change, it should be made configurable. Of course if it's configurable, it could then easily land on 4 with the default config having it operate as it does now. Standard back-compat stuffs.

[dougwilson]

Also, feel free to read through all the git commits in the history, at least through all the ones over the past couple months. They demonstrate that back-compat pattern well, giving people the ability to opt-in to cool new stuff, but not change the existing behavior for people who do nothing, etc.

[ritch]

The reason I only suggested it wait for 5 is because there is a bunch of work around it.

Makes sense. So would the ideal approach (with a bunch of work) be to land the ability to configure in 4.x and change the default in 5?

[dougwilson]

So would the ideal approach

I already highlighted the ideal approach in the comment below what you quoted :)

[ritch]

Haha not sure how I missed that you said exactly that.... Re-reading my comment it doesn't make any sense. What I'm getting at is that I'd like to take this on and do it the right way.

Can we mark this as 4.x? Or is there anything else to sort out?

[tunniclm]

Would something like this be considered a reasonable back-compatible solution?

diff --git a/lib/application.js b/lib/application.js
index 0ee4def..47e573a 100644
--- a/lib/application.js
+++ b/lib/application.js
@@ -76,6 +76,7 @@ app.defaultConfiguration = function defaultConfiguration() {
   this.set('query parser', 'extended');
   this.set('subdomain offset', 2);
   this.set('trust proxy', false);
+  this.set('log client errors', true);

   // trust proxy inherit back-compat
   Object.defineProperty(this.settings, trustProxyDefaultSymbol, {
@@ -625,6 +626,10 @@ app.listen = function listen() {
  */

 function logerror(err) {
+  if (!this.get('log client errors')) {
+    var status = err.status || err.statusCode || res.statusCode || 500;
+    if (status >= 400 && status <= 499) return;
+  }
   /* istanbul ignore next */
   if (this.get('env') !== 'test') console.error(err.stack || err.toString());
 }

[dougwilson]

@tunniclm, we should focus on getting the needed changed into Express 5.0 instead of adding more weird back-compat solutions. There are already two solutions for Express 4.x users can use for this, and so adding yet another solution is, i.m.o., not very valuable.

Currently the best solution in Express 4.x is to just handle your errors with an error middleware and not forward 4xx ones to the default handler.

As part of the "minimalist web framework" ideology of Express, we historically favor not adding a bunch of configuration switches, as by the time we're done, you may as well just use Hapi.js :)

[tunniclm]

@dougwilson That's fine by me. I assume the next step is to raise a pull request against pillarjs/router then (with a forward-looking change)?