Skip to content

Changed errorHandler() to honor options.remoting.errorHandler.handler [GoDaddy] - #114

Merged
ritch merged 5 commits into
masterfrom
feature/error-handlers
Oct 23, 2014
Merged

Changed errorHandler() to honor options.remoting.errorHandler.handler [GoDaddy]#114
ritch merged 5 commits into
masterfrom
feature/error-handlers

Conversation

@ritch

@ritch ritch commented Oct 9, 2014

Copy link
Copy Markdown
Member

A follow-up for #110.

Close #102

/to @bajtos
/cc @raymondfeng

@ritch ritch added the #review label Oct 9, 2014
Comment thread lib/rest-adapter.js Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider this handler:

function(err, req, res, next) { next(err); }

The current implementation will skip the error handler provided by the REST adapter and pass the error to the app where the adapter is mounted on.

Is that our intention?

The alternative is to pass restErrorHandler to the next argument of the custom handler.

Also the current implementation is tracking an changes of options.handler, I am not sure if it is a good thing. An alternative version:

RestAdapter.errorHandler = function(options) {
   options = options || {};
   if (options.handler) return options.handler;
   return function restErrorHandler(err, req, res, next) {
     // etc.
   };
}

Lastly, the line L309 is missing a semicolon.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I interpret our discussion in #102 correctly, then you @ritch need to rework this PR so that the following usage is supported:

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I interpret our discussion in #102 correctly, then you @ritch need to rework this PR so that the following usage is supported:

Yep.

@projectxmaker

Copy link
Copy Markdown

👍

@slnode

slnode commented Oct 16, 2014

Copy link
Copy Markdown

Test PASSed.

@ritch ritch added #wip and removed #review labels Oct 22, 2014
@altsang altsang changed the title Changed errorHandler() to honor options.remoting.errorHandler.handler GoDaddy: Changed errorHandler() to honor options.remoting.errorHandler.handler Oct 22, 2014
@altsang altsang changed the title GoDaddy: Changed errorHandler() to honor options.remoting.errorHandler.handler Changed errorHandler() to honor options.remoting.errorHandler.handler Oct 22, 2014
@altsang altsang changed the title Changed errorHandler() to honor options.remoting.errorHandler.handler Changed errorHandler() to honor options.remoting.errorHandler.handler [GoDaddy] Oct 22, 2014
@ritch

ritch commented Oct 22, 2014

Copy link
Copy Markdown
Member Author

should it be if(typeof options.handler === 'function') {?

Sure... added.

@raymondfeng

Copy link
Copy Markdown
Member

LGTM

ritch added a commit that referenced this pull request Oct 23, 2014
Changed errorHandler() to honor options.remoting.errorHandler.handler [GoDaddy]
@ritch
ritch merged commit babb1ce into master Oct 23, 2014
@ritch ritch removed the #wip label Oct 23, 2014
@ritch
ritch deleted the feature/error-handlers branch October 23, 2014 00:04
@ritch

ritch commented Oct 23, 2014

Copy link
Copy Markdown
Member Author

Released as v2.7.0

/cc @shelbys

@STRML

STRML commented Oct 23, 2014

Copy link
Copy Markdown
Member

This is great - is there any way to use this in loopback 2.5.0?

@bajtos

bajtos commented Oct 23, 2014

Copy link
Copy Markdown
Member

AFAIK loopback 2.5.0 works with strong-remoting 2.7.0, just rm -rf node_modules/loopback && npm install.

@STRML

STRML commented Oct 23, 2014

Copy link
Copy Markdown
Member

Yes - what I'm asking is, how do I get this option down to strong-remoting?

@bajtos

bajtos commented Oct 23, 2014

Copy link
Copy Markdown
Member

Ouch, I was wrong, you can't do that now. Apparently loopback.rest does not support handler options - see lib/middleware/rest.js#L28.

@raymondfeng is working on a patch than includes a fix for that - see strongloop/loopback#337.

@bajtos

bajtos commented Oct 23, 2014

Copy link
Copy Markdown
Member

I have created an issue to update our docs - see strongloop/loopback#688

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.

Logging from rest-adapter.errorHandler

7 participants