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: set several methods EOL #56966

Merged
merged 6 commits into from
Feb 10, 2025
Merged

timers: set several methods EOL #56966

merged 6 commits into from
Feb 10, 2025

Conversation

anonrig
Copy link
Member

@anonrig anonrig commented Feb 8, 2025 •
edited
Loading

Make _unrefActive, active, unenroll and enroll EOL. They have been runtime deprecated since v10 and v11, and not even mentioned on Node.js documentation page

cc @nodejs/tsc for visibility

Thanks to @RafaelGSS, here's the CITGM run: https://ci.nodejs.org/job/citgm-smoker/3547/

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. labels Feb 8, 2025
@anonrig anonrig force-pushed the yagiz/eol-timers-fns branch from 12fe2b9 to 3be2f72 Compare February 8, 2025 19:55
@anonrig anonrig added the semver-major PRs that contain breaking changes and should be released in the next major version. label Feb 8, 2025
@anonrig anonrig force-pushed the yagiz/eol-timers-fns branch 2 times, most recently from 4d86d75 to b565091 Compare February 8, 2025 20:06
@anonrig anonrig added the needs-citgm PRs that need a CITGM CI run. label Feb 8, 2025
@anonrig anonrig force-pushed the yagiz/eol-timers-fns branch from b565091 to e359ae6 Compare February 8, 2025 20:09
@nodejs-github-bot
Copy link
Collaborator

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

@codecov Codecov
Copy link

codecov bot commented Feb 8, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.14%. Comparing base (b181535) to head (f94223f).
Report is 19 commits behind head on main.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main   #56966   +/-   ##
=======================================
  Coverage   89.13%   89.14%           
=======================================
  Files         665      665           
  Lines      193165   193144   -21     
  Branches    37191    37187    -4     
=======================================
- Hits       172181   172174    -7     
+ Misses      13729    13726    -3     
+ Partials     7255     7244   -11
Files with missing lines Coverage Δ
lib/timers.js 100.00% <ø> (ø)

... and 37 files with indirect coverage changes

@RafaelGSS RafaelGSS added the deprecations Issues and PRs related to deprecations. label Feb 8, 2025
@anonrig
Copy link
Member Author

anonrig commented Feb 8, 2025

@RafaelGSS would you mind running the citgm for this pull-request? I am not sure what to write to git ref label on jenkins ui for this branch (since this is a branch in a fork)

@RafaelGSS
Copy link
Member

https://ci.nodejs.org/job/citgm-smoker/3547/

@anonrig anonrig added commit-queue Add this label to land a pull request using GitHub Actions. and removed needs-citgm PRs that need a CITGM CI run. labels Feb 8, 2025
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
atlowChemi
atlowChemi approved these changes Feb 9, 2025
View reviewed changes
test/parallel/test-timers-unenroll-unref-interval.js Outdated
Copy link
Member

Choose a reason for hiding this comment

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

the file name seems wrong now?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it's ok since unenroll is the function that these functions are calling. They're un-enrolling the timers.

@nodejs-github-bot
Copy link
Collaborator

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

marco-ippolito
marco-ippolito requested changes Feb 9, 2025
View reviewed changes
Copy link
Member

@marco-ippolito marco-ippolito 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.

