[XMLHttpRequest] Add support for ontimeout and onerror handler when using XMLHttpRequest for Android and iOS #6841

grgmo · 2016-04-06T16:12:55Z

Currently React-Native does not have ontimeout and onerror handlers for XMLHttpRequest. This is an extension to No timeout on XMLHttpRequest.

With addition to two handlers, both Android and iOS can now handle ontimeout if request times out and onerror when there is general network error.

Test plan

Code has been tested on both Android and iOS with Charles by setting a breakpoint on the request which fires ontimeout when the request waits beyond timeout time and onerror when there is network error.

Usage

JavaScript -

var request = new XMLHttpRequest();

function onLoad() {
    console.log(request.status);
};

function onTimeout() {
    console.log('Timeout');
};

function onError() {
    console.log('General network error');
};

request.onload = onLoad;
request.ontimeout = onTimeout;
request.onerror = onError;
request.open('GET', 'https://mywebsite.com/endpoint.php');
request.send();

facebook-github-bot · 2016-04-06T16:14:22Z

By analyzing the blame information on this pull request, we identified @lexs, @nicklockwood and @mkonicek to be potential reviewers.

mkonicek · 2016-04-07T13:59:23Z

I'll let @lexs review this one.

mkonicek · 2016-04-07T14:00:31Z

Do we have an XHR example in the UI Explorer? Can you add this there?

Can you add some unit / integration tests?

lexs · 2016-04-07T17:40:17Z

ReactAndroid/src/main/java/com/facebook/react/modules/network/NetworkingModule.java

