util: add options to util.promisify()

[LiviaMedeiros]

This PR adds an optional second argument to util.promisify().

options.resolveArray solves a problem with callbacks taking more than one non-error argument: Promise can't be resolved with more than one value, so further results are lost. With this option, Promise is resolved with an array of all non-error arguments.
For internal methods, this problem is solved with appropriate kCustomPromisifiedSymbol and kCustomPromisifyArgsSymbol properties. But util.promisify() is a part of public API, and this approach is not applicable to userland.

options.callbackPosition solves a problem with functions not having callback-last signature.
Most notably, a function can't have callback as last argument when we want to be able to omit last arguments or use ...rest:

• We can't do util.promisify((foo, optionalBar, cb) => cb(!foo, optionalBar))('fooValue', callbackFn) and expect optionalBar to be magically undefined or arguments.length to be magically adjusted.
• We can't do (foo, ...optionalArgs, cb) => {} at all.

With that new option, we'll be able to promisify (foo, cb, optionalBar = 'defaultBar', ...restArgs) => {}.

Example:

'use strict';
const { promisify } = require('node:util');

const fn = (foo, bar, baz, cb) => void cb(baz !== undefined, foo, bar, baz);
const fnOpt = (cb, ...args) => void cb(args[2] !== undefined, ...args);

// old
const Pfn = promisify(fn);
(async () => {
  console.log(await Pfn(1, 2, undefined)); // 1
  console.log(await Pfn(1, 2)); // TypeError: cb is not a function
})().catch(err => console.error('caught:', err));

// new
const PfnArr = promisify(fn, { resolveArray: true }); // callbackPosition: 3
const PfnOpt = promisify(fnOpt, { resolveArray: true, callbackPosition: 0 });
(async () => {
  console.log(await PfnArr(1, 2, undefined)); // [ 1, 2, undefined ]
  console.log(await PfnOpt(1, 2)); // [ 1, 2 ]
  console.log(await PfnArr(1, 2, 3)); // caught: true (desired truthy error)
})().catch(err => console.error('caught:', err));

Documentation might require some rewording.

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[aduh95]

import { promisify } from 'node:util';

const fn = (foo, bar, baz, cb) => void cb(baz !== undefined, foo, bar, baz);
const Pfn = promisify(fn);
console.log(await Pfn(1, 2)); // TypeError: cb is not a function

Worth noting that the TypeError is not on util.promisify, it's the original function that requires to pass three parameter, I don't think util.promisify should change that.

const fn = (foo, bar, baz, cb) => void cb(baz !== undefined, foo, bar, baz);
fn(1, 2, undefined, () => {}); // works
fn(1, 2, () => {}); // TypeError: cb is not a function

const fnOpt = (cb, ...args) => void cb(args[2] !== undefined, ...args);

I don't think that pattern is very common, probably not to the point it deserves a change of the promisify API.

For internal methods, this problem is solved with appropriate kCustomPromisifiedSymbol and kCustomPromisifyArgsSymbol properties. But util.promisify() is a part of public API, and this approach is not applicable to userland.

Worth noting that kCustomPromisifiedSymbol is part of the public API as well: https://nodejs.org/api/util.html#utilpromisifycustom

[LiviaMedeiros]

Worth noting that the TypeError is not on util.promisify, it's the original function that requires to pass three parameter, I don't think util.promisify should change that.

Yes, this version doesn't change that.
Even if callbackPosition explicitly points on the last parameter, promisify(fn, { callbackPosition: 3 })(1, 2) will throw RangeError.

Thoughts behind this

I'm considering an additional option that would allow to force the number of parameters desired by original function. Roughly:

• magicArgs: Pfn(foo, bar) -> fn(foo, bar, undefined, callback)
• arrayArgs: Pfn([foo, bar]) -> fn(foo, bar, undefined, callback)

This looks convenient when fn() doesn't have any boilerplate code inside, checking arguments.length, typeof baz === 'function', etc.
However, IMHO it wouldn't add anything but questionable syntax sugar, complexity and potential confusion.

