search

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

fixed resp.statusCode >= 400 #51

Closed EvanCarroll closed 9 years ago

EvanCarroll commented 9 years ago

Seems a lot cleaner in the patch. The body is also null -- so you need to generate the error as I did in the patch.

naugtur commented 9 years ago

if both error and body are null, it means there's no information about the error... In that case it seems you actually want a feature that'd return a message with the statusCode. It can be done, let's talk about it and see if others see the point of doing that.

EvanCarroll commented 9 years ago

Right, clearly that's what I want. But, moreover, it's needed and that's what this request does. Your just failing silently rather than failing because of bad conditional. That's a very small step up. The problem is only the ampersand-sync knows that the resp >= 400, even though we're triggering an error for it. That makes it very difficult to test and rather useless.

naugtur commented 9 years ago

current ampersand-sync behavior in xhr2-and-node does not differ from latest released ampersand-sync as far as I know. and nothing is failing silently (failing silently would be not calling the error callback at all)

returning status code is a good idea, but this branch is not about changing the behavior.

EvanCarroll commented 9 years ago

So how do you get the reason for the failure? Failing silently is precisely invoking the error callback without a reason.. That's what we're doing here. Failing.. Silently..

EvanCarroll commented 9 years ago

Force-pushed to reflect change in names, and the resp.statusCode oversight you pointed out.

naugtur commented 9 years ago

too much verbosity and unnecessary changes, force push and other merges. This PR is now too hard to merge. I addressed the issue in #55