timers: runtime-deprecate Timeout.prototype[Symbol.dispose]#64615
timers: runtime-deprecate Timeout.prototype[Symbol.dispose]#64615mcollina wants to merge 2 commits into
Conversation
|
Review requested:
|
f139ee5 to
669757d
Compare
Emit a DEP0208 deprecation warning when Timeout.prototype[Symbol.dispose] is called. The web platform setTimeout()/setInterval() APIs return a number, which cannot implement Symbol.dispose. Prefer clearTimeout() instead. Immediate.prototype[Symbol.dispose] is left unchanged. Fixes: nodejs#58689 Signed-off-by: Matteo Collina <hello@matteocollina.com>
669757d to
c6558b0
Compare
| [`setTimeout()`][] and [`setInterval()`][] APIs return a number, which cannot | ||
| implement `Symbol.dispose`. Prefer [`clearTimeout()`][] instead to cancel a | ||
| timeout. This deprecation does not apply to [`Immediate`][] objects returned by | ||
| [`setImmediate()`][]. |
There was a problem hiding this comment.
since this deprecation have clear before after change could you add code snippets to that.
Add code examples showing migration from Timeout.prototype[Symbol.dispose] to clearTimeout(). Refs: nodejs#64615 Signed-off-by: Matteo Collina <hello@matteocollina.com>
|
Added before/after code snippets to DEP0208 as requested. |
Web or not, I'd add a +1 for deprecating this as well for consistency, are there any real uses for disposing of an Immediate in the same block as it was created? https://github.com/search?q=language%3AJavaScript+%2Fusing+%5CS%2B+%3D+setImmediate%2F&type=code |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #64615 +/- ##
==========================================
- Coverage 90.24% 90.14% -0.11%
==========================================
Files 739 741 +2
Lines 241772 242077 +305
Branches 45561 45541 -20
==========================================
+ Hits 218194 218211 +17
- Misses 15111 15362 +251
- Partials 8467 8504 +37
🚀 New features to boost your workflow:
|
AugustinMauroy
left a comment
There was a problem hiding this comment.
in term of docs it's super nice.
and in terms of the deprecation LGTM
Emit a DEP0208 runtime deprecation warning when
Timeout.prototype[Symbol.dispose]is called.The web platform
setTimeout()/setInterval()APIs return a number,which cannot implement
Symbol.dispose. Leaving this integration in placeencourages code that is incompatible between Node.js and browsers (and
other server-side platforms that follow the web timers API).
Immediate.prototype[Symbol.dispose]is intentionally left unchanged,since
setImmediateis not a web platform API.Prefer
clearTimeout()to cancel a timeout.Fixes: #58689