Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(ajax): Handle timeouts as errors #3653

Merged
merged 2 commits into from
May 21, 2018

Conversation

bbonnet
Copy link
Contributor

@bbonnet bbonnet commented May 6, 2018

Description:
XHR Timeouts are not being handled properly.

It turns out the XHR timeouts fire the readystatechange event (with status=0, readyState=4) and then the timeout event. This is causing next to be called before error.

Confirmed this behavior in Chrome, added a test for timeouts and fixed the behavior by pointing the main event handler to fire on load. Definitely recommend confirming this behavior and compatibility in other browsers.

Fixed up our MockXMLHttpRequest to cause timeouts after the send method is called.
Overall, I want to recommend moving away from this MockXMLHttpRequest so that we aren't re-implementing browser functionality in the tests. I saw that the current browser tests are broken, wanted to take a stab at either fixing those or porting over some integration tests to Karma. Totally open to suggestions there.

This is my first contribution to rxjs, really open to feedback and suggestions!

Related issue (if exists):
#3606

@coveralls
Copy link

coveralls commented May 6, 2018

Coverage Status

Coverage increased (+0.3%) to 96.836% when pulling 7766822 on bbonnet:fix-ajax-timeout into 7e4cef4 on ReactiveX:master.

spec/observables/dom/ajax-spec.ts Outdated Show resolved Hide resolved
@benlesh benlesh merged commit e4128ea into ReactiveX:master May 21, 2018
@benlesh
Copy link
Member

benlesh commented May 21, 2018

Thanks, @bbonnet!

@lock lock bot locked as resolved and limited conversation to collaborators Jun 20, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants