-
-
Notifications
You must be signed in to change notification settings - Fork 35.4k
timers: enable timers to be used as primitives #19683
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 6 commits
9ac2bd4
6101d52
e32b969
c4ec0e5
d963047
54b8894
d7ae747
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -145,6 +145,7 @@ const kRefed = Symbol('refed'); | |
| const refedLists = Object.create(null); | ||
| const unrefedLists = Object.create(null); | ||
|
|
||
| const KNOWN_TIMERS = Object.create(null); | ||
|
|
||
| // Schedule or re-schedule a timer. | ||
| // The item must have been enroll()'d first. | ||
|
|
@@ -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) { | ||
|
|
@@ -496,7 +504,15 @@ exports.setInterval = function(callback, repeat, arg1, arg2, arg3) { | |
| return timeout; | ||
| }; | ||
|
|
||
| exports.clearInterval = function(timer) { | ||
| const clearInterval = exports.clearInterval = function(timer) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why the addition of
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This fails linting.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fixed |
||
| return; | ||
| } | ||
| } | ||
| if (timer && timer._repeat) { | ||
| timer._repeat = null; | ||
| clearTimeout(timer); | ||
|
|
@@ -521,6 +537,19 @@ function unrefdHandle(timer, now) { | |
| return true; | ||
| } | ||
|
|
||
| Timeout.prototype[Symbol.toPrimitive] = function() { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(); | ||
| } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fixed / it cannot be factored out since it closes over
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. since it is public and mutable, that is not certain.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
| } | ||
| return id; | ||
| }; | ||
|
|
||
| Timeout.prototype.unref = function() { | ||
| if (this._handle) { | ||
|
|
||
| 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); | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you check that |
||
| 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]); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extraneous line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed