Skip to content

timers: support Symbol.dispose#48633

Merged
nodejs-github-bot merged 1 commit intonodejs:mainfrom
MoLow:timers-destroy
Jul 5, 2023
Merged

timers: support Symbol.dispose#48633
nodejs-github-bot merged 1 commit intonodejs:mainfrom
MoLow:timers-destroy

Conversation

@MoLow
Copy link
Member

@MoLow MoLow commented Jul 2, 2023 •
edited
Loading

this does not change the behavior of node:timers/promises.

@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 Jul 2, 2023
@MoLow MoLow requested review from benjamingr and ronag July 2, 2023 08:02
@aduh95
Copy link
Contributor

aduh95 commented Jul 2, 2023 •
edited
Loading

Is the web going to follow this? I assume they will, but if it hasn't been spec'd yet, it seems more prudent to hold off until than happens.
EDIT: well obviously the web is returning a number, not an object, so it seems highly unlikely that they will ever do that. So I guess the question is wether we want to introduce this knowing that it's going to make code diverge between Node.js and other runtimes.

@MoLow
Copy link
Member Author

MoLow commented Jul 2, 2023

So I guess the question is whether we want to introduce this knowing that it's going to make code diverge between Node.js and other runtimes.

I am ok with adding it since it is experimental and using keyword is opt-in, but this is a very good point.
also we already expose unref/ref etc. so it wont be the first difference.

@MoLow MoLow added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 5, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 5, 2023
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jul 5, 2023

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jul 5, 2023

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

@MoLow MoLow added the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 5, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 5, 2023
@nodejs-github-bot nodejs-github-bot merged commit 56b8de1 into nodejs:main Jul 5, 2023
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jul 5, 2023

Landed in 56b8de1

@MoLow MoLow deleted the timers-destroy branch July 5, 2023 13:49
This was referenced Jul 6, 2023
Open
Open
Open
Open
Open
juanarbol pushed a commit that referenced this pull request Jul 13, 2023
@MoLow @juanarbol
timers: support Symbol.dispose
e2442bb 
PR-URL: #48633
Reviewed-By: Zeyu "Alex" Yang <himself65@outlook.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
@juanarbol juanarbol mentioned this pull request Jul 13, 2023
Merged
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@MoLow @Ceres6
timers: support Symbol.dispose
7b2121d 
PR-URL: nodejs#48633
Reviewed-By: Zeyu "Alex" Yang <himself65@outlook.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@MoLow @Ceres6
timers: support Symbol.dispose
3d76ff8 
PR-URL: nodejs#48633
Reviewed-By: Zeyu "Alex" Yang <himself65@outlook.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
@ruyadorno
Copy link
Member

ruyadorno commented Sep 11, 2023

This commit does not land cleanly on v18.x-staging and will need manual backport in case we want it in v18.

@MoLow
Copy link
Member Author

MoLow commented Sep 11, 2023

@ruyadorno now that #49598 landed on v18.x-staging this seems to land cleanly

ruyadorno pushed a commit that referenced this pull request Sep 12, 2023
@MoLow @ruyadorno
timers: support Symbol.dispose
0a12055 
PR-URL: #48633
Reviewed-By: Zeyu "Alex" Yang <himself65@outlook.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
ruyadorno pushed a commit that referenced this pull request Sep 13, 2023
@MoLow @ruyadorno
timers: support Symbol.dispose
959b0c2 
PR-URL: #48633
Reviewed-By: Zeyu "Alex" Yang <himself65@outlook.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
@ruyadorno ruyadorno mentioned this pull request Sep 13, 2023
Merged
ruyadorno pushed a commit that referenced this pull request Sep 17, 2023
@MoLow @ruyadorno
timers: support Symbol.dispose
7822a54 
PR-URL: #48633
Reviewed-By: Zeyu "Alex" Yang <himself65@outlook.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
@skyclouds2001 skyclouds2001 mentioned this pull request Apr 5, 2025
Closed
@lucacasonato lucacasonato mentioned this pull request Jun 12, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@benjamingr benjamingr benjamingr approved these changes

@ronag ronag ronag approved these changes

@himself65 himself65 himself65 approved these changes

Assignees

No one assigned

Labels

needs-ci PRs that need a full CI run. 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

@MoLow @aduh95 @nodejs-github-bot @ruyadorno @benjamingr @ronag @himself65