search

zooniverse / json-api-client

Apache License 2.0
10 stars 5 forks source link

Only reject promise if there's a network issue #28

Closed rogerhutchings closed 8 years ago

rogerhutchings commented 8 years ago

Will need to test for regression errors... or make this a major version bump, since the API is changing.

Fixes #27

brian-c commented 8 years ago

I don't really agree that this is the right behavior. The library is for working with resources, not making HTTP requests (this behavior would make sense for superagent though, for example). resolve-ing after an action should indicate that it was completed as intended, rejecting should indicate that it did not.

This would also require rewriting a good chunk of the main app.

I think the right change would be to include the status code in the error object. Or maybe a subclass of Error would be more appropriate? This one looks nice: https://www.npmjs.com/package/standard-http-error

I think this would only require rewriting the handleError method in panoptes-client's JSONAPIClient instance.

eatyourgreens commented 8 years ago

Sounds good to me http://jsonapi.org/examples/#error-objects includes the status code in the first example.

:+1: to re-writing panoptes-client's handleError method, eg. so that we can catch 300 redirects and 400 client-side errors on resource.save() so that we don't continuously retry bad requests from PFE.

brian-c commented 8 years ago

We're using an old prerelease version of JSON API, and I don't think error responses had been standardized at that point so we're really just wingin' it.

I used to have a Wayback Machine link to near the version we used, I'll see if I can find it and add it to the README.

brian-c commented 8 years ago

Replaced by #29.