From 0eb82b3287a6cc924f8bcb0ad9df103a1c90f0ec Mon Sep 17 00:00:00 2001 From: LiviaMedeiros Date: Fri, 13 May 2022 23:17:16 +0800 Subject: [PATCH 1/9] util: add `options` to util.promisify() `options.resolveArray` allows to resolve all callback arguments `options.callbackPosition` allows to adjust place of callback --- doc/api/util.md | 22 +++++++++++++++++----- lib/internal/util.js | 39 +++++++++++++++++++++++++++++++-------- 2 files changed, 48 insertions(+), 13 deletions(-) diff --git a/doc/api/util.md b/doc/api/util.md index 8cafd120503764..1bd179a065d3f4 100644 --- a/doc/api/util.md +++ b/doc/api/util.md @@ -1024,9 +1024,16 @@ equality. * `original` {Function} +* `options` {Object} + * `resolveArray` {boolean} **Default:** `false` + * `callbackPosition` {integer|null} **Default:** `null` * Returns: {Function} Takes a function following the common error-first callback style, i.e. taking @@ -1062,11 +1069,16 @@ async function callStat() { If there is an `original[util.promisify.custom]` property present, `promisify` will return its value, see [Custom promisified functions][]. -`promisify()` assumes that `original` is a function taking a callback as its -final argument in all cases. If `original` is not a function, `promisify()` -will throw an error. If `original` is a function but its last argument is not -an error-first callback, it will still be passed an error-first -callback as its last argument. +If `options.resolveArray` is truthy, the promise is resolved with an array of +arguments passed to callback. Otherwise it resolves only with the first one. + +By default, `promisify()` assumes that `original` is a function taking +a callback as its final argument. If `original` is not a function, +`promisify()` will throw an error. If `original` is a function but its last +argument is not an error-first callback, it will still be passed an error-first +callback as its last argument. If `options.callbackPosition` is a number in +range from 0 to `arguments.length`, argument on this position will be used +instead of final one. Using `promisify()` on class methods or other methods that use `this` may not work as expected unless handled specially: diff --git a/lib/internal/util.js b/lib/internal/util.js index 52039d0595c629..ec9c02c8c686af 100644 --- a/lib/internal/util.js +++ b/lib/internal/util.js @@ -5,6 +5,7 @@ const { ArrayIsArray, ArrayPrototypePush, ArrayPrototypeSlice, + ArrayPrototypeSplice, ArrayPrototypeSort, Error, ObjectCreate, @@ -320,14 +321,28 @@ const kCustomPromisifiedSymbol = SymbolFor('nodejs.util.promisify.custom'); const kCustomPromisifyArgsSymbol = Symbol('customPromisifyArgs'); let validateFunction; +let validateInteger; -function promisify(original) { +function promisify(original, options) { // Lazy-load to avoid a circular dependency. - if (validateFunction === undefined) - ({ validateFunction } = require('internal/validators')); + if (validateFunction === undefined || validateInteger === undefined) { + ({ + validateFunction, + validateInteger, + } = require('internal/validators')); + } validateFunction(original, 'original'); + // No validateObject so .map(util.promisify) can work + if (options == null || typeof options !== 'object') { + options = ObjectCreate(null); + } + const { + resolveArray = false, + callbackPosition = null, + } = options; + if (original[kCustomPromisifiedSymbol]) { const fn = original[kCustomPromisifiedSymbol]; @@ -344,19 +359,27 @@ function promisify(original) { function fn(...args) { return new Promise((resolve, reject) => { - ArrayPrototypePush(args, (err, ...values) => { + const callback = (err, ...values) => { if (err) { return reject(err); } + if (resolveArray) { + return resolve(values); + } if (argumentNames !== undefined && values.length > 1) { const obj = {}; for (let i = 0; i < argumentNames.length; i++) obj[argumentNames[i]] = values[i]; - resolve(obj); - } else { - resolve(values[0]); + return resolve(obj); } - }); + return resolve(values[0]); + }; + if (typeof callbackPosition === 'number') { + validateInteger(callbackPosition, 'options.callbackPosition', 0, args.length); + ArrayPrototypeSplice(args, callbackPosition, 0, callback); + } else { + ArrayPrototypePush(args, callback); + } ReflectApply(original, this, args); }); } From d189f86eb384fb4cabe7a7abdf9bb531b5158f54 Mon Sep 17 00:00:00 2001 From: LiviaMedeiros Date: Mon, 16 May 2022 13:09:29 +0800 Subject: [PATCH 2/9] squash: add test --- test/parallel/test-util-promisify-options.js | 169 +++++++++++++++++++ 1 file changed, 169 insertions(+) create mode 100644 test/parallel/test-util-promisify-options.js diff --git a/test/parallel/test-util-promisify-options.js b/test/parallel/test-util-promisify-options.js new file mode 100644 index 00000000000000..d75b39c1117cca --- /dev/null +++ b/test/parallel/test-util-promisify-options.js @@ -0,0 +1,169 @@ +'use strict'; +const common = require('../common'); +const assert = require('assert'); +const { promisify } = require('util'); + +// Callback gets "error" when first two args are the same +function cbError(arg1, arg2) { + return arg1 === arg2 ? { code: 'CbError' } : null; +} + +const cb = { + classic(arg1, arg2, arg3, callback) { + callback(cbError(arg1, arg2), arg3); + }, + smart(arg1, arg2, arg3, callback) { + // Try penultimate argument if the last is not set + callback ??= arg3; + callback(cbError(arg1, arg2), arg3); + }, + returnMultiple(arg1, arg2, arg3, callback) { + callback(cbError(arg1, arg2), arg1, arg2, arg3); + }, + callbackFirst(callback, arg1 = 'default1', arg2 = 'default2', arg3 = 'default3') { + callback(cbError(arg1, arg2), arg3); + }, + callbackSecond(arg1, callback, arg2 = 'default2', arg3 = 'default3') { + callback(cbError(arg1, arg2), arg3); + }, + rest(callback, ...args) { + callback(cbError(args[0], args[1]), args[2]); + }, + returnRest(callback, ...args) { + callback(cbError(args[0], args[1]), ...args); + }, + hybrid(arg1, arg2, callback, ...args) { + callback(cbError(arg1, arg2), args.length); + }, + returnHybrid(arg1, arg2, callback, ...args) { + callback(cbError(arg1, arg2), ...args); + }, +}; + +// Test that function name and length are always retained +Object.entries(cb).forEach(([fnName, fn]) => { + const promisifiedFn = promisify(fn); + assert.strictEqual(fnName, promisifiedFn.name); + assert.strictEqual(fn.name, promisifiedFn.name); + assert.strictEqual(fn.length, promisifiedFn.length); +}); + +// Tests for invalid numbers +{ + [ + NaN, -Infinity, -1, 1.5, 4, Infinity, + ].forEach((invalidCallbackPosition) => { + assert.rejects( + promisify(cb.classic, { callbackPosition: invalidCallbackPosition })(1, 2, 3), + { code: 'ERR_OUT_OF_RANGE' } + ); + }); +} + +// Various tests +(async () => { + assert.strictEqual( + await promisify(cb.classic)(1, 2, 3), + 3 + ); + assert.strictEqual( + await promisify(cb.classic, { callbackPosition: 3 })(1, 2, 3), + 3 + ); + assert.deepStrictEqual( + await promisify(cb.classic, { resolveArray: true })(1, 2, 3), + [3] + ); + assert.rejects( + promisify(cb.classic)(1, 1, 3), + { code: 'CbError' } + ); + assert.rejects( + promisify(cb.classic)(1, 2), + TypeError + ); + + assert.strictEqual( + await promisify(cb.smart)(1, 2, 3), + 3 + ); + assert.strictEqual( + typeof await promisify(cb.smart)(1, 2), + 'function' + ); + assert.strictEqual( + await promisify(cb.smart, { callbackPosition: 3 })(1, 2, 3), + 3 + ); + assert.strictEqual( + typeof await promisify(cb.smart, { callbackPosition: 2 })(1, 2), + 'function' + ); + assert.rejects( + promisify(cb.smart)(1, 1, 3), + { code: 'CbError' } + ); + assert.rejects( + promisify(cb.smart, { callbackPosition: 3 })(1, 2), + { code: 'ERR_OUT_OF_RANGE' } + ); + assert.rejects( + promisify(cb.smart, { callbackPosition: 2 })(1, 2, 3), + TypeError + ); + + assert.strictEqual( + await promisify(cb.returnMultiple, { resolveArray: false })(1, 2, 3), + 1 + ); + assert.deepStrictEqual( + await promisify(cb.returnMultiple, { resolveArray: true })(1, 2, 3), + [1, 2, 3] + ); + + assert.strictEqual( + await promisify(cb.callbackFirst, { callbackPosition: 0 })(1, 2, 3), + 3 + ); + assert.strictEqual( + await promisify(cb.callbackFirst, { callbackPosition: 0 })(1, 2), + 'default3' + ); + + assert.strictEqual( + await promisify(cb.callbackSecond, { callbackPosition: 1 })(1, 2, 3), + 3 + ); + assert.strictEqual( + await promisify(cb.callbackSecond, { callbackPosition: 1 })(1, 2), + 'default3' + ); + + assert.strictEqual( + await promisify(cb.rest, { callbackPosition: 0 })(1, 2, 3, 4), + 3 + ); + assert.rejects( + promisify(cb.rest, { callbackPosition: 0 })(1, 1, 3, 4), + { code: 'CbError' } + ); + assert.strictEqual( + await promisify(cb.rest, { callbackPosition: 0 })(1, 2), + undefined + ); + + assert.deepStrictEqual( + await promisify(cb.returnRest, { callbackPosition: 0, resolveArray: true })(1, 2, 3, 4, 5), + [1, 2, 3, 4, 5] + ); + + assert.strictEqual( + await promisify(cb.hybrid, { callbackPosition: 2 })(1, 2, 3, 4, 5, 6), + 4 + ); + + assert.deepStrictEqual( + await promisify(cb.returnHybrid, { callbackPosition: 2, resolveArray: true })(1, 2, 3, 4, 5, 6), + [3, 4, 5, 6] + ); +})().then(common.mustCall()); From fd7fc129b6f80698f86624ec2101ae2875e2376b Mon Sep 17 00:00:00 2001 From: LiviaMedeiros Date: Wed, 18 May 2022 14:51:42 +0800 Subject: [PATCH 3/9] squash: add `resolveObject` option --- doc/api/util.md | 4 +++ lib/internal/util.js | 5 ++-- test/parallel/test-util-promisify-options.js | 29 ++++++++++++++++++++ 3 files changed, 36 insertions(+), 2 deletions(-) diff --git a/doc/api/util.md b/doc/api/util.md index 1bd179a065d3f4..7b1b308cd9bc6f 100644 --- a/doc/api/util.md +++ b/doc/api/util.md @@ -1033,6 +1033,7 @@ changes: * `original` {Function} * `options` {Object} * `resolveArray` {boolean} **Default:** `false` + * `resolveObject` {Array|null} **Default:** `null` * `callbackPosition` {integer|null} **Default:** `null` * Returns: {Function} @@ -1072,6 +1073,9 @@ will return its value, see [Custom promisified functions][]. If `options.resolveArray` is truthy, the promise is resolved with an array of arguments passed to callback. Otherwise it resolves only with the first one. +If `options.resolveObject` is an array, the promise is resolved with an object +having its values as keys and callback arguments as values. + By default, `promisify()` assumes that `original` is a function taking a callback as its final argument. If `original` is not a function, `promisify()` will throw an error. If `original` is a function but its last diff --git a/lib/internal/util.js b/lib/internal/util.js index ec9c02c8c686af..cb3c3aab7e7596 100644 --- a/lib/internal/util.js +++ b/lib/internal/util.js @@ -340,6 +340,7 @@ function promisify(original, options) { } const { resolveArray = false, + resolveObject = null, callbackPosition = null, } = options; @@ -355,7 +356,7 @@ function promisify(original, options) { // Names to create an object from in case the callback receives multiple // arguments, e.g. ['bytesRead', 'buffer'] for fs.read. - const argumentNames = original[kCustomPromisifyArgsSymbol]; + const argumentNames = resolveObject || original[kCustomPromisifyArgsSymbol]; function fn(...args) { return new Promise((resolve, reject) => { @@ -366,7 +367,7 @@ function promisify(original, options) { if (resolveArray) { return resolve(values); } - if (argumentNames !== undefined && values.length > 1) { + if (argumentNames) { const obj = {}; for (let i = 0; i < argumentNames.length; i++) obj[argumentNames[i]] = values[i]; diff --git a/test/parallel/test-util-promisify-options.js b/test/parallel/test-util-promisify-options.js index d75b39c1117cca..692fa515b0f353 100644 --- a/test/parallel/test-util-promisify-options.js +++ b/test/parallel/test-util-promisify-options.js @@ -74,6 +74,10 @@ Object.entries(cb).forEach(([fnName, fn]) => { await promisify(cb.classic, { resolveArray: true })(1, 2, 3), [3] ); + assert.deepStrictEqual( + await promisify(cb.classic, { resolveObject: ['kFoo'] })(1, 2, 3), + { kFoo: 3 } + ); assert.rejects( promisify(cb.classic)(1, 1, 3), { code: 'CbError' } @@ -120,6 +124,10 @@ Object.entries(cb).forEach(([fnName, fn]) => { await promisify(cb.returnMultiple, { resolveArray: true })(1, 2, 3), [1, 2, 3] ); + assert.deepStrictEqual( + await promisify(cb.returnMultiple, { resolveObject: ['kFoo', 'kBar', 'kBaz'] })(1, 2, 3), + { kFoo: 1, kBar: 2, kBaz: 3 } + ); assert.strictEqual( await promisify(cb.callbackFirst, { callbackPosition: 0 })(1, 2, 3), @@ -156,6 +164,20 @@ Object.entries(cb).forEach(([fnName, fn]) => { await promisify(cb.returnRest, { callbackPosition: 0, resolveArray: true })(1, 2, 3, 4, 5), [1, 2, 3, 4, 5] ); + assert.deepStrictEqual( + await promisify(cb.returnRest, { + callbackPosition: 0, + resolveObject: ['a', 'b', 'c', 'd', 'e'] + })(1, 2, 3, 4, 5), + { a: 1, b: 2, c: 3, d: 4, e: 5 } + ); + assert.deepStrictEqual( + await promisify(cb.returnRest, { + callbackPosition: 0, + resolveObject: ['a', 'b', 'c', 'd', 'e', 'f'], + })(1, 2, 3, 4, 5), + { a: 1, b: 2, c: 3, d: 4, e: 5, f: undefined } + ); assert.strictEqual( await promisify(cb.hybrid, { callbackPosition: 2 })(1, 2, 3, 4, 5, 6), @@ -166,4 +188,11 @@ Object.entries(cb).forEach(([fnName, fn]) => { await promisify(cb.returnHybrid, { callbackPosition: 2, resolveArray: true })(1, 2, 3, 4, 5, 6), [3, 4, 5, 6] ); + assert.deepStrictEqual( + await promisify(cb.returnHybrid, { + callbackPosition: 2, + resolveObject: ['a', 'b', 'c', 'd', 'e', 'f'] + })(1, 2, 3, 4, 5, 6), + { a: 3, b: 4, c: 5, d: 6, e: undefined, f: undefined } + ); })().then(common.mustCall()); From 9420a34cee79b704dd47fcd9985d6ec12a1f2c8b Mon Sep 17 00:00:00 2001 From: Livia Medeiros <74449973+LiviaMedeiros@users.noreply.github.com> Date: Wed, 18 May 2022 16:26:33 +0800 Subject: [PATCH 4/9] squash: add explicit default `undefined` for optional parameter Co-authored-by: Antoine du Hamel --- lib/internal/util.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/internal/util.js b/lib/internal/util.js index cb3c3aab7e7596..1719c8a57ee1e2 100644 --- a/lib/internal/util.js +++ b/lib/internal/util.js @@ -323,7 +323,7 @@ const kCustomPromisifyArgsSymbol = Symbol('customPromisifyArgs'); let validateFunction; let validateInteger; -function promisify(original, options) { +function promisify(original, options = undefined) { // Lazy-load to avoid a circular dependency. if (validateFunction === undefined || validateInteger === undefined) { ({ From eba911f62cf68df409ddf52f002e0f2d60e3eb53 Mon Sep 17 00:00:00 2001 From: LiviaMedeiros Date: Wed, 18 May 2022 16:34:49 +0800 Subject: [PATCH 5/9] squash: avoid creating throwaway object --- lib/internal/util.js | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/lib/internal/util.js b/lib/internal/util.js index 1719c8a57ee1e2..5591426aeee69b 100644 --- a/lib/internal/util.js +++ b/lib/internal/util.js @@ -335,14 +335,10 @@ function promisify(original, options = undefined) { validateFunction(original, 'original'); // No validateObject so .map(util.promisify) can work - if (options == null || typeof options !== 'object') { - options = ObjectCreate(null); - } - const { - resolveArray = false, - resolveObject = null, - callbackPosition = null, - } = options; + const useDefaultOptions = options == null || typeof options !== 'object'; + const resolveArray = useDefaultOptions ? false : options.resolveArray; + const resolveObject = useDefaultOptions ? null : options.resolveObject; + const callbackPosition = useDefaultOptions ? null : options.callbackPosition; if (original[kCustomPromisifiedSymbol]) { const fn = original[kCustomPromisifiedSymbol]; From 6f4204365276bc750d67d45764d6929f7665b6b5 Mon Sep 17 00:00:00 2001 From: LiviaMedeiros Date: Fri, 20 May 2022 12:27:50 +0800 Subject: [PATCH 6/9] squash: add `noCustom` option --- doc/api/util.md | 8 +++++--- lib/internal/util.js | 15 +++++++++------ 2 files changed, 14 insertions(+), 9 deletions(-) diff --git a/doc/api/util.md b/doc/api/util.md index 7b1b308cd9bc6f..163bb2148b4009 100644 --- a/doc/api/util.md +++ b/doc/api/util.md @@ -1020,7 +1020,7 @@ Otherwise, returns `false`. See [`assert.deepStrictEqual()`][] for more information about deep strict equality. -## `util.promisify(original)` +## `util.promisify(original[, options])`