Worth noting that kCustomPromisifiedSymbol is part of the public API as well: https://nodejs.org/api/util.html#utilpromisifycustom

It is, while kCustomPromisifyArgsSymbol (Symbol('customPromisifyArgs')) is not. And I'm not 100% confident that it should be exposed.
Even if it was, at least it won't help with variable number of arguments (although technically it's a feasible feature, and Iterable options.resolveObject might be added if there will be use cases worth for that).

[tniessen]

For internal methods, this problem is solved with appropriate kCustomPromisifiedSymbol and kCustomPromisifyArgsSymbol properties. But util.promisify() is a part of public API, and this approach is not applicable to userland.

Worth noting that kCustomPromisifiedSymbol is part of the public API as well: https://nodejs.org/api/util.html#utilpromisifycustom

It is, while kCustomPromisifyArgsSymbol (Symbol('customPromisifyArgs')) is not. And I'm not 100% confident that it should be exposed.

My understanding is that kCustomPromisifyArgsSymbol is mostly a convenience feature for internals, and anything that can be achieved using kCustomPromisifyArgsSymbol can also be achieved using the public util.promisify.custom symbol.

When I come across unusual signatures, it's often enough to have a simple wrapper (with some downsides such as losing the function name, etc.):

const PfnArr = promisify((foo, bar, baz, cb) => fn(foo, bar, baz, (err, ...r) => cb(err, r));

That being said, as far as I can tell, these new options still wouldn't support the conversion of multiple results to an object (instead of to an array), which can be implemented using kCustomPromisifyArgsSymbol internally and using util.promisify.custom publicly.

[LiviaMedeiros]

My understanding is that kCustomPromisifyArgsSymbol is mostly a convenience feature for internals, and anything that can be achieved using kCustomPromisifyArgsSymbol can also be achieved using the public util.promisify.custom symbol.

Yep, technically, anything can be achieved with the util.promisify.custom; and this approach is fine to be used under the hood. But in userland, it requires a full-fledged user-defined function. At this point, util.promisify usually becomes redundant (no benefits over calling custom function directly), inconvenient (we must define property on original function just to implicitly retrieve it afterwards) and inconsistent (see below).

The inconsistent part

const util = require('node:util');
const child_process = require('node:child_process');
const fs = require('node:fs');
const readline = require('node:readline');
const http2 = require('node:http2');

function fancyNamedFunction(a, b, cb) { cb(a + b); }
function dummyFunction() {}
console.log('fNF:', fancyNamedFunction.name);
console.log('P fNF:', util.promisify(fancyNamedFunction).name);

Object.defineProperty(fancyNamedFunction, util.promisify.custom, {
  value: (a, b) => {
    return new Promise((resolve) => fancyNamedFunction(a, b, resolve));
  }, configurable: true
});
console.log('C fNFe:', util.promisify(fancyNamedFunction).name);

Object.defineProperty(fancyNamedFunction, util.promisify.custom, {
  value: dummyFunction
});
console.log('D fNFe:', util.promisify(fancyNamedFunction).name);

console.log('exec:', child_process.exec.name);
console.log('C exec:', util.promisify(child_process.exec).name);

console.log('exists:', fs.exists.name);
console.log('C exists:', util.promisify(fs.exists).name);
console.log('P read:', util.promisify(fs.read).name);

console.log('connect:', http2.connect.name);
console.log('C connect:', util.promisify(http2.connect).name);

fNF: fancyNamedFunction
P fNF: fancyNamedFunction
C fNFe: value
D fNFe: dummyFunction
exec: exec
C exec:
exists: exists
C exists: value
P read: read
connect: connect
C connect: value

Of course "customly" promisified function doesn't inherit any other property as well, while "normally" promisified does.
This part is probably just a bug, roughly fixable by configurability workaround of

- ObjectDefineProperty(fn, kCustomPromisifiedSymbol, {
-   value: fn, enumerable: false, writable: false, configurable: true
- })
+ ObjectSetPrototypeOf(fn, ObjectGetPrototypeOf(original));
+ ObjectDefineProperties(fn, ObjectGetOwnPropertyDescriptors(original));

Also, while we're at it, was there any intentional reason to keep .length from original on promisified function? I think it should be either original.length - 1 or promisified.length.

That being said, as far as I can tell, these new options still wouldn't support the conversion of multiple results to an object (instead of to an array), which can be implemented using kCustomPromisifyArgsSymbol internally and using util.promisify.custom publicly.

I originally considered array to be superior, but it makes sense. Added resolveObject option to keep this feature as well.

[tniessen]

At this point, util.promisify usually becomes redundant (no benefits over calling custom function directly), inconvenient (we must define property on original function just to implicitly retrieve it afterwards)

I don't think I agree with this. The point of util.promisify.custom is not for the author to be able to use the promisified function themselves, but so that other people using the function can pass it to util.promisify() and get a good API designed by the author of the original function. If the author doesn't override util.promisify.custom, most function signatures will just work with the built-in behavior, and the very few unusual signatures can be fixed with a wrapper function.

In fact, this feels wrong to me:

// Third-party library:
const fn = (a, b, cb) => cb(null, a, b);

// Somewhere:
fn[util.promisify.custom] = util.promisify(fn);

// Application A:
util.promisify(fn)('a', 'b').then((x) => console.log({ x }));

// Application B:
// fn.length is 3, so we will surely get an array of 2 values back... nope.
util.promisify(fn, {
  resolveArray: true
})('a', 'b').then((x) => console.log({ y });

I'd consider most node-style callback APIs legacy at this point, with a few exceptions. Many modern runtimes don't have a built-in promisify() API at all. I don't know if adding complexity to support unusual (legacy) signatures is really worth it.

[LiviaMedeiros]

The point of util.promisify.custom is not for the author to be able to use the promisified function themselves, but so that other people using the function can pass it to util.promisify() and get a good API designed by the author of the original function.

The main usecase for options is if author of callback-oriented third-party functions didn't define promisify.custom (for whatever reason: "simplicity", religious, proctastic, or the code isn't Node.js-oriented) but had to use "unusual" signature.
The second is, yes, to promisify own functions. It is more convenient, especially if we want to keep ownProperties from originals or guarantee strict alignment with other promisified functions.
The third is for authors who want to add promisify.custom without having to write a wrapper manually, e.g. stRead[promisify.custom] = promisify(stRead, { resolveArray: true });

In fact, this feels wrong to me

It is wrong indeed! Updated it to neither get or set [promisify.custom] if options are used.

This also solves potential issues in case of this scenario:

// third-party
function stRead(buffer, ..., cb) {
  ...
  cb(err, byteLength, buffer)
}

// end-user code is happy with byteLength and doesn't need returned buffer, they already have it
PstRead = promisifiy(stRead);
const length = await PstRead(buffer, ...);

// someone (author of thirdpartie?) somewhere suddenly improved it
stRead[promisify.custom] = (...) => new Promise((res) => stRead(..., (err, byteLength, buffer) => res({ byteLength, buffer }));

// now end-user code is broken and there was no way to preemptively avoid that
typeof await PstRead(buffer, ...) === 'number'; // false

// this would work as long as stRead itself is unchanged
typeof (await promisify(stRead, { resolveObject: ['byteLength'] })(buffer, ...)).byteLength === 'number';
typeof await promisify(stRead, { resolveObject: false })(buffer, ...) === 'number';

Also, a factor of lower importance: it feels weird for the original function decoration to give names to its "returned" values. Idiomatically, these names should be unexposed (resolveArray is ideal for that); and if values have to be named, it should be done on the calling side.

Of course if opinions on unusualness of different signatures across ecosystem, legacy reasons and complexity are still strong enough, I'm not insisting on adding that. :)

[LiviaMedeiros]

Closing as unneeded; the alternative is userland implementation. Thanks for reviews!