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.

Sign up for GitHub

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

Jump to bottom

timers: deprecate active() and _unrefActive() #26760

Closed
wants to merge 1 commit into from
Closed

timers: deprecate active() and _unrefActive() #26760

wants to merge 1 commit into from

Conversation

Fishrock123
Copy link
Contributor

@Fishrock123 Fishrock123 commented Mar 18, 2019 •
edited
Loading

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

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

@nodejs-github-bot
Copy link
Collaborator

@Fishrock123 build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/2976/pipeline

@nodejs-github-bot nodejs-github-bot added the timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. label Mar 18, 2019
@Fishrock123 Fishrock123 force-pushed the timers-active-deprecation branch from 67a522f to 41d48d7 Compare March 18, 2019 23:05
@Fishrock123 Fishrock123 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Mar 18, 2019
mscdex
mscdex reviewed Mar 18, 2019
View reviewed changes
doc/api/deprecations.md Outdated Show resolved Hide resolved
mscdex
mscdex reviewed Mar 18, 2019
View reviewed changes
doc/api/deprecations.md
Type: Runtime

The previously undocumented and "private" `timers.active()` is deprecated.
Please use the publicly documented [`timeout.refresh()`][] instead.
Copy link
Contributor

@mscdex mscdex Mar 18, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Does timers._unrefActive() really do the same thing as timeout.refresh()? Shouldn't we only be recommending timeout.unref() as a replacement?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's an awkward combination of both. It is essentially "refresh this timer as unrefed". Active is the same, but refed.

Perhaps there was an extra meaning back when {un}ref() made separate libuv handles. I'm not sure but that doesn't exist anymore regardless.

@Fishrock123 Fishrock123 force-pushed the timers-active-deprecation branch from 41d48d7 to 696a869 Compare March 19, 2019 00:06
vsemozhetbyt
vsemozhetbyt reviewed Mar 19, 2019
View reviewed changes
doc/api/deprecations.md
@@ -2368,6 +2368,38 @@ Type: Runtime

The `_stream_wrap` module is deprecated.

<a id="DEP0126"></a>
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be DEP0XXX till landing (here, below and in the next section)?

Copy link
Contributor Author

@Fishrock123 Fishrock123 Mar 19, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Does it really matter? ¯\_(ツ)_/¯

So long as it's updated if necessary at landing...

doc/api/deprecations.md Outdated Show resolved Hide resolved
doc/api/deprecations.md Outdated Show resolved Hide resolved
@Fishrock123
timers: deprecate active() and _unrefActive()
1b80a2b 
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 Fishrock123 force-pushed the timers-active-deprecation branch from 696a869 to 1b80a2b Compare March 19, 2019 18:14
@richardlau richardlau added the deprecations Issues and PRs related to deprecations. label Mar 19, 2019
@Fishrock123 Fishrock123 added the semver-minor PRs that contain new features and should be released in the next minor version. label Mar 20, 2019
@Fishrock123
Copy link
Contributor Author

@nodejs/tsc I would like approval to land this as semver-minor, just like the related former pr: #20298, considering it deprecates undocumented methods.

mcollina
mcollina approved these changes Mar 21, 2019
View reviewed changes
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

apapirovski
apapirovski approved these changes Mar 21, 2019
View reviewed changes
Copy link
Member

@apapirovski apapirovski left a comment •
edited
Loading

Choose a reason for hiding this comment

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

In favor of semver-minor.

@BridgeAR
Copy link
Member

CI https://ci.nodejs.org/job/node-test-pull-request/21746/

@Fishrock123 just as a note: this was not author-ready until now (our guidelines say the CI has to be triggered and AFAIK the lite CI does not count in this case).

This was referenced Mar 22, 2019
Closed
Closed
@Fishrock123
Copy link
Contributor Author

@BridgeAR Oh, whoops. Will remember for next time. This LG to you now? CI is happy.

@BridgeAR
Copy link
Member

@Fishrock123 yes, this is still LGTM.

Fishrock123 added a commit to Fishrock123/node that referenced this pull request Mar 23, 2019
@Fishrock123
timers: deprecate active() and _unrefActive()
7c80f18 
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 <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
@Fishrock123
Copy link
Contributor Author

Landed in 7c80f18

@Fishrock123 Fishrock123 deleted the timers-active-deprecation branch March 23, 2019 02:59
targos pushed a commit that referenced this pull request Mar 28, 2019
@Fishrock123 @targos
timers: deprecate active() and _unrefActive()
fb83505 
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 <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
targos pushed a commit that referenced this pull request Mar 30, 2019
@Fishrock123 @targos
timers: deprecate active() and _unrefActive()
b24e45a 
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 <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
@BethGriggs BethGriggs mentioned this pull request Apr 9, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vsemozhetbyt vsemozhetbyt vsemozhetbyt left review comments

@mscdex mscdex mscdex left review comments

@BridgeAR BridgeAR BridgeAR approved these changes

@joyeecheung joyeecheung joyeecheung approved these changes

@mcollina mcollina mcollina approved these changes

@apapirovski apapirovski apapirovski approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. deprecations Issues and PRs related to deprecations. semver-minor PRs that contain new features and should be released in the next minor version. timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@Fishrock123 @nodejs-github-bot @BridgeAR @apapirovski @mcollina @mscdex @joyeecheung @vsemozhetbyt @richardlau @BethGriggs