Skip to content

Commit

Permalink
fix(ajax): upload progress is now set correctly (#2200)
Browse files Browse the repository at this point in the history 
* 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
  • Loading branch information
@Brooooooklyn @benlesh
Brooooooklyn authored and benlesh committed Dec 23, 2016
1 parent 2269618 commit 1a83041
Show file tree
Hide file tree
Showing 3 changed files with 103 additions and 31 deletions.
31 changes: 29 additions & 2 deletions spec/helpers/ajax-helper.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ export class MockXMLHttpRequest {
onerror: (e: ErrorEvent) => any;
onprogress: (e: ProgressEvent) => any;
ontimeout: (e: ProgressEvent) => any;
upload: XMLHttpRequestUpload;
upload: XMLHttpRequestUpload = <any>{ };

constructor() {
this.previousRequest = MockXMLHttpRequest.recentRequest;
Expand All @@ -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 {
Expand Down Expand Up @@ -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'
Expand Down Expand Up @@ -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 {
Expand All @@ -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);
}
}
}
85 changes: 62 additions & 23 deletions spec/observables/dom/ajax-spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 = (<any>{
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 = (<any>{
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() {
Expand Down Expand Up @@ -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: (<any>{
Expand All @@ -763,12 +814,11 @@ describe('Observable.ajax', () => {
const request = MockXMLHttpRequest.mostRecent;

expect(() => {
request.onprogress((<any>'onprogress'));
request.upload.onprogress((<any>'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() {
Expand All @@ -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'
})
Expand All @@ -813,6 +853,5 @@ describe('Observable.ajax', () => {

delete root.XMLHttpRequest.prototype.onerror;
delete root.XMLHttpRequest.prototype.upload;
delete root.XDomainRequest;
});
});
18 changes: 12 additions & 6 deletions src/observable/dom/AjaxObservable.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -224,7 +224,12 @@ export class AjaxSubscriber<T> extends Subscriber<Event> {
} 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);
Expand All @@ -244,9 +249,6 @@ export class AjaxSubscriber<T> extends Subscriber<Event> {
// 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) {
Expand Down Expand Up @@ -304,14 +306,18 @@ export class AjaxSubscriber<T> extends Subscriber<Event> {
(<any>xhrTimeout).request = request;
(<any>xhrTimeout).subscriber = this;
(<any>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 } = (<any>xhrProgress);
progressSubscriber.next(e);
};
xhr.onprogress = xhrProgress;
if (root.XDomainRequest) {
xhr.onprogress = xhrProgress;
} else {
xhr.upload.onprogress = xhrProgress;
}
(<any>xhrProgress).progressSubscriber = progressSubscriber;
}
let xhrError: (e: ErrorEvent) => void;
Expand Down

0 comments on commit 1a83041

Please sign in to comment.