Blocking because the description and the deprecation type needs to be updated (I'm in favor of this change)

@marco-ippolito marco-ippolito removed the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 9, 2025
@anonrig
Copy link
Member Author

anonrig commented Feb 9, 2025

@marco-ippolito Appreciate a re-review if you don't mind.

anonrig and others added 4 commits February 9, 2025 14:34
@anonrig @marco-ippolito
Update doc/api/deprecations.md
d4be015 
Co-authored-by: Marco Ippolito <[email protected]>
@anonrig @marco-ippolito
Update doc/api/deprecations.md
455b2ef 
Co-authored-by: Marco Ippolito <[email protected]>
@anonrig @marco-ippolito
Update doc/api/deprecations.md
8559006 
Co-authored-by: Marco Ippolito <[email protected]>
@anonrig @marco-ippolito
Update doc/api/deprecations.md
f94223f 
Co-authored-by: Marco Ippolito <[email protected]>
@anonrig anonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 9, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 9, 2025
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Feb 9, 2025 •
edited by jasnell
Loading

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

@anonrig anonrig added the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 10, 2025
@nodejs-github-bot nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Feb 10, 2025
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/56966
✔  Done loading data for nodejs/node/pull/56966
----------------------------------- PR info ------------------------------------
Title      timers: set several methods EOL (#56966)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     anonrig:yagiz/eol-timers-fns -> nodejs:main
Labels     timers, semver-major, deprecations, needs-ci
Commits    6
 - timers: set several methods EOL
 - fixup! timers: set several methods EOL
 - Update doc/api/deprecations.md
 - Update doc/api/deprecations.md
 - Update doc/api/deprecations.md
 - Update doc/api/deprecations.md
Committers 2
 - Yagiz Nizipli <[email protected]>
 - GitHub <[email protected]>
PR-URL: https://github.com/nodejs/node/pull/56966
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Chemi Atlow <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/56966
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Chemi Atlow <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
--------------------------------------------------------------------------------
   ℹ  This PR was created on Sat, 08 Feb 2025 19:54:12 GMT
   ✔  Approvals: 5
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/56966#pullrequestreview-2604018082
   ✔  - Michaël Zasso (@targos) (TSC): https://github.com/nodejs/node/pull/56966#pullrequestreview-2604053067
   ✔  - Rafael Gonzaga (@RafaelGSS) (TSC): https://github.com/nodejs/node/pull/56966#pullrequestreview-2604066665
   ✔  - Chemi Atlow (@atlowChemi): https://github.com/nodejs/node/pull/56966#pullrequestreview-2604321043
   ✔  - Marco Ippolito (@marco-ippolito) (TSC): https://github.com/nodejs/node/pull/56966#pullrequestreview-2604442341
   ✔  Last GitHub CI successful
   ℹ  Last CITGM CI on 2025-02-08T23:18:43Z: https://ci.nodejs.org/job/citgm-smoker/3547/
   ℹ  Last Full PR CI on 2025-02-09T19:39:54Z: https://ci.nodejs.org/job/node-test-pull-request/65097/
- Querying data for job/node-test-pull-request/65097/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  No git cherry-pick in progress
   ✔  No git am in progress
   ✔  No git rebase in progress
--------------------------------------------------------------------------------
- Bringing origin/main up to date...
From https://github.com/nodejs/node
 * branch                  main       -> FETCH_HEAD
✔  origin/main is now up-to-date
- Downloading patch for 56966
From https://github.com/nodejs/node
 * branch                  refs/pull/56966/merge -> FETCH_HEAD
✔  Fetched commits as 3ea97d5c2513..f94223f1e5a1
--------------------------------------------------------------------------------
[main 2ebbf340e3] timers: set several methods EOL
 Author: Yagiz Nizipli <[email protected]>
 Date: Sat Feb 8 14:52:40 2025 -0500
 10 files changed, 14 insertions(+), 254 deletions(-)
 delete mode 100644 test/parallel/test-timers-active.js
 delete mode 100644 test/parallel/test-timers-enroll-invalid-msecs.js
 delete mode 100644 test/parallel/test-timers-enroll-second-time.js
 delete mode 100644 test/parallel/test-timers-unref-active.js
 delete mode 100644 test/parallel/test-timers-unref-remove-other-unref-timers-only-one-fires.js
 delete mode 100644 test/parallel/test-timers-unref-remove-other-unref-timers.js
[main 75817d3c9b] fixup! timers: set several methods EOL
 Author: Yagiz Nizipli <[email protected]>
 Date: Sun Feb 9 14:16:36 2025 -0500
 1 file changed, 4 insertions(+), 4 deletions(-)
[main 55ae74a2db] Update doc/api/deprecations.md
 Author: Yagiz Nizipli <[email protected]>
 Date: Sun Feb 9 14:34:20 2025 -0500
 1 file changed, 1 insertion(+), 1 deletion(-)
[main 419f84c3c3] Update doc/api/deprecations.md
 Author: Yagiz Nizipli <[email protected]>
 Date: Sun Feb 9 14:34:28 2025 -0500
 1 file changed, 1 insertion(+), 1 deletion(-)
[main 905f8b5875] Update doc/api/deprecations.md
 Author: Yagiz Nizipli <[email protected]>
 Date: Sun Feb 9 14:34:36 2025 -0500
 1 file changed, 1 insertion(+), 1 deletion(-)
[main f91c2cffb6] Update doc/api/deprecations.md
 Author: Yagiz Nizipli <[email protected]>
 Date: Sun Feb 9 14:34:43 2025 -0500
 1 file changed, 1 insertion(+), 1 deletion(-)
   ✔  Patches applied
There are 6 commits in the PR. Attempting autorebase.
Rebasing (2/11)
Rebasing (3/11)
Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
timers: set several methods EOL

PR-URL: #56966

Reviewed-By: James M Snell <[email protected]>

Reviewed-By: Michaël Zasso <[email protected]>

Reviewed-By: Rafael Gonzaga <[email protected]>

Reviewed-By: Chemi Atlow <[email protected]>

Reviewed-By: Marco Ippolito <[email protected]>


[detached HEAD dd76d08c37] timers: set several methods EOL

Author: Yagiz Nizipli <[email protected]>

Date: Sat Feb 8 14:52:40 2025 -0500

10 files changed, 18 insertions(+), 258 deletions(-)

delete mode 100644 test/parallel/test-timers-active.js

delete mode 100644 test/parallel/test-timers-enroll-invalid-msecs.js

delete mode 100644 test/parallel/test-timers-enroll-second-time.js

delete mode 100644 test/parallel/test-timers-unref-active.js

delete mode 100644 test/parallel/test-timers-unref-remove-other-unref-timers-only-one-fires.js

delete mode 100644 test/parallel/test-timers-unref-remove-other-unref-timers.js

Rebasing (4/11)

Rebasing (5/11)

Executing: git node land --amend --yes

--------------------------------- New Message ----------------------------------

Update doc/api/deprecations.md


Co-authored-by: Marco Ippolito <[email protected]>

PR-URL: #56966

Reviewed-By: James M Snell <[email protected]>

Reviewed-By: Michaël Zasso <[email protected]>

Reviewed-By: Rafael Gonzaga <[email protected]>

Reviewed-By: Chemi Atlow <[email protected]>

Reviewed-By: Marco Ippolito <[email protected]>


[detached HEAD 8695ca90da] Update doc/api/deprecations.md

Author: Yagiz Nizipli <[email protected]>

Date: Sun Feb 9 14:34:20 2025 -0500

1 file changed, 1 insertion(+), 1 deletion(-)

Rebasing (6/11)

Rebasing (7/11)

Executing: git node land --amend --yes

--------------------------------- New Message ----------------------------------

Update doc/api/deprecations.md


Co-authored-by: Marco Ippolito <[email protected]>

PR-URL: #56966

Reviewed-By: James M Snell <[email protected]>

Reviewed-By: Michaël Zasso <[email protected]>

Reviewed-By: Rafael Gonzaga <[email protected]>

Reviewed-By: Chemi Atlow <[email protected]>

Reviewed-By: Marco Ippolito <[email protected]>


[detached HEAD 425e3e75d3] Update doc/api/deprecations.md

Author: Yagiz Nizipli <[email protected]>

Date: Sun Feb 9 14:34:28 2025 -0500

1 file changed, 1 insertion(+), 1 deletion(-)

Rebasing (8/11)

Rebasing (9/11)

Executing: git node land --amend --yes

--------------------------------- New Message ----------------------------------

Update doc/api/deprecations.md


Co-authored-by: Marco Ippolito <[email protected]>

PR-URL: #56966

Reviewed-By: James M Snell <[email protected]>

Reviewed-By: Michaël Zasso <[email protected]>

Reviewed-By: Rafael Gonzaga <[email protected]>

Reviewed-By: Chemi Atlow <[email protected]>

Reviewed-By: Marco Ippolito <[email protected]>


[detached HEAD dd17d296f4] Update doc/api/deprecations.md

Author: Yagiz Nizipli <[email protected]>

Date: Sun Feb 9 14:34:36 2025 -0500

1 file changed, 1 insertion(+), 1 deletion(-)

Rebasing (10/11)

Rebasing (11/11)

Executing: git node land --amend --yes

--------------------------------- New Message ----------------------------------

Update doc/api/deprecations.md


Co-authored-by: Marco Ippolito <[email protected]>

PR-URL: #56966

Reviewed-By: James M Snell <[email protected]>

Reviewed-By: Michaël Zasso <[email protected]>

Reviewed-By: Rafael Gonzaga <[email protected]>

Reviewed-By: Chemi Atlow <[email protected]>

Reviewed-By: Marco Ippolito <[email protected]>


[detached HEAD 935381ce72] Update doc/api/deprecations.md

Author: Yagiz Nizipli <[email protected]>

Date: Sun Feb 9 14:34:43 2025 -0500

1 file changed, 1 insertion(+), 1 deletion(-)

Successfully rebased and updated refs/heads/main.


ℹ  Add commit-queue-squash label to land the PR as one commit, or commit-queue-rebase to land as separate commits.
https://github.com/nodejs/node/actions/runs/13249312889

@anonrig anonrig added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Feb 10, 2025
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 10, 2025
@nodejs-github-bot nodejs-github-bot merged commit 5d7091f into nodejs:main Feb 10, 2025
69 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 5d7091f

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasnell jasnell jasnell approved these changes

@targos targos targos approved these changes

@RafaelGSS RafaelGSS RafaelGSS approved these changes

@marco-ippolito marco-ippolito marco-ippolito approved these changes

@atlowChemi atlowChemi atlowChemi approved these changes

Assignees
No one assigned
Labels
commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. deprecations Issues and PRs related to deprecations. needs-ci PRs that need a full CI run. 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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@anonrig @nodejs-github-bot @RafaelGSS @jasnell @targos @marco-ippolito @atlowChemi