update to xhr 2.0.0 breaks on errors

[mkaminsky]

After the update to xhr 2.0.0, it seems that the error handling path of fetchr, when accessed from the client side, is breaking. In the new version, xhr no longer returns an error in the callback for HTTP errors--their documentation now reads, "Your callback will be called with an Error if there is an error in the browser that prevents sending the request. A HTTP 500 response is not going to cause an error to be returned.").

fetchr's http.client.js:io() function seems to assume the old behavior. Thus, if the xhr call produces a status code 500, for example, err will still be false and the success callback will be called. That, in turn, results in a JSON parsing error.

The easy way to see this is to run the fetchr-simple-example, comparing the current HEAD and a commit just before the upgrade to xhr 2.0.0. By default, they both behave the same. Even if one changes the flickr API key to something invalid, everything still works because flickr returns its error in the body of its response with HTTP status 200. To see the problem, change the code on line 27 of server/fetchers/flickr.js to return an error:

//callback(err, JSON.parse(res.text));
callback(new Error("Oops"));

Then, access localhost:3000/client. In both case, the xhr call returns a HTTP status of 400:

"NetworkError: 400 Bad Request - http://localhost:3000/myapi/resource/flickr;method=flickr.photos.getRecent;per_page=5"

With the old version of xhr, the client throws an error, as expected. With the new version, I get the following:

FetchrClient json parse failed:SyntaxError: JSON.parse: unexpected character at line 1 column 1 of the JSON data +0ms error

Because of the new xhr 2.0.0 semantics, the io() function doesn't think there was any error and executes the success path, resulting in the parsing error. I didn't look into it, but the unit tests don't seem to catch this.

(As a side note, I ran into this problem while playing around with flux-examples, which use fetchr via fluxible-plugin-fetchr. The todo service, for instance, returns errors in this way.)

[jedireza]

Thanks for creating an issue. We're looking into this and will keep you posted.

[jedireza]

@mkaminsky would you mind trying out the fix I published in #47?

[mkaminsky]

Thank you for the quick response. Your new commit turns the unsuccessful http request into an error, but I think that there still is a problem in how the error is handled.

Your commit creates an Error object in io():

err = new Error(errMessage);

which then propagates to createErrorObj(response, timeout). That function expects the response object to have a status, statusText, and responseText, which it doesn't.

The errObj created/returned by createErrorObj thus has a bunch of null fields. Running fetchr-simple-example shows this in its debug output:

FetchrClient Syncing flickr failed: statusCode=undefined +0ms info FetcherClient

Then, when fetcher.client.js returns the error via its callback, the error message itself is now lost.

Perhaps this actually raises a more fundamental issue, in that the error object returned via the "client" path is different from the one returned via the "server" path (where there is no http request, statusCode, etc.).

[jedireza]

Yes you're right. The success and failure handlers have the signature (id, response), which also doesn't seem right. Previously we were sending back the response object to the handlers. Now we're just sending the body instead.

The createErrorObj will also need to be updated to properly build it's response. For example response.status is going to be response.statusCode.

Thanks again for bringing these things up.

[mridgway]

Woo, ok. Sorry for the delay on this, we basically had to reverse engineer what was going on in xhr.

@mkaminsky I created a new PR that should fix everything that was broken: #48. Would you mind testing this out? I have tested against our example code and against the todo flux-example with success.

We need to add some client-side tests to make sure we're not broken by our dependency in the future. Will add an issue for this.

[mkaminsky]

The new version seems to work for me. Thanks again everyone.