Skip to content

process: move multipleResolves event to EOL #58707

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.

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Jun 14, 2025

The multipleResolves event has been deprecated for several years now. It's time.

@jasnell jasnell added semver-major PRs that contain breaking changes and should be released in the next major version. process Issues and PRs related to the process subsystem. deprecations Issues and PRs related to deprecations. labels Jun 14, 2025
@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Jun 14, 2025
Copy link

codecov bot commented Jun 14, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.09%. Comparing base (eafbe27) to head (bb5a924).
Report is 29 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #58707   +/-   ##
=======================================
  Coverage   90.09%   90.09%           
=======================================
  Files         640      640           
  Lines      188287   188225   -62     
  Branches    36917    36903   -14     
=======================================
- Hits       169628   169582   -46     
+ Misses      11361    11358    -3     
+ Partials     7298     7285   -13     
Files with missing lines Coverage Δ
lib/internal/process/promises.js 100.00% <100.00%> (ø)

... and 25 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Member

@LiviaMedeiros LiviaMedeiros left a comment

Choose a reason for hiding this comment

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

Not blocking but IMHO this event should get Legacy status rather than being removed. Or we can keep it forever runtime deprecated (similar to how DEP0174 works) and make the warning message more descriptive.
Despite the false positives with Promise.race and Promise.all, it still can be useful for locating problems where code unintentionally keeps flowing after calling resolve() or reject().

If keeping this causes noticeable runtime overhead even when there's no listener (judging from this PR, it's just one boolean check per fired event?), +0 solely for performance reasons.

Copy link
Contributor

@Ethan-Arrowood Ethan-Arrowood left a comment

Choose a reason for hiding this comment

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

LGTM

@jasnell jasnell requested a review from Ethan-Arrowood June 20, 2025 17:17
@jasnell
Copy link
Member Author

jasnell commented Jun 20, 2025

@nodejs/tsc

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

@jasnell jasnell 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. labels Jun 21, 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 Jun 21, 2025
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/58707
✔  Done loading data for nodejs/node/pull/58707
----------------------------------- PR info ------------------------------------
Title      process: move multipleResolves event to EOL (#58707)
Author     James M Snell <[email protected]> (@jasnell)
Branch     jasnell:jasnell/multipleresolves-eol -> nodejs:main
Labels     semver-major, process, deprecations, needs-ci, commit-queue-squash
Commits    2
 - process: move multipleResolves event to EOL
 - Apply suggestions from code review
Committers 2
 - James M Snell <[email protected]>
 - GitHub <[email protected]>
PR-URL: https://github.com/nodejs/node/pull/58707
Reviewed-By: Ethan Arrowood <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/58707
Reviewed-By: Ethan Arrowood <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
--------------------------------------------------------------------------------
   ℹ  This PR was created on Sat, 14 Jun 2025 21:50:54 GMT
   ✔  Approvals: 3
   ✔  - Ethan Arrowood (@Ethan-Arrowood): https://github.com/nodejs/node/pull/58707#pullrequestreview-2946874060
   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/58707#pullrequestreview-2947696086
   ✔  - Rafael Gonzaga (@RafaelGSS) (TSC): https://github.com/nodejs/node/pull/58707#pullrequestreview-2947784147
   ✘  Last GitHub CI failed
   ✘  No Jenkins CI runs detected
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/15798026652

@nodejs-github-bot

This comment was marked as outdated.

The `multipleResolves` event has been deprecated for several
years now. It's time.
@jasnell jasnell force-pushed the jasnell/multipleresolves-eol branch from 9561a09 to bb5a924 Compare June 21, 2025 17:57
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot
Copy link
Collaborator

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
commit-queue-failed An error occurred while landing this 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. deprecations Issues and PRs related to deprecations. needs-ci PRs that need a full CI run. process Issues and PRs related to the process subsystem. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants