Fix - Add default timeout to XMLHttpRequest

[bombillazo]

This proposed change is to fix a bug when using Pretender with React Native in Android. If the Pretender request is passthrough and synchronous, Pretender does not assign a timeout to the xhr request variable. When the request is dispatched and reaches native code, it errors out:

Attempt to invoke virtual method 'double java.lang.Double.doubleValue()' on a null object reference

This is due to the Android native library expecting a numeric timeout value.

NativeNetworkingAndroid.js Code

  +sendRequest: (
    method: string,
    url: string,
    requestId: number,
    headers: Array<Header>,
    data: Object,
    responseType: string,
    useIncrementalUpdates: boolean,
    timeout: number,
    withCredentials: boolean,
  ) => void;

NetworkingModule.java Code

@Override
  public void sendRequest(
      String method,
      String url,
      double requestIdAsDouble,
      ReadableArray headers,
      ReadableMap data,
      String responseType,
      boolean useIncrementalUpdates,
      double timeoutAsDouble,
      boolean withCredentials) {

To Dos:

• Update tests in passthrough_test.js since now we are always adding a timeout, and this test will fail.

[bombillazo]

Not sure who I should mention, but based on the contributor list and organization:

@bantic @xg-wang @mike-north @trek

[xg-wang]

This looks fine. I think if you can fix the failing test to say assert.equal(xhr.timeout, 0); it would pass.

That said tho, I'm confused by the fact timeout could be null/undefined here, because if the default is 0 when the xhr is created, i.e.,

let xhr = new XMLHttpRequest()
console.log(xhr.timeout)  // prints 0

Do you know in your case is there code that deletes the timeout?

Also sync xhr are not allowed to set timeout in a document environment per MDN

Timeout shouldn't be used for synchronous XMLHttpRequests requests used in a document environment or it will throw an InvalidAccessError exception.

The following code will throw (https://codepen.io/xg-wang/pen/BaRmXOR)

const xhr = new XMLHttpRequest();
xhr.open('GET', '/server', false);
xhr.timeout = 0; // pen.js:5 Uncaught DOMException: Failed to set the 'timeout' property on 'XMLHttpRequest': Timeouts cannot be set for synchronous requests made from a document.
xhr.send(null);

[bombillazo]

No, I'm not changing the request directly nor indirectly. The package being used by node does not behave like the example you posted though.

According to the source, this is the package Pretender is utilizing: xmlhttprequest-ssl

[xg-wang]

@bombillazo ah that ponyfill is not spec compatible, looks like repo does not have issue section, not sure if we can submit a bug report to xmlhttprequest-ssl.

We can add a compatibility check here but I'd want to add more context to the change, something like

diff --git a/src/create-passthrough.ts b/src/create-passthrough.ts
index f2b3c1d..25e3706 100644
--- a/src/create-passthrough.ts
+++ b/src/create-passthrough.ts
@@ -90,6 +90,12 @@ export function createPassthrough(fakeXHR, nativeXMLHttpRequest) {
     xhr.timeout = fakeXHR.timeout;
     xhr.withCredentials = fakeXHR.withCredentials;
   }
+  // XMLHttpRequest.timeout default initializes to 0, and is not allowed to be used for
+  // synchronous XMLHttpRequests requests in a document environment. However, when a XHR
+  // polyfill does not sets the timeout value, it will throw in React Native environment.
+  // TODO:
+  // synchronous XHR is deprecated, make async the default as XMLHttpRequest.open(),
+  // and throw error if sync XHR has timeout not 0
   if (!xhr.timeout) {
     xhr.timeout = 0; // default XMLHttpRequest timeout
   }
diff --git a/test/passthrough_test.js b/test/passthrough_test.js
index 5734ee5..c26685b 100644
--- a/test/passthrough_test.js
+++ b/test/passthrough_test.js
@@ -112,7 +112,7 @@ describe('passthrough requests', function(config) {
     }
   );

-  it('synchronous request does not have timeout, withCredentials and onprogress event', function(
+  it('synchronous request has timeout=0, withCredentials and onprogress event', function(
     assert
   ) {
     var pretender = this.pretender;
@@ -125,7 +125,7 @@ describe('passthrough requests', function(config) {
       this.send = {
         pretender: pretender,
         apply: function(xhr/*, argument*/) {
-          assert.ok(!('timeout' in xhr));
+          assert.equal(xhr.timeout, 0);
           assert.ok(!('withCredentials' in xhr));
           assert.ok(!('onprogress' in xhr));
           this.pretender.resolve(xhr);
@@ -139,7 +139,6 @@ describe('passthrough requests', function(config) {

     var xhr = new window.XMLHttpRequest();
     xhr.open('POST', '/some/path', false);
-    xhr.timeout = 1000;
     xhr.withCredentials = true;
     xhr.send('some data');
   });

[xg-wang]

https://www.npmjs.com/package/pretender/v/3.4.4

[elwayman02]

This PR appears to have created a new bug:

Error: Failed to set the 'timeout' property on 'XMLHttpRequest': Timeouts cannot be set for synchronous requests made from a document.
    at createPassthrough (webpack://__ember_auto_import__/./node_modules/pretender/dist/pretender.es.js?:1453:17)
    at FakeRequest.passthrough (webpack://__ember_auto_import__/./node_modules/pretender/dist/pretender.es.js?:1493:15)
    at FakeRequest.send (webpack://__ember_auto_import__/./node_modules/pretender/dist/pretender.es.js?:1482:12)
    at takeScreenshot (http://localhost:7357/assets/test-support.js:23526:9)
    at http://localhost:7357/assets/test-support.js:23565:37
    at invokeCallback (http://localhost:7357/assets/vendor.js:71723:17)
    at publish (http://localhost:7357/assets/vendor.js:71706:9)
    at http://localhost:7357/assets/vendor.js:66030:9
    at invoke (http://localhost:7357/assets/vendor.js:64236:16)
    at Queue.flush (http://localhost:7357/assets/vendor.js:64127:13)"

We may need to revisit this bugfix and find a way to solve the original problem without introducing a new regression for scenarios where pretender was previously working.