AmpersandJS / ampersand-sync

Provides sync behavior for updating data from ampersand models and collections to the server.
http://ampersandjs.com
MIT License
20 stars 28 forks source link

On error, what if we return json? #52

Closed EvanCarroll closed 9 years ago

EvanCarroll commented 9 years ago

We currently process on success the response -- if it's JSON, we return the data structure. However, on error we don't do that. I think we should.. Ideas?

Would you accept a patch that included this functionality?

naugtur commented 9 years ago

Could you elaborate? What data structure would you expect on error? Which error? Examples would be best suited here.

EvanCarroll commented 9 years ago

If I return...

HTTP 400 /
{"message": "This error is ------"}

I'd expect to get back a JSON object -- like I would if it was response code 200, and not a string that I have to explicitly decode..

naugtur commented 9 years ago

Ok, now I get it.

What if the server returns

HTTP 404 /
Not Found

and it's not JSON?

ampersand-sync would have to try to parse JSON and if it fails, return text. In that case you'd never know if you get text or string in the error callback argument.

If it was for me to decide, I'd always return an error object that has message, body and statusCode fields every time (I actually did just that in some of my own projects)

Anyway, this is not related to xhr2 and node, and is not a regression, so I'll let someone else address it.

EvanCarroll commented 9 years ago

If the body is Not Found -- return that. Otherwise, make the response object available so you can get the status code (we already do that). At least you know what the code is even if the server doesn't return a body. I'm just thinking it should be consistent with the success cb. The problem is on a reasonable JSON interface we're returning 400 with JSON body when the user isn't logged in. If the response code is 200 the resp argument is useful and I can do resp.message (assuming the above example). However, when I sent 400 I get my error callback but my response is a string. I have to call the JSON decode myself. That's not a good interface. It's inconsistent.

ampersand-sync would have to try to parse JSON and if it fails, return text. In that case you'd never know if you get text or string in the error callback argument.

This is exactly what we do in the success cb though. I like that behavior myself. It's great!

If it was for me to decide, I'd always return an error object that has message, body and statusCode fields every time (I actually did just that in some of my own projects)

That's again inconsistent with the success cb. I wouldn't do that. I'd make that stuff easily available in options, or define options as the last argument and push that stuff off to another argument. The error and success callbacks should have the same signature for sanity.

naugtur commented 9 years ago

if response content-type is application/json the body of the message could be parsed. Can be done after #55 gets merged