Fix ajax upload progress event is not set correctly #2200

Brooooooklyn · 2016-12-19T04:41:34Z

Description:
progress event should be set before xhr.open,
see: https://developer.mozilla.org/en/docs/Web/API/XMLHttpRequest/Using_XMLHttpRequest

coveralls · 2016-12-19T04:52:08Z

Coverage increased (+0.02%) to 97.689% when pulling 95b3026 on Brooooooklyn:issue/ajax-upload-progress into dc06e01 on ReactiveX:master.

coveralls · 2016-12-19T05:03:35Z

Coverage increased (+0.02%) to 97.689% when pulling a2d1ff2 on Brooooooklyn:issue/ajax-upload-progress into dc06e01 on ReactiveX:master.

kwonoj · 2016-12-19T05:04:38Z

src/observable/dom/AjaxObservable.ts

@@ -304,14 +303,14 @@ 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) {


is this passes browser test on IE9? Seems IE9 relies on XDomainRequest and xdr.onprogress as eventhandler.

I don't know why root.XDomainRequest here, it caused progressSubscriber only worked in IE9. @Blesh

I think && constraint is probably needed to be updated to deal each cases (if xhr vs else if XDomainRequest), but checking itself is still needed.

I've checked the specs of XDomainRequest, and I'm sure the only difference between xdr and xhr in handling with events is here https://github.com/Brooooooklyn/rxjs/blob/9bc37772a71ec34b2cb5aef9f3f910b1177ee58f/src/observable/dom/AjaxObservable.ts#L316

kwonoj · 2016-12-19T05:06:54Z

spec/helpers/ajax-helper.ts

@@ -158,6 +158,14 @@ export class MockXMLHttpRequest {
    this.password = password;
    this.readyState = 1;
    this.triggerEvent('readyStateChange');
+    // https://developer.mozilla.org/en/docs/Web/API/XMLHttpRequest/Using_XMLHttpRequest


just fyi, since ajax-helper is mock this comment may need to be placed under AjaxObservable implementation.

kwonoj · 2016-12-19T05:12:06Z

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 = Object.create(null);


what about simply do

upload = { progress: { get() { return this.upload.onprogress; } } }

? any strong reason to declare empty object then define property later?
(other than specifying type XMLHttpRequestUpload. This'll be easily solvable with TS2.1 by declare as Partial<XMLHttpRequestUpload>)

It's not about Type declare... I just want mock the behavior that listeners after open called would not be fired. So I just want remove setter on this.upload.progress here

I see. but Object.create(null) is required then?

because https://github.com/Brooooooklyn/rxjs/blob/issue/ajax-upload-progress/src/observable/dom/AjaxObservable.ts#L319 would set xhr.upload.onprogress while progressSubscriber not empty.

OOOOOh I see. sorry for that. Sorry for those.

Seems it'd good to leave small comments to describe reason for this for further note.

updated commit

It seems confusing to fill this property with an empty Object. Also, just use {} instead of Object.create(null) if we do decide to keep this. No reason to be fancy. It's just test code.

coveralls · 2016-12-19T06:55:30Z

Coverage increased (+0.0007%) to 97.674% when pulling 9bc3777 on Brooooooklyn:issue/ajax-upload-progress into dc06e01 on ReactiveX:master.

coveralls · 2016-12-19T07:52:40Z

Coverage increased (+0.0007%) to 97.674% when pulling af6f8b9 on Brooooooklyn:issue/ajax-upload-progress into dc06e01 on ReactiveX:master.

coveralls · 2016-12-19T08:28:01Z

Coverage increased (+0.02%) to 97.69% when pulling a7e9c3a on Brooooooklyn:issue/ajax-upload-progress into dc06e01 on ReactiveX:master.

benlesh · 2016-12-20T00:51:14Z

spec/observables/dom/ajax-spec.ts

+        'responseText': JSON.stringify({})
+      }, 3);
+
+      expect(spy).to.be.calledThrice;


LOL... is this really a thing? "Thrice"? Hahaha... How about just callCount(3) so I don't snicker every time I read it? 😄

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.

coveralls · 2016-12-20T01:56:55Z

Coverage increased (+0.02%) to 97.69% when pulling 8201e76 on Brooooooklyn:issue/ajax-upload-progress into dc06e01 on ReactiveX:master.

Brooooooklyn · 2016-12-21T13:02:11Z

will this commit be landed in next version? I'm working on a fileUploader using RxJS, and I will be happy if this commit available before the end of November.

kwonoj · 2016-12-21T16:13:59Z

if this commit available before the end of November

uh..since it's already nearby x-mas it missed train already? :)

If it's Dec. You mean, it depends on @Blesh but may possibly not be available due to holiday season.

benlesh

Basically, I think we need to ensure our tests are mocking both available paths for upload (as I naively understand them). Both request.onprogress and request.upload.onprogress firing, depending on what's available, should work. Although perhaps I'm just getting myself confused here.

benlesh · 2016-12-21T22:58:21Z

spec/observables/dom/ajax-spec.ts

@@ -763,12 +814,11 @@ describe('Observable.ajax', () => {
    const request = MockXMLHttpRequest.mostRecent;

    expect(() => {
-      request.onprogress((<any>'onprogress'));
+      request.upload.onprogress((<any>'onprogress'));


it seems like we should have to test both paths for this. It's tedious, but I want to make sure this works in every browser we can.

@Brooooooklyn @kwonoj thoughts?

On second thought. I think this can be remedied at a later time. This is fine for now.

benlesh · 2016-12-23T01:06:11Z

LGTM

lock · 2018-06-06T16:40:39Z

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.

Brooooooklyn force-pushed the issue/ajax-upload-progress branch from f84f4d7 to 95b3026 Compare December 19, 2016 04:47

Brooooooklyn force-pushed the issue/ajax-upload-progress branch from 95b3026 to a2d1ff2 Compare December 19, 2016 04:59

kwonoj reviewed Dec 19, 2016

View reviewed changes

Brooooooklyn force-pushed the issue/ajax-upload-progress branch from a2d1ff2 to 9bc3777 Compare December 19, 2016 06:50

Brooooooklyn force-pushed the issue/ajax-upload-progress branch from 9bc3777 to af6f8b9 Compare December 19, 2016 07:47

Brooooooklyn force-pushed the issue/ajax-upload-progress branch from af6f8b9 to a7e9c3a Compare December 19, 2016 08:23

benlesh reviewed Dec 20, 2016

View reviewed changes

Brooooooklyn added 3 commits December 20, 2016 09:50

feat(ajax-spec): support upload progress mock

38022dc

fix(ajax): upload progress event is not set correctly

8201e76

Brooooooklyn force-pushed the issue/ajax-upload-progress branch from a7e9c3a to 8201e76 Compare December 20, 2016 01:52

benlesh added the Next Minor Version label Dec 21, 2016

benlesh requested changes Dec 21, 2016

View reviewed changes

benlesh added Next Patch Version and removed Next Minor Version labels Dec 22, 2016

benlesh approved these changes Dec 23, 2016

View reviewed changes

benlesh merged commit 1a83041 into ReactiveX:master Dec 23, 2016

lock bot locked as resolved and limited conversation to collaborators Jun 6, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix ajax upload progress event is not set correctly #2200

Fix ajax upload progress event is not set correctly #2200

Brooooooklyn commented Dec 19, 2016

coveralls commented Dec 19, 2016 •

edited

Loading

coveralls commented Dec 19, 2016 •

edited

Loading

kwonoj Dec 19, 2016

Brooooooklyn Dec 19, 2016

kwonoj Dec 19, 2016

Brooooooklyn Dec 19, 2016

kwonoj Dec 19, 2016

kwonoj Dec 19, 2016

Brooooooklyn Dec 19, 2016

kwonoj Dec 19, 2016

Brooooooklyn Dec 19, 2016

kwonoj Dec 19, 2016

kwonoj Dec 19, 2016

Brooooooklyn Dec 19, 2016

benlesh Dec 20, 2016

coveralls commented Dec 19, 2016 •

edited

Loading

coveralls commented Dec 19, 2016 •

edited

Loading

coveralls commented Dec 19, 2016 •

edited

Loading

benlesh Dec 20, 2016

Brooooooklyn Dec 20, 2016

coveralls commented Dec 20, 2016 •

edited

Loading

Brooooooklyn commented Dec 21, 2016

kwonoj commented Dec 21, 2016 •

edited

Loading

benlesh left a comment

benlesh Dec 21, 2016

benlesh Dec 23, 2016

benlesh Dec 23, 2016

benlesh commented Dec 23, 2016

lock bot commented Jun 6, 2018

Fix ajax upload progress event is not set correctly #2200

Fix ajax upload progress event is not set correctly #2200

Conversation

Brooooooklyn commented Dec 19, 2016

coveralls commented Dec 19, 2016 • edited Loading

coveralls commented Dec 19, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Dec 19, 2016 • edited Loading

coveralls commented Dec 19, 2016 • edited Loading

coveralls commented Dec 19, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Dec 20, 2016 • edited Loading

Brooooooklyn commented Dec 21, 2016

kwonoj commented Dec 21, 2016 • edited Loading

benlesh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benlesh commented Dec 23, 2016

lock bot commented Jun 6, 2018

coveralls commented Dec 19, 2016 •

edited

Loading

coveralls commented Dec 19, 2016 •

edited

Loading

coveralls commented Dec 19, 2016 •

edited

Loading

coveralls commented Dec 19, 2016 •

edited

Loading

coveralls commented Dec 19, 2016 •

edited

Loading

coveralls commented Dec 20, 2016 •

edited

Loading

kwonoj commented Dec 21, 2016 •

edited

Loading