Repository navigation
[DEP0096] DeprecationWarning: timers.unenroll() question #20261
Description
Activity
/cc @Fishrock123
It will work, yes, but you should probably move to using
setTimeout. (The old API causes more issues than it solves perf-wise.)Also,
Timers.activedoesn't really initialize everything properly. (That's whatenroll()was for.)Timers.active()is not yet deprecated as it is useful to refresh existing timeouts (even those made bysetTimeoutin-place without allocating new resources.In the future I want to expose this internal function which does that in a better way but I wasn't sure what the end API should be: https://github.com/nodejs/node/blob/master/lib/internal/timers.js#L85-L96
I could do that sooner I suppose, as the API seems fine to me. I guess browsers might also be interested in this so maybe in the distant future
refreshTimeout()would be better, but I am not going to bother with trying to do web standards stuff.Maybe the deprecation should also mention that if you are using
Timers.activein that way you should probably move away from it...?Maybe. I'm fine with moving, just not sure how to make the move. This came out from the "mysql" module. Is there like a guide I can read to understand the changes I need to make?
I'm just basically enrolling an object to a timer and I extend the timeout every time a data chunk arrives so it is basically an inactivity timeout.
This older code:
class ThingWithTimer { constructor () { Timers.enroll(this) } start () { Timers.active(this) } refresh () { Timers.active(this) } done () { Timers.unenroll(this) } }
Becomes something like:
class ThingWithTimer { constructor () { this.timer = null } start () { this.timer = setTimeout() } refresh () { // Refresh timeout in-place Timers.active(this.timer) // Eventually, this.timer.refresh() or refreshTimeout(this.timer) } done () { clearTimeout(this.timer) } }
Awesome, will try it out. Is there a way to feature detect when to use this new method vs the old method?
Also I saw in the code there is Timeout.prototype[refreshFnSymbol] already. Any reason not to bring it out from behind a symbol in like 10.1? Seems odd to mix the two APIs temporarily. It would also be an easy way to feature detect from my question above: see if there is a refresh method for timers, right?
Is there a way to feature detect when to use this new method vs the old method?
Always. I think you need to be on like Node 0.8 for the full API to be realistically worse for perf. The APIs in the example exist and function correctly on all versions of Node.
Any reason not to bring it out from behind a symbol in like 10.1?
From the above, I wasn't sure if something like
refreshTimeout(timer)would be a better API. Browsers could make use of something like that (although as stated I don't really want a part in that).Such an approach could also work better with #19683. (Although something like a
getTimerFromIdwould also work.)Ehhh, I'll open a PR to make it public. :)
Ok, PR up at #20298
- added a commit that references this issue
on May 8, 2018 - added a commit that references this issue
on May 10, 2018 - added a commit that references this issue
on May 12, 2018 #20298 landed and was released in 11.0.0.
@dougwilson Can this issue be closed? Or is there more to be done here?
Yes, all resolved, thank you.
Just update the mysql library to latest version
npm install mysql@2.16.0 --saveReacted by Jovidon and AnonymousWebHacker
I have a question on this deprecation. I am using Timers.active to start a passive timer on a socket and Timers.unenroll to stop it. I see it says I need to change unenroll to clearTimeout but nothing about Timers.active. Will clearTimeout unenroll the timer created from Timers.active ?