fix(timer): fix #314, setTimeout/interval should return original timerId

lib/common/timers.ts

@@ -10,7 +10,9 @@
  * @suppress {missingRequire}
  */
 
-import {patchMethod} from './utils';
+import {patchMethod, zoneSymbol} from './utils';
+
+const taskSymbol = zoneSymbol('zoneTask');
 
 interface TimerOptions extends TaskData {
   handleId: number;
@@ -38,27 +40,22 @@ export function patchTimer(window: any, setName: string, cancelName: string, nam
         task.invoke.apply(this, arguments);
       } finally {
         if (typeof data.handleId === NUMBER) {
-          // Node returns complex objects as handleIds
+          // in non-nodejs env, we remove timerId
+          // from local cache
           delete tasksByHandleId[data.handleId];
+        } else if (data.handleId) {
+          // Node returns complex objects as handleIds
+          // we remove task reference from timer object
+          (data.handleId as any)[taskSymbol] = null;
         }
       }
     }
     data.args[0] = timer;
     data.handleId = setNative.apply(window, data.args);
-    if (typeof data.handleId === NUMBER) {
-      // Node returns complex objects as handleIds -> no need to keep them around. Additionally,
-      // this throws an
-      // exception in older node versions and has no effect there, because of the stringified key.
-      tasksByHandleId[data.handleId] = task;
-    }
     return task;
   }
 
   function clearTask(task: Task) {
-    if (typeof(<TimerOptions>task.data).handleId === NUMBER) {
-      // Node returns complex objects as handleIds
-      delete tasksByHandleId[(<TimerOptions>task.data).handleId];
-    }
     return clearNative((<TimerOptions>task.data).handleId);
   }
 
@@ -78,13 +75,26 @@ export function patchTimer(window: any, setName: string, cancelName: string, nam
           }
           // Node.js must additionally support the ref and unref functions.
           const handle: any = (<TimerOptions>task.data).handleId;
+          if (typeof handle === NUMBER) {
+            // for non nodejs env, we save handleId: task
+            // mapping in local cache for clearTimeout
+            tasksByHandleId[handle] = task;
+          } else if (handle) {
+            // for nodejs env, we save task
+            // reference in timerId Object for clearTimeout
+            handle[taskSymbol] = task;
+          }
+
           // check whether handle is null, because some polyfill or browser
           // may return undefined from setTimeout/setInterval/setImmediate/requestAnimationFrame
           if (handle && handle.ref && handle.unref && typeof handle.ref === FUNCTION &&
               typeof handle.unref === FUNCTION) {
             (<any>task).ref = (<any>handle).ref.bind(handle);
             (<any>task).unref = (<any>handle).unref.bind(handle);
           }
