Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

timers: support Symbol.dispose #48633

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
Merged
nodejs-github-bot merged 1 commit into from
Jul 5, 2023
Merged

timers: support Symbol.dispose #48633

nodejs-github-bot merged 1 commit into from
Jul 5, 2023

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
Morty Proxy This is a proxified and sanitized view of the page, visit original site.