Skip to content
Closed
Show file tree
Hide file tree
Changes from 6 commits
Commits
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
9 changes: 9 additions & 0 deletions doc/api/timers.md
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,15 @@ of the Node.js application.

Returns a reference to the `Timeout`.

### timeout[Symbol.toPrimitive]()

* Returns: {integer}

When coercing a `Timeout` to a primitive, a primitive will be generated that
can be used with the appropriate function that can be used to clear the
`Timeout`. This allows enhanced compatibility with browser `setTimeout`, and
`setInterval` implementations.

## Scheduling Timers

A timer in Node.js is an internal construct that calls a given function after
Expand Down
31 changes: 30 additions & 1 deletion lib/timers.js
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,7 @@ const kRefed = Symbol('refed');
const refedLists = Object.create(null);
const unrefedLists = Object.create(null);

const KNOWN_TIMERS = Object.create(null);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Extraneous line.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

fixed

// Schedule or re-schedule a timer.
// The item must have been enroll()'d first.
Expand Down Expand Up @@ -453,6 +454,13 @@ function rearm(timer, start = TimerWrap.now()) {


const clearTimeout = exports.clearTimeout = function(timer) {
if (typeof timer === 'number' || typeof timer === 'string') {
if (timer in KNOWN_TIMERS) {
timer = KNOWN_TIMERS[timer];
} else {
return;
}
}
if (timer && timer._onTimeout) {
timer._onTimeout = null;
if (timer instanceof Timeout) {
Expand Down Expand Up @@ -496,7 +504,15 @@ exports.setInterval = function(callback, repeat, arg1, arg2, arg3) {
return timeout;
};

exports.clearInterval = function(timer) {
const clearInterval = exports.clearInterval = function(timer) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why the addition of clearInterval variable? Doesn't seem used anywhere.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

was added in e32b969 as a bug fix to previous iteration

if (typeof timer === 'number' || typeof timer === 'string') {
if (timer in KNOWN_TIMERS) {
timer = KNOWN_TIMERS[timer];
}
else {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This fails linting.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

fixed

return;
}
}
if (timer && timer._repeat) {
timer._repeat = null;
clearTimeout(timer);
Expand All @@ -521,6 +537,19 @@ function unrefdHandle(timer, now) {
return true;
}

Timeout.prototype[Symbol.toPrimitive] = function() {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why @@toPrimitive rather than valueOf?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

see https://codepen.io/bradleymeck/pen/eMMpdR?editors=0012 , .valueOf is not safe for using as a key. toPrimitive handles both string and number coercion.

const id = this[async_id_symbol];
if (id in KNOWN_TIMERS !== true) {
KNOWN_TIMERS[id] = this;
const $close = this.close;
this.close = function () {
delete KNOWN_TIMERS[this[async_id_symbol]];
this.close = $close;
this.close();
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This won't pass linting due to missing semicolon. Also, can the function be factored out?

Copy link
Copy Markdown
Member Author

@bmeck bmeck Mar 30, 2018

Choose a reason for hiding this comment

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

fixed / it cannot be factored out since it closes over $close.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

$close is effectively always equal to Timeout.prototype.close, right?

Copy link
Copy Markdown
Member Author

@bmeck bmeck Mar 30, 2018

Choose a reason for hiding this comment

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

since it is public and mutable, that is not certain.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@bmeck I think it’s fine to just ignore that. You could still run into it anyway, when .close is changed after the [@@toPrimitive]() call…

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

if they replace it they are probably wrapping it, I'm already ignoring all the ways to avoid getter/setters but don't feel comfortable with completely removing .close this seems like one of the metrics people could be profiling handle lifetimes.

}
return id;
};

Timeout.prototype.unref = function() {
if (this._handle) {
Expand Down
23 changes: 23 additions & 0 deletions test/parallel/test-timers-toPrimitive.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
'use strict';
const common = require('../common');
const assert = require('assert');

const timeout1 = setTimeout(common.mustNotCall(), 0);
const timeout2 = setInterval(common.mustNotCall(), 0);

assert.strictEqual(Number.isNaN(+timeout1), false);
assert.strictEqual(Number.isNaN(+timeout2), false);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you check that +timeout1 === timeout1[Symbol.toPrimitive]()?

assert.notStrictEqual(`${timeout1}`, Object.prototype.toString.call(timeout1));
assert.notStrictEqual(`${timeout2}`, Object.prototype.toString.call(timeout2));

assert.notStrictEqual(+timeout1, +timeout2);

const o = {};
o[timeout1] = timeout1;
o[timeout2] = timeout2;
const keys = Object.keys(o);
assert.deepStrictEqual(keys, [`${timeout1}`, `${timeout2}`]);

clearTimeout(keys[0]);
clearInterval(keys[1]);