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

Commit aec4bd4

Browse files
JiaLiPassionmhevery
JiaLiPassion
authored and
mhevery
committed
fix(timer): fix #314, setTimeout/interval should return original timerId (#894)
1 parent 8d27f23 commit aec4bd4

5 files changed

Lines changed: 75 additions & 65 deletions

File tree

‎lib/common/timers.ts‎

Lines changed: 40 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,9 @@
1010
* @suppress {missingRequire}
1111
*/
1212

13-
import {patchMethod} from './utils';
13+
import {patchMethod, zoneSymbol} from './utils';
14+
15+
const taskSymbol = zoneSymbol('zoneTask');
1416

1517
interface TimerOptions extends TaskData {
1618
handleId: number;
@@ -38,27 +40,22 @@ export function patchTimer(window: any, setName: string, cancelName: string, nam
3840
task.invoke.apply(this, arguments);
3941
} finally {
4042
if (typeof data.handleId === NUMBER) {
41-
// Node returns complex objects as handleIds
43+
// in non-nodejs env, we remove timerId
44+
// from local cache
4245
delete tasksByHandleId[data.handleId];
46+
} else if (data.handleId) {
47+
// Node returns complex objects as handleIds
48+
// we remove task reference from timer object
49+
(data.handleId as any)[taskSymbol] = null;
4350
}
4451
}
4552
}
4653
data.args[0] = timer;
4754
data.handleId = setNative.apply(window, data.args);
48-
if (typeof data.handleId === NUMBER) {
49-
// Node returns complex objects as handleIds -> no need to keep them around. Additionally,
50-
// this throws an
51-
// exception in older node versions and has no effect there, because of the stringified key.
52-
tasksByHandleId[data.handleId] = task;
53-
}
5455
return task;
5556
}
5657

5758
function clearTask(task: Task) {
58-
if (typeof(<TimerOptions>task.data).handleId === NUMBER) {
59-
// Node returns complex objects as handleIds
60-
delete tasksByHandleId[(<TimerOptions>task.data).handleId];
61-
}
6259
return clearNative((<TimerOptions>task.data).handleId);
6360
}
6461

