Skip to content
This repository has been archived by the owner on Feb 26, 2024. It is now read-only.

Commit

Permalink
fix(timer): fix #314, setTimeout/interval should return original time…
Browse files Browse the repository at this point in the history 
…rId (#894)
  • Loading branch information
@JiaLiPassion @mhevery
JiaLiPassion authored and mhevery committed Sep 13, 2017
1 parent 8d27f23 commit aec4bd4
Show file tree
Hide file tree
Showing 5 changed files with 75 additions and 65 deletions.
53 changes: 40 additions & 13 deletions lib/common/timers.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
}

Expand All @@ -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.
Expand All @@ -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);
}
Expand Down
31 changes: 12 additions & 19 deletions test/common/setInterval.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand All @@ -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();
});
Expand All @@ -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');
});

Expand Down
42 changes: 16 additions & 26 deletions test/common/setTimeout.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
});
};
Expand All @@ -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');
});

Expand Down Expand Up @@ -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) {
Expand All @@ -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);
});
Expand Down
2 changes: 1 addition & 1 deletion test/zone-spec/fake-async-test.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
12 changes: 6 additions & 6 deletions test/zone-spec/task-tracking.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(() => {
Expand Down Expand Up @@ -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();
});
});
});

0 comments on commit aec4bd4

Please sign in to comment.