fix(ajax): Only set timeout & responseType if request is asynchronous

[jkrehm]

Description:

Setting async: false in the call to Observable.ajax results in an error:

DOMException: Failed to set the 'timeout' property on 'XMLHttpRequest': Timeouts cannot be set for synchronous requests made from a document.

You can observe the behavior here: https://jsfiddle.net/jkrehm/k4o9do0q/1/

My solution is simply to wrap the code that sets timeout and responseType (also a problem) in an if-statement so they're only set if async: true (the default).

[coveralls]

Coverage increased (+0.0003%) to 97.689% when pulling 7d5e752 on hp-mobile:fix-sync-ajax into 01e1343 on ReactiveX:master.

[chrisnoringovo]

LGTM, missing an update in a test though shouldnt there be a spec/observables/dom/ajax-spec.ts that needs this case added and asserted?

[jkrehm]

As far as I can tell, the current tests in spec/observables/dom/ajax-spec.ts do not hit this code. They use MockXMLHttpRequest that has its own send function. I spent an hour trying to make the test work before dropping both console.log and throw statements into src/observable/dom/AjaxObservable.ts's send function and ran the tests and neither the log nor exception showed up.

Am I missing something obvious?

Edit: I'm an idiot and didn't rebuild the project code after adding my console.log and throw. Simply building the tests doesn't take care of that. I'll continue trying to debug the test for a bit.

[chrisnoringovo]

look, I'm not deciding anything here, at all. If it was me maintaining something as big as Rxjs I would want to have a test that proves a bug exist. A fix made and a test that shows the bug no longer exist. If it is possible. If really hard to test then maybe some example runs showing the bug seems resolved. For example used to be : "some code" is now : "some code". So if its hard or impossible to test I would just amend the description with a code snippet where it works.. You already added a jsFiddle which is good

[coveralls]

Coverage increased (+0.0003%) to 97.689% when pulling 1e73755 on hp-mobile:fix-sync-ajax into 01e1343 on ReactiveX:master.

[jkrehm]

Added two tests: one for asynchronous, one for synchronous. It required changing the MockXMLHttpRequest a bit to (a) include async as a property and (b) not require that responseType be set (since synchronous calls cannot set that).

Hopefully that provides enough coverage for this issue. It doesn't show a good before/after shot of it working vs not working (the jsFiddle will have to suffice for the "not working" part) but hopefully it helps prevent future regressions.

[jkrehm]

Anything I can do to help this issue get merged? A project I'm working on requires sync requests in one scenario (iOS Safari) and we cannot use it there because of this issue. I'm happy to help however I can to make sure it's included in a future release.

[regou]

At what state about this issue? Can we merge jkrehm's pr?
I understand the Sync XHR is deprecated, but as long as rxjs support it, the code should be done right

[benlesh]

Thanks @jkrehm

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.