From 1a83041142a95c730cca8362e2a30fe063bd1bad Mon Sep 17 00:00:00 2001 From: LongYinan Date: Fri, 23 Dec 2016 09:06:51 +0800 Subject: [PATCH] fix(ajax): upload progress is now set correctly (#2200) * feat(ajax-helper): update MockXMLHttpRequest upload property behavior Observable.ajax would set xhr.upload.onprogress when progressSubscriber is not empty. And if xhr.upload.onprogress was set after xhr.open, it would not be fired. So ajax-helper should initial an empty upload object, and freeze the setter after open method called. * feat(ajax-spec): support upload progress mock * fix(ajax): upload progress event is not set correctly --- spec/helpers/ajax-helper.ts | 31 +++++++++- spec/observables/dom/ajax-spec.ts | 85 ++++++++++++++++++++-------- src/observable/dom/AjaxObservable.ts | 18 ++++-- 3 files changed, 103 insertions(+), 31 deletions(-) diff --git a/spec/helpers/ajax-helper.ts b/spec/helpers/ajax-helper.ts index 60ae10113a..1f22acf82c 100644 --- a/spec/helpers/ajax-helper.ts +++ b/spec/helpers/ajax-helper.ts @@ -139,7 +139,7 @@ export class MockXMLHttpRequest { onerror: (e: ErrorEvent) => any; onprogress: (e: ProgressEvent) => any; ontimeout: (e: ProgressEvent) => any; - upload: XMLHttpRequestUpload; + upload: XMLHttpRequestUpload = { }; constructor() { this.previousRequest = MockXMLHttpRequest.recentRequest; @@ -158,6 +158,12 @@ export class MockXMLHttpRequest { this.password = password; this.readyState = 1; this.triggerEvent('readyStateChange'); + const originalProgressHandler = this.upload.onprogress; + Object.defineProperty(this.upload, 'progress', { + get() { + return originalProgressHandler; + } + }); } setRequestHeader(key: any, value: any): void { @@ -194,7 +200,12 @@ export class MockXMLHttpRequest { throw new Error('unhandled type "' + this.responseType + '"'); } - respondWith(response: any): void { + respondWith(response: any, progressTimes?: number): void { + if (progressTimes) { + for (let i = 1; i <= progressTimes; ++ i) { + this.triggerUploadEvent('progress', { type: 'ProgressEvent', total: progressTimes, loaded: i }); + } + } this.readyState = 4; this.responseHeaders = { 'Content-Type': response.contentType || 'text/plain' @@ -232,6 +243,15 @@ export class MockXMLHttpRequest { } }); } + + triggerUploadEvent(name: any, eventObj?: any): void { + // TODO: create a better default event + const e: any = eventObj || {}; + + if (this.upload['on' + name]) { + this.upload['on' + name](e); + } + } } export class MockXMLHttpRequestInternetExplorer extends MockXMLHttpRequest { @@ -258,4 +278,11 @@ export class MockXMLHttpRequestInternetExplorer extends MockXMLHttpRequest { return super.defaultResponseValue(); } + triggerUploadEvent(name: any, eventObj?: any): void { + // TODO: create a better default event + const e: any = eventObj || {}; + if (this['on' + name]) { + this['on' + name](e); + } + } } diff --git a/spec/observables/dom/ajax-spec.ts b/spec/observables/dom/ajax-spec.ts index 9914a43110..51c8285d34 100644 --- a/spec/observables/dom/ajax-spec.ts +++ b/spec/observables/dom/ajax-spec.ts @@ -652,6 +652,67 @@ describe('Observable.ajax', () => { expect(complete).to.be.true; }); + it('should emit progress event when progressSubscriber is specified', function() { + const spy = sinon.spy(); + const progressSubscriber = ({ + next: spy, + error: () => { + // noop + }, + complete: () => { + // noop + } + }); + + Rx.Observable.ajax({ + url: '/flibbertyJibbet', + progressSubscriber + }) + .subscribe(); + + const request = MockXMLHttpRequest.mostRecent; + + request.respondWith({ + 'status': 200, + 'contentType': 'application/json', + 'responseText': JSON.stringify({}) + }, 3); + + expect(spy).to.be.calledThrice; + }); + + it('should emit progress event when progressSubscriber is specified in IE', function() { + const spy = sinon.spy(); + const progressSubscriber = ({ + next: spy, + error: () => { + // noop + }, + complete: () => { + // noop + } + }); + + root.XMLHttpRequest = MockXMLHttpRequestInternetExplorer; + root.XDomainRequest = MockXMLHttpRequestInternetExplorer; + + Rx.Observable.ajax({ + url: '/flibbertyJibbet', + progressSubscriber + }) + .subscribe(); + + const request = MockXMLHttpRequest.mostRecent; + + request.respondWith({ + 'status': 200, + 'contentType': 'application/json', + 'responseText': JSON.stringify({}) + }, 3); + + expect(spy.callCount).to.equal(3); + }); + }); it('should work fine when XMLHttpRequest onreadystatechange property is monkey patched', function() { @@ -734,16 +795,6 @@ describe('Observable.ajax', () => { configurable: true }); - Object.defineProperty(root.XMLHttpRequest.prototype, 'upload', { - get() { - return true; - }, - configurable: true - }); - - // mock for onprogress - root.XDomainRequest = true; - Rx.Observable.ajax({ url: '/flibbertyJibbet', progressSubscriber: ({ @@ -763,12 +814,11 @@ describe('Observable.ajax', () => { const request = MockXMLHttpRequest.mostRecent; expect(() => { - request.onprogress(('onprogress')); + request.upload.onprogress(('onprogress')); }).not.throw(); delete root.XMLHttpRequest.prototype.onprogress; delete root.XMLHttpRequest.prototype.upload; - delete root.XDomainRequest; }); it('should work fine when XMLHttpRequest onerror property is monkey patched', function() { @@ -788,16 +838,6 @@ describe('Observable.ajax', () => { configurable: true }); - Object.defineProperty(root.XMLHttpRequest.prototype, 'upload', { - get() { - return true; - }, - configurable: true - }); - - // mock for onprogress - root.XDomainRequest = true; - Rx.Observable.ajax({ url: '/flibbertyJibbet' }) @@ -813,6 +853,5 @@ describe('Observable.ajax', () => { delete root.XMLHttpRequest.prototype.onerror; delete root.XMLHttpRequest.prototype.upload; - delete root.XDomainRequest; }); }); \ No newline at end of file diff --git a/src/observable/dom/AjaxObservable.ts b/src/observable/dom/AjaxObservable.ts index 4c558af454..b5e2e0365b 100644 --- a/src/observable/dom/AjaxObservable.ts +++ b/src/observable/dom/AjaxObservable.ts @@ -224,7 +224,12 @@ export class AjaxSubscriber extends Subscriber { } else { this.xhr = xhr; - // open XHR first + // set up the events before open XHR + // https://developer.mozilla.org/en/docs/Web/API/XMLHttpRequest/Using_XMLHttpRequest + // You need to add the event listeners before calling open() on the request. + // Otherwise the progress events will not fire. + this.setupEvents(xhr, request); + // open XHR let result: any; if (user) { result = tryCatch(xhr.open).call(xhr, method, url, async, user, password); @@ -244,9 +249,6 @@ export class AjaxSubscriber extends Subscriber { // set headers this.setHeaders(xhr, headers); - // now set up the events - this.setupEvents(xhr, request); - // finally send the request result = body ? tryCatch(xhr.send).call(xhr, body) : tryCatch(xhr.send).call(xhr); if (result === errorObject) { @@ -304,14 +306,18 @@ export class AjaxSubscriber extends Subscriber { (xhrTimeout).request = request; (xhrTimeout).subscriber = this; (xhrTimeout).progressSubscriber = progressSubscriber; - if (xhr.upload && 'withCredentials' in xhr && root.XDomainRequest) { + if (xhr.upload && 'withCredentials' in xhr) { if (progressSubscriber) { let xhrProgress: (e: ProgressEvent) => void; xhrProgress = function(e: ProgressEvent) { const { progressSubscriber } = (xhrProgress); progressSubscriber.next(e); }; - xhr.onprogress = xhrProgress; + if (root.XDomainRequest) { + xhr.onprogress = xhrProgress; + } else { + xhr.upload.onprogress = xhrProgress; + } (xhrProgress).progressSubscriber = progressSubscriber; } let xhrError: (e: ErrorEvent) => void;