strongloop / strong-remoting

Communicate between objects in servers, mobile apps, and other servers.
www.strongloop.com
Other
105 stars 93 forks source link

Modify RestAdapter to allow disabling errorHandler #248

Closed digitalsadhu closed 9 years ago

digitalsadhu commented 9 years ago

options.handleErrors = false will allow the RestAdapter error handler to be bypassed

See: https://github.com/strongloop/loopback/issues/445#issuecomment-147722977 for discussion

@bajtos does this look ok?

slnode commented 9 years ago

Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test."

listepo commented 9 years ago

:+1:

bajtos commented 9 years ago

@slnode ok to test

bajtos commented 9 years ago

Hi @digitalsadhu, thank you for the pull request. Could you please add some unit-tests to verify the implementation and prevent regressions in the future? See https://github.com/strongloop/strong-remoting/commit/f26a87104045c5cbde29c02fcde679bb08e31109 for an inspiration.

digitalsadhu commented 9 years ago

@bajtos tests added, still seeing failing checks there. Not sure how I would resolve those as tests are all passing for me. Can you point me in the right direction?

bajtos commented 9 years ago

LGTM.

@ritch would you like to take a look at this change yourself too?

still seeing failing checks there. Not sure how I would resolve those as tests are all passing for me. Can you point me in the right direction?

Those test failures are in projects that are using strong-remoting as a dependency. I think it's the loopback-example-oracle repository that is failing unit-tests because the oracle driver does not support Node v4.x yet.

@rmg any ideas how to fix this false alert?

@digitalsadhu don't worry about those dependent builds, I am ok to land the PR with loopback-example-oracle failing. Could you please squash the commits into a single one?

digitalsadhu commented 9 years ago

@bajtos Squashed

bajtos commented 9 years ago

@digitalsadhu a nitpick, could you please fix the commit message to use the correct option name options.handleErrors (plural, not singular)?

bajtos commented 9 years ago

I asked @ritch to review and land this, I am on vacation tomorrow.

digitalsadhu commented 9 years ago

@bajtos @ritch Oops, fixed commit message now.

rmg commented 9 years ago

@bajtos they aren't warnings, the loopback-example-recipes module has a hard dependency on loopback-connector-oracle :-( I've opened an issue for it: strongloop/loopback-example-recipes#8

digitalsadhu commented 9 years ago

@bajtos how are we looking?

bajtos commented 9 years ago

@digitalsadhu thanks for the reminder. Since @ritch has not commented, I'll assume he does not have any major objections.

Landed, thank you for the valuable contribution!

ritch commented 9 years ago

:+1:

digitalsadhu commented 9 years ago

Yay! Thanks guys.

On Wed, 21 Oct 2015 at 15:33, Ritchie Martori notifications@github.com wrote:

[image: :+1:]

— Reply to this email directly or view it on GitHub https://github.com/strongloop/strong-remoting/pull/248#issuecomment-149897095 .

bajtos commented 9 years ago

Released to npmjs.org as strong-remoting@2.22.0

bajtos commented 9 years ago

Enjoy :)

crandmck commented 8 years ago

@digitalsadhu I'd like to add this to the API documentation on https://apidocs.strongloop.com/strong-remoting/ but I'm not sure where it should go. Can you give me a pointer?

bajtos commented 8 years ago

@crandmck I think here is a better place where to document this new feature: https://docs.strongloop.com/display/LB/config.json?src=search#config.json-Remotingproperties

The property name is rest.handleErrors.

While you are at it, could you please document rest.handleUnknownPaths too? It was added by f26a87104.

crandmck commented 8 years ago

@bajtos Thank you, that does make more sense.

I added both those properties to https://docs.strongloop.com/display/LB/config.json#config.json-Remotingproperties. In particular, the descriptions could perhaps have a bit more... PTAL.

digitalsadhu commented 8 years ago

Nice! 1 thing though, typo: "handleErros" : true,

crandmck commented 8 years ago

Doh! Thanks for catching--fixed...

bajtos commented 8 years ago

@crandmck I have added a bit more info, PTAL.

crandmck commented 8 years ago

Thanks again @bajtos