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: support Symbol.dispose #48633

Merged
merged 1 commit into from
Jul 5, 2023
Merged

timers: support Symbol.dispose #48633

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

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

@nodejs-github-bot
Copy link
Collaborator

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

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 <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
@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 <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
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 <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
@ruyadorno
Copy link
Member

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

@ruyadorno ruyadorno added the backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. label Sep 11, 2023
@MoLow
Copy link
Member Author

MoLow commented Sep 11, 2023

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

@MoLow MoLow removed the backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. label Sep 11, 2023
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 <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
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 <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
@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 <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@himself65 himself65 himself65 approved these changes

@ronag ronag ronag approved these changes

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