+          if (typeof handle === NUMBER || handle) {
+            return handle;
+          }
           return task;
         } else {
           // cause an error by calling it directly.
@@ -94,10 +104,27 @@ export function patchTimer(window: any, setName: string, cancelName: string, nam
 
   clearNative =
       patchMethod(window, cancelName, (delegate: Function) => function(self: any, args: any[]) {
-        const task: Task = typeof args[0] === NUMBER ? tasksByHandleId[args[0]] : args[0];
+        const id = args[0];
+        let task: Task;
+        if (typeof id === NUMBER) {
+          // non nodejs env.
+          task = tasksByHandleId[id];
+        } else {
+          // nodejs env.
+          task = id && id[taskSymbol];
+          // other environments.
+          if (!task) {
+            task = id;
+          }
+        }
         if (task && typeof task.type === STRING) {
           if (task.state !== NOT_SCHEDULED &&
               (task.cancelFn && task.data.isPeriodic || task.runCount === 0)) {
+            if (typeof id === NUMBER) {
+              delete tasksByHandleId[id];
+            } else if (id) {
+              id[taskSymbol] = null;
+            }
             // Do not cancel already canceled functions
             task.zone.cancelTask(task);
           }

test/common/setInterval.spec.ts

@@ -16,7 +16,6 @@ describe('setInterval', function() {
     let cancelId: any;
     const testZone = Zone.current.fork((Zone as any)['wtfZoneSpec']).fork({name: 'TestZone'});
     testZone.run(() => {
-      let id: number;
       let intervalCount = 0;
       let timeoutRunning = false;
       const intervalFn = function() {
@@ -35,12 +34,14 @@ describe('setInterval', function() {
           for (let i = 0; i < intervalCount; i++) {
             intervalLog = intervalLog.concat(intervalUnitLog);
           }
-          expect(wtfMock.log).toEqual([
-            '# Zone:fork("<root>::ProxyZone::WTF", "TestZone")',
-            '> Zone:invoke:unit-test("<root>::ProxyZone::WTF::TestZone")',
-            '# Zone:schedule:macroTask:setInterval("<root>::ProxyZone::WTF::TestZone", ' + id + ')',
-            '< Zone:invoke:unit-test'
-          ].concat(intervalLog));
+          expect(wtfMock.log[0]).toEqual('# Zone:fork("<root>::ProxyZone::WTF", "TestZone")');
+          expect(wtfMock.log[1])
+              .toEqual('> Zone:invoke:unit-test("<root>::ProxyZone::WTF::TestZone")');
+          expect(wtfMock.log[2])
+              .toContain(
+                  '# Zone:schedule:macroTask:setInterval("<root>::ProxyZone::WTF::TestZone"');
+          expect(wtfMock.log[3]).toEqual('< Zone:invoke:unit-test');
+          expect(wtfMock.log.splice(4)).toEqual(intervalLog);
           clearInterval(cancelId);
           done();
         });
@@ -52,18 +53,10 @@ describe('setInterval', function() {
         expect(typeof cancelId.unref).toEqual(('function'));
       }
 
-      // This icky replacer is to deal with Timers in node.js. The data.handleId contains timers in
-      // node.js. They do not stringify properly since they contain circular references.
-      id = JSON.stringify((<MacroTask>cancelId).data, function replaceTimer(key, value) {
-        if (key == 'handleId' && typeof value == 'object') return value.constructor.name;
-        if (typeof value === 'function') return value.name;
-        return value;
-      }) as any as number;
-      expect(wtfMock.log).toEqual([
-        '# Zone:fork("<root>::ProxyZone::WTF", "TestZone")',
-        '> Zone:invoke:unit-test("<root>::ProxyZone::WTF::TestZone")',
-        '# Zone:schedule:macroTask:setInterval("<root>::ProxyZone::WTF::TestZone", ' + id + ')'
-      ]);
+      expect(wtfMock.log[0]).toEqual('# Zone:fork("<root>::ProxyZone::WTF", "TestZone")');
+      expect(wtfMock.log[1]).toEqual('> Zone:invoke:unit-test("<root>::ProxyZone::WTF::TestZone")');
+      expect(wtfMock.log[2])
+          .toContain('# Zone:schedule:macroTask:setInterval("<root>::ProxyZone::WTF::TestZone"');
     }, null, null, 'unit-test');
   });
 

test/common/setTimeout.spec.ts

@@ -14,18 +14,18 @@ describe('setTimeout', function() {
     let cancelId: any;
     const testZone = Zone.current.fork((Zone as any)['wtfZoneSpec']).fork({name: 'TestZone'});
     testZone.run(() => {
-      let id: number;
       const timeoutFn = function() {
         expect(Zone.current.name).toEqual(('TestZone'));
         global[zoneSymbol('setTimeout')](function() {
-          expect(wtfMock.log).toEqual([
-            '# Zone:fork("<root>::ProxyZone::WTF", "TestZone")',
-            '> Zone:invoke:unit-test("<root>::ProxyZone::WTF::TestZone")',
-            '# Zone:schedule:macroTask:setTimeout("<root>::ProxyZone::WTF::TestZone", ' + id + ')',
-            '< Zone:invoke:unit-test',
-            '> Zone:invokeTask:setTimeout("<root>::ProxyZone::WTF::TestZone")',
-            '< Zone:invokeTask:setTimeout'
-          ]);
+          expect(wtfMock.log[0]).toEqual('# Zone:fork("<root>::ProxyZone::WTF", "TestZone")');
+          expect(wtfMock.log[1])
+              .toEqual('> Zone:invoke:unit-test("<root>::ProxyZone::WTF::TestZone")');
+          expect(wtfMock.log[2])
+              .toContain('# Zone:schedule:macroTask:setTimeout("<root>::ProxyZone::WTF::TestZone"');
+          expect(wtfMock.log[3]).toEqual('< Zone:invoke:unit-test');
+          expect(wtfMock.log[4])
+              .toEqual('> Zone:invokeTask:setTimeout("<root>::ProxyZone::WTF::TestZone")');
+          expect(wtfMock.log[5]).toEqual('< Zone:invokeTask:setTimeout');
           done();
         });
       };
@@ -35,18 +35,10 @@ describe('setTimeout', function() {
         expect(typeof cancelId.ref).toEqual(('function'));
         expect(typeof cancelId.unref).toEqual(('function'));
       }
-      // This icky replacer is to deal with Timers in node.js. The data.handleId contains timers in
-      // node.js. They do not stringify properly since they contain circular references.
-      id = JSON.stringify((<MacroTask>cancelId).data, function replaceTimer(key, value) {
-        if (key == 'handleId' && typeof value == 'object') return value.constructor.name;
-        if (typeof value === 'function') return value.name;
-        return value;
-      }) as any as number;
-      expect(wtfMock.log).toEqual([
-        '# Zone:fork("<root>::ProxyZone::WTF", "TestZone")',
-        '> Zone:invoke:unit-test("<root>::ProxyZone::WTF::TestZone")',
-        '# Zone:schedule:macroTask:setTimeout("<root>::ProxyZone::WTF::TestZone", ' + id + ')'
-      ]);
+      expect(wtfMock.log[0]).toEqual('# Zone:fork("<root>::ProxyZone::WTF", "TestZone")');
+      expect(wtfMock.log[1]).toEqual('> Zone:invoke:unit-test("<root>::ProxyZone::WTF::TestZone")');
+      expect(wtfMock.log[2])
+          .toContain('# Zone:schedule:macroTask:setTimeout("<root>::ProxyZone::WTF::TestZone"');
     }, null, null, 'unit-test');
   });
 
@@ -97,11 +89,11 @@ describe('setTimeout', function() {
        });
      });
 
-  it('should return the timeout Id through toString', function() {
+  it('should return the original timeout Id', function() {
     // Node returns complex object from setTimeout, ignore this test.
     if (isNode) return;
     const cancelId = setTimeout(() => {}, 0);
-    expect(typeof(cancelId.toString())).toBe('number');
+    expect(typeof cancelId).toEqual('number');
   });
 
   it('should allow cancelation by numeric timeout Id', function(done) {
@@ -114,12 +106,10 @@ describe('setTimeout', function() {
     const testZone = Zone.current.fork((Zone as any)['wtfZoneSpec']).fork({name: 'TestZone'});
     testZone.run(() => {
       const spy = jasmine.createSpy('spy');
-      const task: Task = <any>setTimeout(spy, 0);
-      const cancelId: number = <any>task;
+      const cancelId = setTimeout(spy, 0);
       clearTimeout(cancelId);
       setTimeout(function() {
         expect(spy).not.toHaveBeenCalled();
-        expect(task.runCount).toEqual(0);
         done();
       }, 1);
     });

test/zone-spec/fake-async-test.spec.ts

@@ -193,7 +193,7 @@ describe('FakeAsyncTestZoneSpec', () => {
     it('should not run cancelled timer', () => {
       fakeAsyncTestZone.run(() => {
         let ran = false;
-        let id = setTimeout(() => {
+        let id: any = setTimeout(() => {
           ran = true;
         }, 10);
         clearTimeout(id);

test/zone-spec/task-tracking.spec.ts

@@ -24,14 +24,14 @@ describe('TaskTrackingZone', function() {
 
   it('should track tasks', (done: Function) => {
     taskTrackingZone.run(() => {
-      const microTask = taskTrackingZone.scheduleMicroTask('test1', () => {});
+      taskTrackingZone.scheduleMicroTask('test1', () => {});
       expect(taskTrackingZoneSpec.microTasks.length).toBe(1);
       expect(taskTrackingZoneSpec.microTasks[0].source).toBe('test1');
 
-      const macroTask = setTimeout(() => {}) as any as Task;
+      setTimeout(() => {});
       expect(taskTrackingZoneSpec.macroTasks.length).toBe(1);
       expect(taskTrackingZoneSpec.macroTasks[0].source).toBe('setTimeout');
-      taskTrackingZone.cancelTask(macroTask);
+      taskTrackingZone.cancelTask(taskTrackingZoneSpec.macroTasks[0]);
       expect(taskTrackingZoneSpec.macroTasks.length).toBe(0);
 
       setTimeout(() => {
@@ -71,10 +71,10 @@ describe('TaskTrackingZone', function() {
 
   it('should capture task creation stacktrace', (done) => {
     taskTrackingZone.run(() => {
-      const task: any = setTimeout(() => {
+      setTimeout(() => {
         done();
-      }) as any as Task;
-      expect(task['creationLocation']).toBeTruthy();
+      });
+      expect((taskTrackingZoneSpec.macroTasks[0] as any)['creationLocation']).toBeTruthy();
     });
   });
 });