fix(xhr): inner onreadystatechange should not triigger Zone callback (#…

…800) fix angular/angular#17167, zone.js will patch xhr, and add onreadystatechange internally, this onreadystatechange just for invoke the xhr MacroTask, so it should not trigger ZoneSpec's callback. in this commit, use original native addEventListener/removeEventListener to add internal onreadystatechange event listener.
angular · Jun 5, 2017 · 7bd1418 · 7bd1418
1 parent 857929d
commit 7bd1418
Show file tree

Hide file tree

Showing 4 changed files with 54 additions and 10 deletions.
diff --git a/lib/browser/browser.ts b/lib/browser/browser.ts
@@ -91,8 +91,11 @@ Zone.__load_patch('XHR', (global: any, Zone: ZoneType, api: _ZonePrivate) => {
       const data = <XHROptions>task.data;
       // remove existing event listener
       const listener = data.target[XHR_LISTENER];
+      const oriAddListener = data.target[zoneSymbol('addEventListener')];
+      const oriRemoveListener = data.target[zoneSymbol('removeEventListener')];
+
       if (listener) {
-        data.target.removeEventListener('readystatechange', listener);
+        oriRemoveListener.apply(data.target, ['readystatechange', listener]);
       }
       const newListener = data.target[XHR_LISTENER] = () => {
         if (data.target.readyState === data.target.DONE) {
@@ -104,7 +107,7 @@ Zone.__load_patch('XHR', (global: any, Zone: ZoneType, api: _ZonePrivate) => {
           }
         }
       };
-      data.target.addEventListener('readystatechange', newListener);
+      oriAddListener.apply(data.target, ['readystatechange', newListener]);
 
       const storedTask: Task = data.target[XHR_TASK];
       if (!storedTask) {

diff --git a/test/browser/XMLHttpRequest.spec.ts b/test/browser/XMLHttpRequest.spec.ts
@@ -6,7 +6,7 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
-import {ifEnvSupports} from '../test-util';
+import {ifEnvSupports, supportPatchXHROnProperty} from '../test-util';
 
 describe('XMLHttpRequest', function() {
   let testZone: Zone;
@@ -26,14 +26,14 @@ describe('XMLHttpRequest', function() {
         // The last entry in the log should be the invocation for the current onload,
         // which will vary depending on browser environment. The prior entries
         // should be the invocation of the send macrotask.
-        expect(wtfMock.log[wtfMock.log.length - 5])
-            .toMatch(/\> Zone\:invokeTask.*addEventListener\:readystatechange/);
-        expect(wtfMock.log[wtfMock.log.length - 4])
-            .toEqual('> Zone:invokeTask:XMLHttpRequest.send("<root>::ProxyZone::WTF::TestZone")');
         expect(wtfMock.log[wtfMock.log.length - 3])
-            .toEqual('< Zone:invokeTask:XMLHttpRequest.send');
+            .toEqual('> Zone:invokeTask:XMLHttpRequest.send("<root>::ProxyZone::WTF::TestZone")');
         expect(wtfMock.log[wtfMock.log.length - 2])
-            .toMatch(/\< Zone\:invokeTask.*addEventListener\:readystatechange/);
+            .toEqual('< Zone:invokeTask:XMLHttpRequest.send');
+        if (supportPatchXHROnProperty()) {
+          expect(wtfMock.log[wtfMock.log.length - 1])
+              .toMatch(/\> Zone\:invokeTask.*addEventListener\:load/);
+        }
         done();
       };
 
@@ -44,6 +44,32 @@ describe('XMLHttpRequest', function() {
     }, null, null, 'unit-test');
   });
 
+  it('should not trigger Zone callback of internal onreadystatechange', function(done) {
+    const scheduleSpy = jasmine.createSpy('schedule');
+    const xhrZone = Zone.current.fork({
+      name: 'xhr',
+      onScheduleTask: (delegate: ZoneDelegate, currentZone: Zone, targetZone, task: Task) => {
+        if (task.type === 'eventTask') {
+          scheduleSpy(task.source);
+        }
+        return delegate.scheduleTask(targetZone, task);
+      }
+    });
+
+    xhrZone.run(() => {
+      const req = new XMLHttpRequest();
+      req.onload = function() {
+        expect(Zone.current.name).toEqual('xhr');
+        if (supportPatchXHROnProperty()) {
+          expect(scheduleSpy).toHaveBeenCalled();
+        }
+        done();
+      };
+      req.open('get', '/', true);
+      req.send();
+    });
+  });
+
   it('should work with onreadystatechange', function(done) {
     let req: XMLHttpRequest;
 

diff --git a/test/test-util.ts b/test/test-util.ts
@@ -57,3 +57,14 @@ function _runTest(test: any, block: Function, done: Function) {
     });
   }
 }
+
+export function supportPatchXHROnProperty() {
+  let desc = Object.getOwnPropertyDescriptor(XMLHttpRequest.prototype, 'onload');
+  if (!desc && (window as any)['XMLHttpRequestEventTarget']) {
+    desc = Object.getOwnPropertyDescriptor(global['XMLHttpRequestEventTarget'].prototype, 'onload');
+  }
+  if (!desc || !desc.configurable) {
+    return false;
+  }
+  return true;
+}
diff --git a/test/zone-spec/task-tracking.spec.ts b/test/zone-spec/task-tracking.spec.ts
@@ -8,6 +8,8 @@
 
 import '../../lib/zone-spec/task-tracking';
 
+import {supportPatchXHROnProperty} from '../test-util';
+
 declare const global: any;
 
 describe('TaskTrackingZone', function() {
@@ -47,7 +49,9 @@ describe('TaskTrackingZone', function() {
             setTimeout(() => {
               expect(taskTrackingZoneSpec.macroTasks.length).toBe(0);
               expect(taskTrackingZoneSpec.microTasks.length).toBe(0);
-              expect(taskTrackingZoneSpec.eventTasks.length).not.toBe(0);
+              if (supportPatchXHROnProperty()) {
+                expect(taskTrackingZoneSpec.eventTasks.length).not.toBe(0);
+              }
               taskTrackingZoneSpec.clearEvents();
               expect(taskTrackingZoneSpec.eventTasks.length).toBe(0);
               done();