@@ -78,13 +75,26 @@ export function patchTimer(window: any, setName: string, cancelName: string, nam
7875
}
7976
// Node.js must additionally support the ref and unref functions.
8077
const handle: any = (<TimerOptions>task.data).handleId;
78+
if (typeof handle === NUMBER) {
79+
// for non nodejs env, we save handleId: task
80+
// mapping in local cache for clearTimeout
81+
tasksByHandleId[handle] = task;
82+
} else if (handle) {
83+
// for nodejs env, we save task
84+
// reference in timerId Object for clearTimeout
85+
handle[taskSymbol] = task;
86+
}
87+
8188
// check whether handle is null, because some polyfill or browser
8289
// may return undefined from setTimeout/setInterval/setImmediate/requestAnimationFrame
8390
if (handle && handle.ref && handle.unref && typeof handle.ref === FUNCTION &&
8491
typeof handle.unref === FUNCTION) {
8592
(<any>task).ref = (<any>handle).ref.bind(handle);
8693
(<any>task).unref = (<any>handle).unref.bind(handle);
8794
}
95+
if (typeof handle === NUMBER || handle) {
96+
return handle;
97+
}
8898
return task;
8999
} else {
90100
// cause an error by calling it directly.
@@ -94,10 +104,27 @@ export function patchTimer(window: any, setName: string, cancelName: string, nam
94104

95105
clearNative =
96106
patchMethod(window, cancelName, (delegate: Function) => function(self: any, args: any[]) {
97-
const task: Task = typeof args[0] === NUMBER ? tasksByHandleId[args[0]] : args[0];
107+
const id = args[0];
108+
let task: Task;
109+
if (typeof id === NUMBER) {
110+
// non nodejs env.
111+
task = tasksByHandleId[id];
112+
} else {
113+
// nodejs env.
114+
task = id && id[taskSymbol];
115+
// other environments.
116+
if (!task) {
117+
task = id;
118+
}
119+
}
98120
if (task && typeof task.type === STRING) {
99121
if (task.state !== NOT_SCHEDULED &&
100122
(task.cancelFn && task.data.isPeriodic || task.runCount === 0)) {
123+
if (typeof id === NUMBER) {
124+
delete tasksByHandleId[id];
125+
} else if (id) {
126+
id[taskSymbol] = null;
127+
}
101128
// Do not cancel already canceled functions
102129
task.zone.cancelTask(task);
103130
}

‎test/common/setInterval.spec.ts‎

Lines changed: 12 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,6 @@ describe('setInterval', function() {
1616
let cancelId: any;
1717
const testZone = Zone.current.fork((Zone as any)['wtfZoneSpec']).fork({name: 'TestZone'});
1818
testZone.run(() => {
19-
let id: number;
2019
let intervalCount = 0;
2120
let timeoutRunning = false;
2221
const intervalFn = function() {
@@ -35,12 +34,14 @@ describe('setInterval', function() {
3534
for (let i = 0; i < intervalCount; i++) {
3635
intervalLog = intervalLog.concat(intervalUnitLog);
3736
}
38-
expect(wtfMock.log).toEqual([
39-
'# Zone:fork("<root>::ProxyZone::WTF", "TestZone")',
40-
'> Zone:invoke:unit-test("<root>::ProxyZone::WTF::TestZone")',
41-
'# Zone:schedule:macroTask:setInterval("<root>::ProxyZone::WTF::TestZone", ' + id + ')',
42-
'< Zone:invoke:unit-test'
43-
].concat(intervalLog));
37+
expect(wtfMock.log[0]).toEqual('# Zone:fork("<root>::ProxyZone::WTF", "TestZone")');
38+
expect(wtfMock.log[1])
39+
.toEqual('> Zone:invoke:unit-test("<root>::ProxyZone::WTF::TestZone")');
40+
expect(wtfMock.log[2])
41+
.toContain(
42+
'# Zone:schedule:macroTask:setInterval("<root>::ProxyZone::WTF::TestZone"');
43+
expect(wtfMock.log[3]).toEqual('< Zone:invoke:unit-test');
44+
expect(wtfMock.log.splice(4)).toEqual(intervalLog);
4445
clearInterval(cancelId);
4546
done();
4647
});
@@ -52,18 +53,10 @@ describe('setInterval', function() {
5253
expect(typeof cancelId.unref).toEqual(('function'));
5354
}
5455

55-
// This icky replacer is to deal with Timers in node.js. The data.handleId contains timers in
56-
// node.js. They do not stringify properly since they contain circular references.
57-
id = JSON.stringify((<MacroTask>cancelId).data, function replaceTimer(key, value) {
58-
if (key == 'handleId' && typeof value == 'object') return value.constructor.name;
59-
if (typeof value === 'function') return value.name;
60-
return value;
61-
}) as any as number;
62-
expect(wtfMock.log).toEqual([
63-
'# Zone:fork("<root>::ProxyZone::WTF", "TestZone")',
64-
'> Zone:invoke:unit-test("<root>::ProxyZone::WTF::TestZone")',
65-
'# Zone:schedule:macroTask:setInterval("<root>::ProxyZone::WTF::TestZone", ' + id + ')'
66-
]);
56+
expect(wtfMock.log[0]).toEqual('# Zone:fork("<root>::ProxyZone::WTF", "TestZone")');
57+
expect(wtfMock.log[1]).toEqual('> Zone:invoke:unit-test("<root>::ProxyZone::WTF::TestZone")');
58+
expect(wtfMock.log[2])
59+
.toContain('# Zone:schedule:macroTask:setInterval("<root>::ProxyZone::WTF::TestZone"');
6760
}, null, null, 'unit-test');
6861
});
6962

‎test/common/setTimeout.spec.ts‎

Lines changed: 16 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -14,18 +14,18 @@ describe('setTimeout', function() {
1414
let cancelId: any;
1515
const testZone = Zone.current.fork((Zone as any)['wtfZoneSpec']).fork({name: 'TestZone'});
1616
testZone.run(() => {
17-
let id: number;
1817
const timeoutFn = function() {
1918
expect(Zone.current.name).toEqual(('TestZone'));
2019
global[zoneSymbol('setTimeout')](function() {
21-
expect(wtfMock.log).toEqual([
22-
'# Zone:fork("<root>::ProxyZone::WTF", "TestZone")',
23-
'> Zone:invoke:unit-test("<root>::ProxyZone::WTF::TestZone")',
24-
'# Zone:schedule:macroTask:setTimeout("<root>::ProxyZone::WTF::TestZone", ' + id + ')',
25-
'< Zone:invoke:unit-test',
26-
'> Zone:invokeTask:setTimeout("<root>::ProxyZone::WTF::TestZone")',
27-
'< Zone:invokeTask:setTimeout'
28-
]);
20+
expect(wtfMock.log[0]).toEqual('# Zone:fork("<root>::ProxyZone::WTF", "TestZone")');
21+
expect(wtfMock.log[1])
22+
.toEqual('> Zone:invoke:unit-test("<root>::ProxyZone::WTF::TestZone")');
23+
expect(wtfMock.log[2])
24+
.toContain('# Zone:schedule:macroTask:setTimeout("<root>::ProxyZone::WTF::TestZone"');
25+
expect(wtfMock.log[3]).toEqual('< Zone:invoke:unit-test');
26+
expect(wtfMock.log[4])
27+
.toEqual('> Zone:invokeTask:setTimeout("<root>::ProxyZone::WTF::TestZone")');
28+
expect(wtfMock.log[5]).toEqual('< Zone:invokeTask:setTimeout');
2929
done();
3030
});
3131
};
@@ -35,18 +35,10 @@ describe('setTimeout', function() {
3535
expect(typeof cancelId.ref).toEqual(('function'));
3636
expect(typeof cancelId.unref).toEqual(('function'));
3737
}
38-
// This icky replacer is to deal with Timers in node.js. The data.handleId contains timers in
39-
// node.js. They do not stringify properly since they contain circular references.
40-
id = JSON.stringify((<MacroTask>cancelId).data, function replaceTimer(key, value) {
41-
if (key == 'handleId' && typeof value == 'object') return value.constructor.name;
42-
if (typeof value === 'function') return value.name;
43-
return value;
44-
}) as any as number;
45-
expect(wtfMock.log).toEqual([
46-
'# Zone:fork("<root>::ProxyZone::WTF", "TestZone")',
47-
'> Zone:invoke:unit-test("<root>::ProxyZone::WTF::TestZone")',
48-
'# Zone:schedule:macroTask:setTimeout("<root>::ProxyZone::WTF::TestZone", ' + id + ')'
49-
]);
38+
expect(wtfMock.log[0]).toEqual('# Zone:fork("<root>::ProxyZone::WTF", "TestZone")');
39+
expect(wtfMock.log[1]).toEqual('> Zone:invoke:unit-test("<root>::ProxyZone::WTF::TestZone")');
40+
expect(wtfMock.log[2])
41+
.toContain('# Zone:schedule:macroTask:setTimeout("<root>::ProxyZone::WTF::TestZone"');
5042
}, null, null, 'unit-test');
5143
});
5244

@@ -97,11 +89,11 @@ describe('setTimeout', function() {
9789
});
9890
});
9991

100-
it('should return the timeout Id through toString', function() {
92+
it('should return the original timeout Id', function() {
10193
// Node returns complex object from setTimeout, ignore this test.
10294
if (isNode) return;
10395
const cancelId = setTimeout(() => {}, 0);
104-
expect(typeof(cancelId.toString())).toBe('number');
96+
expect(typeof cancelId).toEqual('number');
10597
});
10698

10799
it('should allow cancelation by numeric timeout Id', function(done) {
@@ -114,12 +106,10 @@ describe('setTimeout', function() {
114106
const testZone = Zone.current.fork((Zone as any)['wtfZoneSpec']).fork({name: 'TestZone'});
115107
testZone.run(() => {
116108
const spy = jasmine.createSpy('spy');
117-
const task: Task = <any>setTimeout(spy, 0);
118-
const cancelId: number = <any>task;
109+
const cancelId = setTimeout(spy, 0);
119110
clearTimeout(cancelId);
120111
setTimeout(function() {
121112
expect(spy).not.toHaveBeenCalled();
122-
expect(task.runCount).toEqual(0);
123113
done();
124114
}, 1);
125115
});

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -193,7 +193,7 @@ describe('FakeAsyncTestZoneSpec', () => {
193193
it('should not run cancelled timer', () => {
194194
fakeAsyncTestZone.run(() => {
195195
let ran = false;
196-
let id = setTimeout(() => {
196+
let id: any = setTimeout(() => {
197197
ran = true;
198198
}, 10);
199199
clearTimeout(id);

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

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -24,14 +24,14 @@ describe('TaskTrackingZone', function() {
2424

2525
it('should track tasks', (done: Function) => {
2626
taskTrackingZone.run(() => {
27-
const microTask = taskTrackingZone.scheduleMicroTask('test1', () => {});
27+
taskTrackingZone.scheduleMicroTask('test1', () => {});
2828
expect(taskTrackingZoneSpec.microTasks.length).toBe(1);
2929
expect(taskTrackingZoneSpec.microTasks[0].source).toBe('test1');
3030

31-
const macroTask = setTimeout(() => {}) as any as Task;
31+
setTimeout(() => {});
3232
expect(taskTrackingZoneSpec.macroTasks.length).toBe(1);
3333
expect(taskTrackingZoneSpec.macroTasks[0].source).toBe('setTimeout');
34-
taskTrackingZone.cancelTask(macroTask);
34+
taskTrackingZone.cancelTask(taskTrackingZoneSpec.macroTasks[0]);
3535
expect(taskTrackingZoneSpec.macroTasks.length).toBe(0);
3636

3737
setTimeout(() => {
@@ -71,10 +71,10 @@ describe('TaskTrackingZone', function() {
7171

7272
it('should capture task creation stacktrace', (done) => {
7373
taskTrackingZone.run(() => {
74-
const task: any = setTimeout(() => {
74+
setTimeout(() => {
7575
done();
76-
}) as any as Task;
77-
expect(task['creationLocation']).toBeTruthy();
76+
});
77+
expect((taskTrackingZoneSpec.macroTasks[0] as any)['creationLocation']).toBeTruthy();
7878
});
7979
});
8080
});

0 commit comments

Comments
 (0)