Logging from rest-adapter.errorHandler

[STRML]

I'd like to open up a small discussion on logging from this module - I often find it difficult in development to find stacktraces for errors that occur within my routes. They are never printed to the console, they are instead sent out the pipe to the client (except in production, because of a recent PR I submitted).

Of course, one can debug.enable('strong-remoting:rest-adapter') but there is a lot of noise there as well.

Rather than try to come up with a hard-and-fast rule for how these should be handled - since one size definitely does not fit all - how do you feel about an optional errorHandler function that would supersede the built-in error handler? We could pass it similarly to to the existing disableStackTrace option.

[bajtos]

I can see two approaches:

1. add an option errorReporter of type function(Error), the only purpose is to log the error

2. add an option errorHandler of type function(Error, req, res, next) that will be called first to build the error response.

   RestAdapter.errorHandler = function(options) {
     options = options || {};
     return function restErrorHandler(err, req, res, next) {
       if (options.errorHandler && options.errorHandler(err, req, res, next)) return;
       // now it's up to the default error handler to handle the error
     };
   };

I think the second option is better - it is more flexible but still keeps it easy to implement a custom error logger:

options: {
  errorHandler: function(err, req, res, next) {
    console.error(err);
    return false;
  }
}

[STRML]

Rather than using return false to continue, why not just use next - after all, it’s typical in error handlers to assume that you are the last handler in the chain and to simply send a response (am I wrong on this?). Users could simply:

options: {
  errorHandler: function(err, req, res, next) {
    console.error(err);
    next(); // to use default error handler
  }
}

And the implementation would be something like:

RestAdapter.errorHandler = function(options) {
  options = options || {};
  return function restErrorHandler(err, req, res, next) {
    if (typeof options.errorHandler === "function") options.errorHandler(err, req, res, handleError);
    else handleError();
    function handleError() {
      // ... default error handler
    }
  };

};

[bajtos]

Rather than using return false to continue, why not just use next()

I suppose one may want to skip the REST error handler and let the top-level app handle the error.

// Option 1 - let the default REST error handler do the job
return false;

// Option 2 - skip the default REST error handler, let the top-level app handle the err
next();

If that's not the use case we want to support, then you proposal is great.

[STRML]

In the current implementation, users need to know that loopback.rest() defines a separate express app with its own error handling, and it's really confusing that it has separate error handling than routes defined for the rest of the app.

Perhaps we skip the whole thing by default in loopback.rest() via the above configuration (errorHandler: function(err, req, res, next) { next(); }), then modify loopback.errorHandler to use the strong-remoting semantics (perhaps as a separate project required by strong-remoting and loopback?) That way it's explicit and used for all routes, and users can override as they see fit.

[bajtos]

In the current implementation, users need to know that loopback.rest() defines a separate express app with its own error handling, and it's really confusing that it has separate error handling than routes defined for the rest of the app.

Well, my reasoning was that you should always receive JSON error response for all /api paths, even if you send the request using cURL or the browser. And even if you do not install any express error handler.

Perhaps we skip the whole thing by default in loopback.rest() via the above configuration (errorHandler: function(err, req, res, next) { next(); }), then modify loopback.errorHandler to use the strong-remoting semantics (perhaps as a separate project required by strong-remoting and loopback?) That way it's explicit and used for all routes, and users can override as they see fit.

That is a possible way. There is no need to move the error handler to a separate project, exposing it as a public strong-remoting API should be good enough.

[ritch]

+1 for this:

options: {
  errorHandler: function(err, req, res, next) {
    console.error(err);
    next(); // to use default error handler
  }
}

[STRML]

Yeah, this is starting to really annoy me in production, working on a fix for it.

On Oct 15, 2014, at 6:28 PM, Ritchie Martori notifications@github.com wrote:

+1 for this:

options: {
errorHandler: function(err, req, res, next) {
console.error(err);
next(); // to use default error handler
}
}
—
Reply to this email directly or view it on GitHub.

[bajtos]

Yeah, this is starting to really annoy me in production, working on a fix for it.

There is already a pending PR for this - see #114