lib: refactor project to use Promise.withResolvers

[anonrig]

Node.js v22+ supports Promise.withResolvers(). Let's use them!

[nodejs-github-bot]

Review requested:

• @nodejs/streams
• @nodejs/test_runner

[benjamingr]

After all these years - this is still a bad API but we might as well use the official one :D

[nodejs-github-bot]

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

[atlowChemi]

@anonrig could this have any impact on pref?
Should any benchmarks be run?

[anonrig]

@anonrig could this have any impact on pref?

Should any benchmarks be run?

I think it will improve performance but I'm not sure. Once the tests pass, let's have a benchmark ci just to be sure.

[jasnell]

PromiseWithResolvers isn't in primordials yet, is it?

[anonrig]

PromiseWithResolvers isn't in primordials yet, is it?

It seems like it can be removed by a v8 flag: https://github.com/v8/v8/blob/3b12a031d2293acf01872e213473c8657671c8db/src/init/bootstrapper.cc#L5585

[aduh95]

We should probably wait for 20.x to be in maintenance so it does not create conflicts when backporting other PRs (we could also wait for V8 to drop the flag, but we don't officially support running Node.js with undocumented flags, so that's not a blocker)

[jasnell]

@anonrig ... I see you marked this author ready but the PR is still a draft. Going to remove the label for now until you confirm this is ready.

[jasnell]

We should probably wait for 20.x to be in maintenance so it does not create conflicts when backporting other PRs (we could also wait for V8 to drop the flag, but we don't officially support running Node.js with undocumented flags, so that's not a blocker)

Alternatively, if we choose to backport this, we could also choose to polyfill Promise.withResolvers() in the older release lines.

[aduh95]

We should probably wait for 20.x to be in maintenance so it does not create conflicts when backporting other PRs (we could also wait for V8 to drop the flag, but we don't officially support running Node.js with undocumented flags, so that's not a blocker)

Alternatively, if we choose to backport this, we could also choose to polyfill Promise.withResolvers() in the older release lines.

We've decided that any change to globally available APIs would be semver-major in #54330 (I mean it hasn't landed yet, but it already has your approval 😅), so backporting to v20.x is a no-go.

[ljharb]

If it’s a correct polyfill, why does it matter which it is?

[anonrig]

@aduh95 We already have polyfills in primordials

// Define Symbol.dispose and Symbol.asyncDispose
// Until these are defined by the environment.
// TODO(MoLow): Remove this polyfill once Symbol.dispose and Symbol.asyncDispose are available in V8.
primordials.SymbolDispose ??= primordials.SymbolFor('nodejs.dispose');
primordials.SymbolAsyncDispose ??= primordials.SymbolFor('nodejs.asyncDispose');

[codecov]

Codecov Report

Attention: Patch coverage is 96.72131% with 2 lines in your changes missing coverage. Please review.

Project coverage is 88.39%. Comparing base (7ae193d) to head (df1b7e5).
Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/internal/fs/watchers.js	66.66%	1 Missing ⚠️
lib/internal/webstreams/adapters.js	83.33%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #54836      +/-   ##
==========================================
- Coverage   88.40%   88.39%   -0.02%     
==========================================
  Files         653      653              
  Lines      187600   187585      -15     
  Branches    36117    36120       +3     
==========================================
- Hits       165854   165821      -33     
- Misses      14974    14981       +7     
- Partials     6772     6783      +11     

Files with missing lines	Coverage Δ
lib/child_process.js	97.72% <100.00%> (ø)
lib/internal/abort_controller.js	98.05% <100.00%> (ø)
lib/internal/blob.js	99.80% <100.00%> (ø)
lib/internal/streams/duplexify.js	96.56% <100.00%> (ø)
lib/internal/test_runner/harness.js	92.67% <100.00%> (ø)
lib/internal/test_runner/runner.js	88.92% <100.00%> (ø)
lib/internal/test_runner/test.js	96.98% <100.00%> (ø)
lib/internal/test_runner/utils.js	56.67% <100.00%> (ø)
lib/internal/util.js	97.06% <ø> (+0.06%)	⬆️
lib/internal/webstreams/readablestream.js	98.32% <100.00%> (ø)
... and 5 more

... and 22 files with indirect coverage changes

[aduh95]

We already have polyfills in primordials

And we shouldn't – especially the Symbol.[async]dispose are not actual polyfills. I can move them in another PR.

[benjamingr]

LGTM (but I agree with the moving of the polyfill)

[nodejs-github-bot]

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

[aduh95]

Blocked on #55115

[aduh95]

This needs a rebase.

[nodejs-github-bot]

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

[aduh95]

Landed in bb8cc65