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

fix: Support clearing the timeouts with numeric IDs #462

Closed
platosha wants to merge 2 commits into from
Closed

fix: Support clearing the timeouts with numeric IDs #462

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bb84c11
fix: support clearing the timeouts with numeric IDs
platosha Sep 20, 2016
3dd4f06
test(setTimeout): use explicit timeout delays where order matters
platosha Oct 7, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions lib/common/timers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,14 +19,21 @@ export function patchTimer(window: any, setName: string, cancelName: string, nam
setName += nameSuffix;
cancelName += nameSuffix;

const tasksByHandleId: Object = {};

function scheduleTask(task: Task) {
const data = <TimerOptions>task.data;
data.args[0] = task.invoke;
data.args[0] = function() {
task.invoke.apply(this, arguments);
Copy link
Copy Markdown
Contributor

@vicb vicb Oct 7, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be task here ?
or task.invoke(...arguments) ?

Copy link
Copy Markdown
Contributor Author

@platosha platosha Oct 12, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No.

Before the change, task.invoke was used as a timer callback. Timer callbacks are executed with some other context: this is a Timeout instance in Node.js, window in browers, undefined in strict mode.

Within this change, we use another function as a timer callback, but in the function we still call task.invoke with the original timer callback context. As it was called before the change.

Note, that the invoke method of ZoneTask, which is called here, also passes the context further, so it ends as the original user-provided task callback context. Changing it is not good. Consider:

setTimeout(function() {
  console.log(this);
  // should log `window` in browsers, not a `ZoneTask` instance or anything else
});

This doesn’t fail any Zone.js tests, though. Seems like there is a lack of tests for the timer callbacks context.

delete tasksByHandleId[data.handleId];
};
data.handleId = setNative.apply(window, data.args);
tasksByHandleId[data.handleId] = task;
return task;
}

function clearTask(task: Task) {
delete tasksByHandleId[(<TimerOptions>task.data).handleId];
return clearNative((<TimerOptions>task.data).handleId);
}

Expand Down Expand Up @@ -59,7 +66,7 @@ export function patchTimer(window: any, setName: string, cancelName: string, nam

clearNative =
patchMethod(window, cancelName, (delegate: Function) => function(self: any, args: any[]) {
var task: Task = args[0];
var task: Task = typeof args[0] === 'number' ? tasksByHandleId[args[0]] : args[0];
if (task && typeof task.type === 'string') {
if (task.cancelFn && task.data.isPeriodic || task.runCount === 0) {
// Do not cancel already canceled functions
Expand Down
25 changes: 23 additions & 2 deletions test/common/setTimeout.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ describe('setTimeout', function() {
setTimeout(function() {
expect(spy).not.toHaveBeenCalled();
done();
});
}, 1);
});
});

Expand All @@ -72,7 +72,7 @@ describe('setTimeout', function() {
clearTimeout(cancelId);
done();
});
});
}, 1);
});
});

Expand Down Expand Up @@ -102,6 +102,27 @@ describe('setTimeout', function() {
expect(typeof(cancelId.toString())).toBe('number');
});

it('should allow cancelation by numeric timeout Id', function (done) {
// Node returns complex object from setTimeout, ignore this test.
if (isNode) {
done();
return;
}

const testZone = Zone.current.fork(Zone['wtfZoneSpec']).fork({ name: 'TestZone' });
testZone.run(() => {
const spy = jasmine.createSpy('spy');
const task: Task = <any>setTimeout(spy, 0);
const cancelId: number = <any>task;
clearTimeout(0 + cancelId);
setTimeout(function () {
Copy link
Copy Markdown
Contributor

@vicb vicb Oct 6, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should you set a timeout of ie 10 here ?
I don't think there is any guarantee on the order for the same timeout value (even if most impl should work as expected)

Copy link
Copy Markdown
Contributor Author

@platosha platosha Oct 6, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right.

Done. Set the second timeout of 1.

Please note that the same point also applies to another test in this file as well, see https://github.com/platosha/zone.js/blob/02d0784bb9e9cfb44d2eaa32c12be538370b0a0b/test/common/setTimeout.spec.ts#L57

Copy link
Copy Markdown
Contributor

@vicb vicb Oct 6, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you please also modify the other occurence ?

Copy link
Copy Markdown
Contributor Author

@platosha platosha Oct 7, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, two places. Modified in a separate commit.

expect(spy).not.toHaveBeenCalled();
expect(task.runCount).toEqual(-1);
done();
}, 1);
});
});

it('should pass invalid values through', function() {
clearTimeout(null);
clearTimeout(<any>{});
Expand Down