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

[nodejs-github-bot]

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

[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 https://github.com/nodejs/node/pull/54330 (I mean it hasn't landed yet, but it already has your approval 😅), so backporting to v20.x is a no-go.

[anonrig]

I added PromiseWithResolvers to primordials.js, I think we can land this now.

[anonrig]

@MoLow reviewed it faster than I requested a review. I have nothing but utter respect for you!

[ljharb]

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.

the primordial can be polyfilled tho without providing it on the global, which would allow backporting?

[anonrig]

the primordial can be polyfilled tho without providing it on the global, which would allow backporting?

I polyfilled it 👍

[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[bot]]

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 266 commits behind head on main.

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

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%)	:arrow_up:
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.

[nodejs-github-bot]

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

[aduh95]

Blocked on https://github.com/nodejs/node/pull/55115

[aduh95]

This needs a rebase.

[nodejs-github-bot]

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

[aduh95]

Landed in bb8cc65edb6e402cb05ec85da497cbd605b61514

[ruyadorno]

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