Skip to content
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

timers: runtime-deprecate {un}enroll() #18066

Merged
merged 1 commit into from
Feb 2, 2018

Conversation

Fishrock123
Copy link
Contributor

This was never a Very Good API, and generally just left so many open ends for inconsistent behavior. The "optimization" benefit of this API is little to none. Makes a starting step towards removing it so that in the future, timers, especially in their async_hooks interactions, can be simplified.

For posterity: enroll() & unenroll() have been exposed since 0.1.very-early, predating setTimeout() & clearTimeout() in Node. They have never been documented.

@nodejs/tsc

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

timers

@Fishrock123 Fishrock123 added timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. semver-major PRs that contain breaking changes and should be released in the next major version. labels Jan 9, 2018
@nodejs-github-bot nodejs-github-bot added the timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. label Jan 9, 2018
@ChALkeR ChALkeR self-requested a review January 9, 2018 18:38
Copy link
Member

@ChALkeR ChALkeR left a comment

Choose a reason for hiding this comment

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

LGTM.

Those seem to have been used in net.js and/or _http_outgoing.js until #17704 and got to some number of npm modules from there (where whole lib files were copied), but that doesn't seem to be significant.

@jasnell jasnell requested a review from a team January 12, 2018 00:46

Type: Runtime

`timers.enroll()` has been deprecated in favor of using the publicly documented timers APIs, namely [`setTimeout()`][] and [`setInterval()`][].
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Can we make this more concise? Maybe this?:

timers.enroll() is deprecated. Please use [setTimeout()][] or [setInterval()][] instead.

(Similarly for the other deprecation message below.)

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM

lib/timers.js Outdated
}

exports.unenroll = function(item) {
process.emitWarning('timers.unenroll is deprecated. ' +
Copy link
Contributor

Choose a reason for hiding this comment

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

Will this not emit the warning every time it is called vs just the first time? Any reason to not use util.deprecate() for this?

Copy link
Member

Choose a reason for hiding this comment

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

good catch.

Copy link
Member

Choose a reason for hiding this comment

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

Also, message should probably include () after the function name: timers.unenroll() is deprecated.

@maclover7
Copy link
Contributor

ping @Fishrock123

@maclover7 maclover7 added the wip Issues and PRs that are still a work in progress. label Jan 20, 2018
@Fishrock123
Copy link
Contributor Author

@Fishrock123
Copy link
Contributor Author

what...

how do I run the markdown linter? :|

@Fishrock123
Copy link
Contributor Author

Ok, hope I fixed that. New CI: https://ci.nodejs.org/job/node-test-pull-request/12756/

@mcollina
Copy link
Member

@BridgeAR BridgeAR added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed wip Issues and PRs that are still a work in progress. labels Feb 2, 2018
This was never a Very Good API, and generally just left so many open
ends for inconsistent behavior. The "optimization" benefit of this API
is little to none. Makes a starting step towards removing it so that in
the future timers, especially in their async_hooks interactions, can be
simplified.

PR-URL: nodejs#18066
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@Fishrock123 Fishrock123 merged commit 68783ae into nodejs:master Feb 2, 2018
@Fishrock123
Copy link
Contributor Author

Thanks, landed in 68783ae

@Fishrock123 Fishrock123 deleted the deprecate-{un}enroll branch February 2, 2018 19:08
@Fishrock123 Fishrock123 added doc Issues and PRs related to the documentations. and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Feb 2, 2018
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
This was never a Very Good API, and generally just left so many open
ends for inconsistent behavior. The "optimization" benefit of this API
is little to none. Makes a starting step towards removing it so that in
the future timers, especially in their async_hooks interactions, can be
simplified.

PR-URL: nodejs#18066
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@tniessen tniessen added the deprecations Issues and PRs related to deprecations. label Sep 7, 2018
Fishrock123 added a commit to Fishrock123/node that referenced this pull request Mar 19, 2019
Another nail in the coffin here, farewell ye ol C-style apis.

These apis caused numerous other issues that required far too many
safeguards. This gets us one step closer to not having to worry about
those issues anymore.

Refs: nodejs#18066
Refs: nodejs#20298

PR-URL: nodejs#26760
Fishrock123 added a commit to Fishrock123/node that referenced this pull request Mar 23, 2019
Another nail in the coffin here, farewell ye ol C-style apis.

These apis caused numerous other issues that required far too many
safeguards. This gets us one step closer to not having to worry about
those issues anymore.

Refs: nodejs#18066
Refs: nodejs#20298

PR-URL: nodejs#26760

Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
targos pushed a commit that referenced this pull request Mar 28, 2019
Another nail in the coffin here, farewell ye ol C-style apis.

These apis caused numerous other issues that required far too many
safeguards. This gets us one step closer to not having to worry about
those issues anymore.

Refs: #18066
Refs: #20298

PR-URL: #26760

Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
targos pushed a commit that referenced this pull request Mar 30, 2019
Another nail in the coffin here, farewell ye ol C-style apis.

These apis caused numerous other issues that required far too many
safeguards. This gets us one step closer to not having to worry about
those issues anymore.

Refs: #18066
Refs: #20298

PR-URL: #26760

Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deprecations Issues and PRs related to deprecations. doc Issues and PRs related to the documentations. semver-major PRs that contain breaking changes and should be released in the next major version. timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout.
Projects
None yet
Development

Successfully merging this pull request may close these issues.