From 8bab8264c29c1fde673618cafb93c86dc82f4bb4 Mon Sep 17 00:00:00 2001 From: Bryan Bonnet Date: Sat, 5 May 2018 19:38:37 -0700 Subject: [PATCH 1/2] fix(ajax): Handle timeouts as errors --- spec/observables/dom/ajax-spec.ts | 72 ++++++++++++------- src/internal/observable/dom/AjaxObservable.ts | 18 +++-- 2 files changed, 59 insertions(+), 31 deletions(-) diff --git a/spec/observables/dom/ajax-spec.ts b/spec/observables/dom/ajax-spec.ts index 598427dd0b..1d8ef27591 100644 --- a/spec/observables/dom/ajax-spec.ts +++ b/spec/observables/dom/ajax-spec.ts @@ -380,6 +380,38 @@ describe('Observable.ajax', () => { }); }); + it('should error on timeout of asynchronous request', (done) => { + const obj: Rx.AjaxRequest = { + url: '/flibbertyJibbet', + responseType: 'text', + timeout: 10 + }; + + Rx.Observable.ajax(obj) + .subscribe((x: any) => { + throw 'should not have been called'; + }, (e) => { + expect(e.status).to.equal(0); + expect(e.xhr.method).to.equal('GET'); + expect(e.xhr.async).to.equal(true); + expect(e.xhr.timeout).to.equal(10); + expect(e.xhr.responseType).to.equal('text'); + }); + + const request = MockXMLHttpRequest.mostRecent; + + expect(request.url).to.equal('/flibbertyJibbet'); + + setTimeout(() => { + request.respondWith({ + 'status': 200, + 'contentType': 'text/plain', + 'responseText': 'Wee! I am text!' + }); + done(); + }, 1000); + }); + it('should create a synchronous request', () => { const obj: Rx.AjaxRequest = { url: '/flibbertyJibbet', @@ -1028,8 +1060,20 @@ class MockXMLHttpRequest { MockXMLHttpRequest.requests.push(this); } + timeout: number; + send(data: any): void { this.data = data; + if (this.timeout && this.timeout > 0) { + setTimeout(() => { + if (this.readyState != 4) { + this.readyState = 4; + this.status = 0; + this.triggerEvent('readystatechange'); + this.triggerEvent('timeout'); + } + }, this.timeout); + } } open(method: any, url: any, async: any, user: any, password: any): void { @@ -1039,7 +1083,7 @@ class MockXMLHttpRequest { this.user = user; this.password = password; this.readyState = 1; - this.triggerEvent('readyStateChange'); + this.triggerEvent('readystatechange'); const originalProgressHandler = this.upload.onprogress; Object.defineProperty(this.upload, 'progress', { get() { @@ -1052,24 +1096,6 @@ class MockXMLHttpRequest { this.requestHeaders[key] = value; } - addEventListener(name: string, handler: any): void { - this.eventHandlers.push({ name: name, handler: handler }); - } - - removeEventListener(name: string, handler: any): void { - for (let i = this.eventHandlers.length - 1; i--; ) { - let eh = this.eventHandlers[i]; - if (eh.name === name && eh.handler === handler) { - this.eventHandlers.splice(i, 1); - } - } - } - - throwError(err: any): void { - // TODO: something better with errors - this.triggerEvent('error'); - } - protected jsonResponseValue(response: any) { try { this.response = JSON.parse(response.responseText); @@ -1117,17 +1143,11 @@ class MockXMLHttpRequest { triggerEvent(name: any, eventObj?: any): void { // TODO: create a better default event - const e: any = eventObj || {}; + const e: any = eventObj || { type: name }; if (this['on' + name]) { this['on' + name](e); } - - this.eventHandlers.forEach(function (eh) { - if (eh.name === name) { - eh.handler.call(this, e); - } - }); } triggerUploadEvent(name: any, eventObj?: any): void { diff --git a/src/internal/observable/dom/AjaxObservable.ts b/src/internal/observable/dom/AjaxObservable.ts index d708d39c69..e243d487ae 100644 --- a/src/internal/observable/dom/AjaxObservable.ts +++ b/src/internal/observable/dom/AjaxObservable.ts @@ -353,7 +353,15 @@ export class AjaxSubscriber extends Subscriber { } function xhrReadyStateChange(this: XMLHttpRequest, e: Event) { - const { subscriber, progressSubscriber, request } = (xhrReadyStateChange); + return; + } + xhr.onreadystatechange = xhrReadyStateChange; + (xhrReadyStateChange).subscriber = this; + (xhrReadyStateChange).progressSubscriber = progressSubscriber; + (xhrReadyStateChange).request = request; + + function xhrLoad(this: XMLHttpRequest, e: Event) { + const { subscriber, progressSubscriber, request } = (xhrLoad); if (this.readyState === 4) { // normalize IE9 bug (http://bugs.jquery.com/ticket/1450) let status: number = this.status === 1223 ? 204 : this.status; @@ -382,10 +390,10 @@ export class AjaxSubscriber extends Subscriber { } } } - xhr.onreadystatechange = xhrReadyStateChange; - (xhrReadyStateChange).subscriber = this; - (xhrReadyStateChange).progressSubscriber = progressSubscriber; - (xhrReadyStateChange).request = request; + xhr.onload = xhrLoad; + (xhrLoad).subscriber = this; + (xhrLoad).progressSubscriber = progressSubscriber; + (xhrLoad).request = request; } unsubscribe() { From 7766822cf2d72d2be058da37165e66ab2a773939 Mon Sep 17 00:00:00 2001 From: Bryan Bonnet Date: Wed, 9 May 2018 17:54:49 -0700 Subject: [PATCH 2/2] fix(ajax): Use TestScheduler in timeout test --- spec/observables/dom/ajax-spec.ts | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/spec/observables/dom/ajax-spec.ts b/spec/observables/dom/ajax-spec.ts index 1d8ef27591..cec73a7132 100644 --- a/spec/observables/dom/ajax-spec.ts +++ b/spec/observables/dom/ajax-spec.ts @@ -2,8 +2,10 @@ import { expect } from 'chai'; import * as sinon from 'sinon'; import * as Rx from 'rxjs/Rx'; import { root } from 'rxjs/util/root'; +import { TestScheduler } from 'rxjs/testing'; declare const global: any; +declare const rxTestScheduler: TestScheduler; /** @test {ajax} */ describe('Observable.ajax', () => { @@ -380,7 +382,7 @@ describe('Observable.ajax', () => { }); }); - it('should error on timeout of asynchronous request', (done) => { + it('should error on timeout of asynchronous request', () => { const obj: Rx.AjaxRequest = { url: '/flibbertyJibbet', responseType: 'text', @@ -402,14 +404,15 @@ describe('Observable.ajax', () => { expect(request.url).to.equal('/flibbertyJibbet'); - setTimeout(() => { + rxTestScheduler.schedule(() => { request.respondWith({ 'status': 200, 'contentType': 'text/plain', 'responseText': 'Wee! I am text!' }); - done(); }, 1000); + + rxTestScheduler.flush(); }); it('should create a synchronous request', () => {