@@ -326,6 +326,9 @@ private void onRequestError(ExecutorToken ExecutorToken, int requestId, String e
    WritableArray args = Arguments.createArray();
    args.pushInt(requestId);
    args.pushString(error);
+    if (error.equals("timeout")) {


Can you make this instead of using the string perhaps pass down the IOException? I'm not sure exactly what the exception type it'll be but perhaps SocketTimeoutException. Then just compare the getClass(). This avoids depending on an implementation detail somewhere setting "timeout" as the message.

It's probably easiest to split onRequestError() into two methods, one taking an error string and one taking an exception.

facebook-github-bot · 2016-04-09T18:10:21Z

@grgmo updated the pull request.

facebook-github-bot · 2016-04-09T18:12:25Z

@grgmo updated the pull request.

facebook-github-bot · 2016-04-09T18:15:20Z

@grgmo updated the pull request.

eslint-bot · 2016-04-09T18:17:30Z

Examples/UIExplorer/XHRExampleOnTimeOut.js

+
+  xhr: XMLHttpRequest;
+
+  constructor(props) {


parameter props Missing annotation

facebook-github-bot · 2016-04-09T19:07:17Z

@grgmo updated the pull request.

eslint-bot · 2016-04-09T19:11:54Z

Libraries/Network/XMLHttpRequestBase.js

      // We should send an event to handler, but since we don't process that
      // event anywhere, let's leave it empty
-      onload(null);
+      newEvent(null);
    }
  }


eol-last: Newline required at end of file but not found.

facebook-github-bot · 2016-04-09T20:56:15Z

@grgmo updated the pull request.

facebook-github-bot · 2016-04-10T14:17:23Z

@grgmo updated the pull request.

eslint-bot · 2016-04-10T14:22:26Z

Libraries/Network/__tests__/XMLHttpRequestBase-test.js

+    	xhr._didCompleteResponse(1, 'Generic error');
+
+    	expect(xhr.onerror).toBeCalledWith(null);
+    	expect(xhr.ontimeout).not.toBeCalled();


no-mixed-spaces-and-tabs: Mixed spaces and tabs.

facebook-github-bot · 2016-04-10T14:56:44Z

@grgmo updated the pull request.

grgmo · 2016-04-10T15:44:57Z

@lexs i've made the changes, added unit test and example on UIExplorer. Comments from eslint-bot are also fixed. I need to update the Network documentation if you're ok with the changes.

lexs · 2016-04-14T10:25:41Z

@facebook-github-bot shipit

lexs · 2016-04-14T10:26:04Z

Thanks! Looks great, sorry for taking a few days coming back to this.

facebook-github-bot · 2016-04-14T10:27:26Z

Thanks for importing. If you are an FB employee go to Phabricator to review.

lexs · 2016-04-14T10:35:45Z

@grgmo Seems like there's merge issue, could you rebase and I'll reimport?

grgmo · 2016-04-14T10:45:02Z

@lexs on it, will update the doc too. thanks for reviewing

skellock · 2016-04-14T11:39:33Z

Thank you patchers and mergers. <3

skellock · 2016-04-14T11:48:22Z

@grgmo i know this PR is wrapping up, and the following question changes nothing, but i'm curious: does onerror bubble up some semblance of a error code?

I ask because in my testing, i've seen the error messages appear differently on different devices:

For example:

<= iOS 8.2 The operation couldn’t be completed. (NSURLErrorDomain error -1001.)
>= iOS 8.3 The request timed out.
android 22 - SSL handshake timed out (if a low timeout is set and server is ssl)
android 22 - timeout (if a high timeout is set)
android 16 - Read timed out (if a high timeout is set)

Not just timeouts too. Also non-http server errors. Seems like iOS 8.2 and under at least manifested the underlying error code, but that stopped at 8.3.

And Android... well... ¯\_(ツ)_/¯

TL;DR: Any chance we could harvest the underlying error codes? I wouldn't mind tackling that work in native side, but it feels a bit weird to decorate XMLHttpRequest with extra things that aren't in the spec. (potentially over/underthinking it).

[edit 1: had to wrap my shrugger in backticks cuz he lost his right forearm)

lexs · 2016-04-14T11:55:29Z

@skellock Yeah, problem with network errors is that there is no real standard way of expressing them as with HTTP error codes (404 etc). We could try to consolidate them (by exception type?) but as you said the XMLHttpRequest API isn't very granular and apart from ontimeout and the like.

skellock · 2016-04-14T11:59:41Z

Well this PR is a beautiful first step. Now that I know the difference between timeout and other, that's good enough to relay better info on to the user. Thank you.

facebook-github-bot · 2016-04-14T12:48:17Z

@grgmo updated the pull request.

facebook-github-bot · 2016-04-14T17:01:20Z

@grgmo updated the pull request.

grgmo · 2016-04-15T08:48:59Z

@lexs rebased

lexs · 2016-04-15T09:54:30Z

@facebook-github-bot shipit

facebook-github-bot · 2016-04-15T10:03:00Z

Thanks for importing. If you are an FB employee go to Phabricator to review.

…st for Android and iOS Summary:Currently React-Native does not have `ontimeout` and `onerror` handlers for [XMLHttpRequest](https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest). This is an extension to [No timeout on XMLHttpRequest](facebook#4648). With addition to two handlers, both Android and iOS can now handle `ontimeout` if request times out and `onerror` when there is general network error. **Test plan** Code has been tested on both Android and iOS with [Charles](https://www.charlesproxy.com/) by setting a breakpoint on the request which fires `ontimeout` when the request waits beyond `timeout` time and `onerror` when there is network error. **Usage** JavaScript - ``` var request = new XMLHttpRequest(); function onLoad() { console.log(request.status); }; function onTimeout() { console.log('Timeout'); }; function onError() { console.log('General network error'); }; request.onload = onLoad; request.ontimeout = onTimeout; request.onerr Closes facebook#6841 Differential Revision: D3178859 Pulled By: lexs fb-gh-sync-id: 30674570653e92ab5f7e74bd925dd5640fc862b6 fbshipit-source-id: 30674570653e92ab5f7e74bd925dd5640fc862b6

facebook-github-bot added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Apr 6, 2016

mkonicek assigned lexs Apr 7, 2016

lexs reviewed Apr 7, 2016
View reviewed changes

grgmo force-pushed the xmlhttprequest-add-missing-ontimeout-onerror-handlers branch from 2c85103 to c58e929 Compare April 9, 2016 18:09

grgmo force-pushed the xmlhttprequest-add-missing-ontimeout-onerror-handlers branch from c58e929 to 7873aa9 Compare April 9, 2016 18:11

grgmo force-pushed the xmlhttprequest-add-missing-ontimeout-onerror-handlers branch from 7873aa9 to 3fae0bc Compare April 9, 2016 18:13

eslint-bot reviewed Apr 9, 2016
View reviewed changes

grgmo force-pushed the xmlhttprequest-add-missing-ontimeout-onerror-handlers branch from 3fae0bc to ab924b8 Compare April 9, 2016 19:06

eslint-bot reviewed Apr 9, 2016
View reviewed changes

grgmo force-pushed the xmlhttprequest-add-missing-ontimeout-onerror-handlers branch from ab924b8 to 655e647 Compare April 9, 2016 20:55

grgmo force-pushed the xmlhttprequest-add-missing-ontimeout-onerror-handlers branch from 655e647 to 3f3c8f5 Compare April 10, 2016 14:16

eslint-bot reviewed Apr 10, 2016
View reviewed changes

grgmo force-pushed the xmlhttprequest-add-missing-ontimeout-onerror-handlers branch from 3f3c8f5 to 64940ee Compare April 10, 2016 14:56

lexs added GH Review: accepted and removed GH Review: review-needed labels Apr 14, 2016

skellock mentioned this pull request Apr 14, 2016

Error Message Detection on React Native infinitered/apisauce#7

Closed

grgmo force-pushed the xmlhttprequest-add-missing-ontimeout-onerror-handlers branch from 64940ee to 6332a4a Compare April 14, 2016 12:46

Add support for ontimeout and onerror handler when using XMLHttpRequest

235c24e

grgmo force-pushed the xmlhttprequest-add-missing-ontimeout-onerror-handlers branch from 6332a4a to 235c24e Compare April 14, 2016 17:00

ghost closed this in d09cd62 Apr 15, 2016

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[XMLHttpRequest] Add support for ontimeout and onerror handler when using XMLHttpRequest for Android and iOS #6841

[XMLHttpRequest] Add support for ontimeout and onerror handler when using XMLHttpRequest for Android and iOS #6841

grgmo commented Apr 6, 2016

facebook-github-bot commented Apr 6, 2016

mkonicek commented Apr 7, 2016

mkonicek commented Apr 7, 2016

lexs Apr 7, 2016

facebook-github-bot commented Apr 9, 2016

facebook-github-bot commented Apr 9, 2016

facebook-github-bot commented Apr 9, 2016

eslint-bot Apr 9, 2016

eslint-bot Apr 9, 2016

facebook-github-bot commented Apr 9, 2016

eslint-bot Apr 9, 2016

facebook-github-bot commented Apr 9, 2016

facebook-github-bot commented Apr 10, 2016

eslint-bot Apr 10, 2016

facebook-github-bot commented Apr 10, 2016

grgmo commented Apr 10, 2016

lexs commented Apr 14, 2016

lexs commented Apr 14, 2016

facebook-github-bot commented Apr 14, 2016

lexs commented Apr 14, 2016

grgmo commented Apr 14, 2016

skellock commented Apr 14, 2016

skellock commented Apr 14, 2016

lexs commented Apr 14, 2016

skellock commented Apr 14, 2016

facebook-github-bot commented Apr 14, 2016

facebook-github-bot commented Apr 14, 2016

grgmo commented Apr 15, 2016

lexs commented Apr 15, 2016

facebook-github-bot commented Apr 15, 2016

[XMLHttpRequest] Add support for ontimeout and onerror handler when using XMLHttpRequest for Android and iOS #6841

[XMLHttpRequest] Add support for ontimeout and onerror handler when using XMLHttpRequest for Android and iOS #6841

Conversation

grgmo commented Apr 6, 2016

facebook-github-bot commented Apr 6, 2016

mkonicek commented Apr 7, 2016

mkonicek commented Apr 7, 2016

lexs Apr 7, 2016

Choose a reason for hiding this comment

facebook-github-bot commented Apr 9, 2016

facebook-github-bot commented Apr 9, 2016

facebook-github-bot commented Apr 9, 2016

eslint-bot Apr 9, 2016

Choose a reason for hiding this comment

eslint-bot Apr 9, 2016

Choose a reason for hiding this comment

facebook-github-bot commented Apr 9, 2016

eslint-bot Apr 9, 2016

Choose a reason for hiding this comment

facebook-github-bot commented Apr 9, 2016

facebook-github-bot commented Apr 10, 2016

eslint-bot Apr 10, 2016

Choose a reason for hiding this comment

facebook-github-bot commented Apr 10, 2016

grgmo commented Apr 10, 2016

lexs commented Apr 14, 2016

lexs commented Apr 14, 2016

facebook-github-bot commented Apr 14, 2016

lexs commented Apr 14, 2016

grgmo commented Apr 14, 2016

skellock commented Apr 14, 2016

skellock commented Apr 14, 2016

lexs commented Apr 14, 2016

skellock commented Apr 14, 2016

facebook-github-bot commented Apr 14, 2016

facebook-github-bot commented Apr 14, 2016

grgmo commented Apr 15, 2016

lexs commented Apr 15, 2016

facebook-github-bot commented Apr 15, 2016