Skip to content

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

Closed
Fishrock123 wants to merge 1 commit intonodejs:masterfrom
Fishrock123:timers-active-deprecation
Closed

timers: deprecate active() and _unrefActive()#26760
Fishrock123 wants to merge 1 commit intonodejs:masterfrom
Fishrock123:timers-active-deprecation

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

nodejs-github-bot commented Mar 18, 2019

@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
mscdex
mscdex reviewed Mar 18, 2019
View reviewed changes
doc/api/deprecations.md Outdated
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

@Fishrock123 Fishrock123 Mar 19, 2019

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 Outdated
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt Mar 19, 2019

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...

@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

Fishrock123 commented Mar 20, 2019

@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
Contributor

@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

BridgeAR commented Mar 21, 2019

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

Fishrock123 commented Mar 23, 2019

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

@BridgeAR
Copy link
Member

BridgeAR commented Mar 23, 2019

@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 <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>
@Fishrock123
Copy link
Contributor Author

Fishrock123 commented Mar 23, 2019

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 <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
@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 <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>
@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

@mcollina mcollina mcollina approved these changes

@joyeecheung joyeecheung joyeecheung approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

+3 more reviewers

@mscdex mscdex mscdex left review comments

@vsemozhetbyt vsemozhetbyt vsemozhetbyt left review comments

@apapirovski apapirovski apapirovski approved these changes

Reviewers whose approvals may not affect merge requirements

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