From 0fbeabe84359837c5e5f20beb5766d3b9a695286 Mon Sep 17 00:00:00 2001 From: Dylan Conway Date: Fri, 2 May 2025 12:09:48 -0700 Subject: [PATCH 1/4] fixes --- src/js/internal/promisify.ts | 18 +- .../node/test/parallel/test-util-promisify.js | 232 ++++++++++++++++++ .../test/parallel/test-util-types-exists.js | 6 + 3 files changed, 251 insertions(+), 5 deletions(-) create mode 100644 test/js/node/test/parallel/test-util-promisify.js create mode 100644 test/js/node/test/parallel/test-util-types-exists.js diff --git a/src/js/internal/promisify.ts b/src/js/internal/promisify.ts index 305fda50356b7a..a27e76938b867f 100644 --- a/src/js/internal/promisify.ts +++ b/src/js/internal/promisify.ts @@ -1,6 +1,8 @@ const kCustomPromisifiedSymbol = Symbol.for("nodejs.util.promisify.custom"); const kCustomPromisifyArgsSymbol = Symbol("customPromisifyArgs"); +const { validateFunction } = require("internal/validators"); + function defineCustomPromisify(target, callback) { Object.defineProperty(target, kCustomPromisifiedSymbol, { value: callback, @@ -19,12 +21,10 @@ function defineCustomPromisifyArgs(target, args) { } var promisify = function promisify(original) { - if (typeof original !== "function") throw new TypeError('The "original" argument must be of type Function'); + validateFunction(original, "original"); const custom = original[kCustomPromisifiedSymbol]; if (custom) { - if (typeof custom !== "function") { - throw new TypeError('The "util.promisify.custom" argument must be of type Function'); - } + validateFunction(custom, "custom"); // ensure that we don't create another promisified function wrapper return defineCustomPromisify(custom, custom); } @@ -33,7 +33,7 @@ var promisify = function promisify(original) { function fn(...originalArgs) { const { promise, resolve, reject } = Promise.withResolvers(); try { - original.$apply(this, [ + const maybePromise = original.$apply(this, [ ...originalArgs, function (err, ...values) { if (err) { @@ -57,6 +57,14 @@ var promisify = function promisify(original) { } }, ]); + + if ($isPromise(maybePromise)) { + process.emitWarning( + "Calling promisify on a function that returns a Promise is likely a mistake.", + "DeprecationWarning", + "DEP0174", + ); + } } catch (err) { reject(err); } diff --git a/test/js/node/test/parallel/test-util-promisify.js b/test/js/node/test/parallel/test-util-promisify.js new file mode 100644 index 00000000000000..f0e5eceefede01 --- /dev/null +++ b/test/js/node/test/parallel/test-util-promisify.js @@ -0,0 +1,232 @@ +'use strict'; +// Flags: --expose-internals +const common = require('../common'); +const assert = require('assert'); +const fs = require('fs'); +const vm = require('vm'); +const { promisify } = require('util'); +// const { customPromisifyArgs } = require('internal/util'); + +{ + const warningHandler = common.mustNotCall(); + process.on('warning', warningHandler); + function foo() {} + foo.constructor = (async () => {}).constructor; + promisify(foo); + process.off('warning', warningHandler); +} + +common.expectWarning( + 'DeprecationWarning', + 'Calling promisify on a function that returns a Promise is likely a mistake.', + 'DEP0174'); +promisify(async (callback) => { callback(); })().then(common.mustCall(() => { + // We must add the second `expectWarning` call in the `.then` handler, when + // the first warning has already been triggered. + common.expectWarning( + 'DeprecationWarning', + 'Calling promisify on a function that returns a Promise is likely a mistake.', + 'DEP0174'); + promisify(async () => {})().then(common.mustNotCall()); +})); + +const stat = promisify(fs.stat); + +{ + const promise = stat(__filename); + assert(promise instanceof Promise); + promise.then(common.mustCall((value) => { + assert.deepStrictEqual(value, fs.statSync(__filename)); + })); +} + +{ + const promise = stat('/dontexist'); + promise.catch(common.mustCall((error) => { + assert(error.message.includes('ENOENT: no such file or directory, stat')); + })); +} + +{ + function fn() {} + + function promisifedFn() {} + fn[promisify.custom] = promisifedFn; + assert.strictEqual(promisify(fn), promisifedFn); + assert.strictEqual(promisify(promisify(fn)), promisifedFn); +} + +{ + function fn() {} + + function promisifiedFn() {} + + // util.promisify.custom is a shared symbol which can be accessed + // as `Symbol.for("nodejs.util.promisify.custom")`. + const kCustomPromisifiedSymbol = Symbol.for('nodejs.util.promisify.custom'); + fn[kCustomPromisifiedSymbol] = promisifiedFn; + + assert.strictEqual(kCustomPromisifiedSymbol, promisify.custom); + assert.strictEqual(promisify(fn), promisifiedFn); + assert.strictEqual(promisify(promisify(fn)), promisifiedFn); +} + +{ + function fn() {} + fn[promisify.custom] = 42; + assert.throws( + () => promisify(fn), + { code: 'ERR_INVALID_ARG_TYPE', name: 'TypeError' } + ); +} + +// { +// const firstValue = 5; +// const secondValue = 17; + +// function fn(callback) { +// callback(null, firstValue, secondValue); +// } + +// fn[customPromisifyArgs] = ['first', 'second']; + +// promisify(fn)().then(common.mustCall((obj) => { +// assert.deepStrictEqual(obj, { first: firstValue, second: secondValue }); +// })); +// } + +{ + const fn = vm.runInNewContext('(function() {})'); + assert.notStrictEqual(Object.getPrototypeOf(promisify(fn)), + Function.prototype); +} + +{ + function fn(callback) { + callback(null, 'foo', 'bar'); + } + promisify(fn)().then(common.mustCall((value) => { + assert.strictEqual(value, 'foo'); + })); +} + +{ + function fn(callback) { + callback(null); + } + promisify(fn)().then(common.mustCall((value) => { + assert.strictEqual(value, undefined); + })); +} + +{ + function fn(callback) { + callback(); + } + promisify(fn)().then(common.mustCall((value) => { + assert.strictEqual(value, undefined); + })); +} + +{ + function fn(err, val, callback) { + callback(err, val); + } + promisify(fn)(null, 42).then(common.mustCall((value) => { + assert.strictEqual(value, 42); + })); +} + +{ + function fn(err, val, callback) { + callback(err, val); + } + promisify(fn)(new Error('oops'), null).catch(common.mustCall((err) => { + assert.strictEqual(err.message, 'oops'); + })); +} + +{ + function fn(err, val, callback) { + callback(err, val); + } + + (async () => { + const value = await promisify(fn)(null, 42); + assert.strictEqual(value, 42); + })().then(common.mustCall()); +} + +{ + const o = {}; + const fn = promisify(function(cb) { + + cb(null, this === o); + }); + + o.fn = fn; + + o.fn().then(common.mustCall((val) => assert(val))); +} + +{ + const err = new Error('Should not have called the callback with the error.'); + const stack = err.stack; + + const fn = promisify(function(cb) { + cb(null); + cb(err); + }); + + (async () => { + await fn(); + await Promise.resolve(); + return assert.strictEqual(stack, err.stack); + })().then(common.mustCall()); +} + +{ + function c() { } + const a = promisify(function() { }); + const b = promisify(a); + assert.notStrictEqual(c, a); + assert.strictEqual(a, b); +} + +{ + let errToThrow; + const thrower = promisify(function(a, b, c, cb) { + errToThrow = new Error(); + throw errToThrow; + }); + thrower(1, 2, 3) + .then(assert.fail) + .then(assert.fail, (e) => assert.strictEqual(e, errToThrow)); +} + +{ + const err = new Error(); + + const a = promisify((cb) => cb(err))(); + const b = promisify(() => { throw err; })(); + + Promise.all([ + a.then(assert.fail, function(e) { + assert.strictEqual(err, e); + }), + b.then(assert.fail, function(e) { + assert.strictEqual(err, e); + }), + ]); +} + +[undefined, null, true, 0, 'str', {}, [], Symbol()].forEach((input) => { + assert.throws( + () => promisify(input), + { + code: 'ERR_INVALID_ARG_TYPE', + name: 'TypeError', + message: 'The "original" argument must be of type function.' + + common.invalidArgTypeHelper(input) + }); +}); diff --git a/test/js/node/test/parallel/test-util-types-exists.js b/test/js/node/test/parallel/test-util-types-exists.js new file mode 100644 index 00000000000000..6a66be87fb384b --- /dev/null +++ b/test/js/node/test/parallel/test-util-types-exists.js @@ -0,0 +1,6 @@ +'use strict'; + +require('../common'); +const assert = require('assert'); + +assert.strictEqual(require('util/types'), require('util').types); From 7152440e245fab0991721f154d3d37388adf0795 Mon Sep 17 00:00:00 2001 From: Dylan Conway Date: Fri, 2 May 2025 12:28:52 -0700 Subject: [PATCH 2/4] fix test-util-promisify-custom-names.mjs --- src/js/node/fs.ts | 7 ++- src/js/node/readline.ts | 50 ++++++++++--------- src/js/node/timers.promises.ts | 25 ++++++---- .../test-util-promisify-custom-names.mjs | 40 +++++++++++++++ 4 files changed, 86 insertions(+), 36 deletions(-) create mode 100644 test/js/node/test/parallel/test-util-promisify-custom-names.mjs diff --git a/src/js/node/fs.ts b/src/js/node/fs.ts index f0aedf128d573e..b85b90125b4c45 100644 --- a/src/js/node/fs.ts +++ b/src/js/node/fs.ts @@ -619,7 +619,12 @@ var access = function access(path, mode, callback) { const { defineCustomPromisifyArgs } = require("internal/promisify"); var kCustomPromisifiedSymbol = Symbol.for("nodejs.util.promisify.custom"); -exists[kCustomPromisifiedSymbol] = path => new Promise(resolve => exists(path, resolve)); +{ + const existsCb = exists; + exists[kCustomPromisifiedSymbol] = function exists(path) { + return new Promise(resolve => existsCb(path, resolve)); + }; +} defineCustomPromisifyArgs(read, ["bytesRead", "buffer"]); defineCustomPromisifyArgs(readv, ["bytesRead", "buffers"]); defineCustomPromisifyArgs(write, ["bytesWritten", "buffer"]); diff --git a/src/js/node/readline.ts b/src/js/node/readline.ts index 277972770254eb..59d75ad490b44c 100644 --- a/src/js/node/readline.ts +++ b/src/js/node/readline.ts @@ -2268,32 +2268,34 @@ Interface.prototype.question = function question(query, options, cb) { } }; -Interface.prototype.question[promisify.custom] = function question(query, options) { - if (options === null || typeof options !== "object") { - options = kEmptyObject; - } - - var signal = options?.signal; +{ + Interface.prototype.question[promisify.custom] = function question(query, options) { + if (options === null || typeof options !== "object") { + options = kEmptyObject; + } - if (signal && signal.aborted) { - return PromiseReject($makeAbortError(undefined, { cause: signal.reason })); - } + var signal = options?.signal; - return new Promise((resolve, reject) => { - var cb = resolve; - if (signal) { - var onAbort = () => { - reject($makeAbortError(undefined, { cause: signal.reason })); - }; - signal.addEventListener("abort", onAbort, { once: true }); - cb = answer => { - signal.removeEventListener("abort", onAbort); - resolve(answer); - }; - } - this.question(query, options, cb); - }); -}; + if (signal && signal.aborted) { + return PromiseReject($makeAbortError(undefined, { cause: signal.reason })); + } + + return new Promise((resolve, reject) => { + var cb = resolve; + if (signal) { + var onAbort = () => { + reject($makeAbortError(undefined, { cause: signal.reason })); + }; + signal.addEventListener("abort", onAbort, { once: true }); + cb = answer => { + signal.removeEventListener("abort", onAbort); + resolve(answer); + }; + } + this.question(query, options, cb); + }); + }; +} /** * Creates a new `readline.Interface` instance. diff --git a/src/js/node/timers.promises.ts b/src/js/node/timers.promises.ts index eab85e0a2323bb..c08a4a41103133 100644 --- a/src/js/node/timers.promises.ts +++ b/src/js/node/timers.promises.ts @@ -4,6 +4,9 @@ const { validateBoolean, validateAbortSignal, validateObject, validateNumber } = require("internal/validators"); const symbolAsyncIterator = Symbol.asyncIterator; +const setImmediateGlobal = globalThis.setImmediate; +const setTimeoutGlobal = globalThis.setTimeout; +const setIntervalGlobal = globalThis.setInterval; function asyncIterator({ next: nextFunction, return: returnFunction }) { const result = {}; @@ -20,7 +23,7 @@ function asyncIterator({ next: nextFunction, return: returnFunction }) { return result; } -function setTimeoutPromise(after = 1, value, options = {}) { +function setTimeout(after = 1, value, options = {}) { const arguments_ = [].concat(value ?? []); try { // If after is a number, but an invalid one (too big, Infinity, NaN), we only want to emit a @@ -53,7 +56,7 @@ function setTimeoutPromise(after = 1, value, options = {}) { } let onCancel; const returnValue = new Promise((resolve, reject) => { - const timeout = setTimeout(() => resolve(value), after, ...arguments_); + const timeout = setTimeoutGlobal(() => resolve(value), after, ...arguments_); if (!reference) { timeout?.unref?.(); } @@ -70,7 +73,7 @@ function setTimeoutPromise(after = 1, value, options = {}) { : returnValue; } -function setImmediatePromise(value, options = {}) { +function setImmediate(value, options = {}) { try { validateObject(options, "options"); } catch (error) { @@ -92,7 +95,7 @@ function setImmediatePromise(value, options = {}) { } let onCancel; const returnValue = new Promise((resolve, reject) => { - const immediate = setImmediate(() => resolve(value)); + const immediate = setImmediateGlobal(() => resolve(value)); if (!reference) { immediate?.unref?.(); } @@ -109,7 +112,7 @@ function setImmediatePromise(value, options = {}) { : returnValue; } -function setIntervalPromise(after = 1, value, options = {}) { +function setInterval(after = 1, value, options = {}) { /* eslint-disable no-undefined, no-unreachable-loop, no-loop-func */ try { // If after is a number, but an invalid one (too big, Infinity, NaN), we only want to emit a @@ -166,7 +169,7 @@ function setIntervalPromise(after = 1, value, options = {}) { try { let notYielded = 0; let callback; - interval = setInterval(() => { + interval = setIntervalGlobal(() => { notYielded++; if (callback) { callback(); @@ -228,11 +231,11 @@ function setIntervalPromise(after = 1, value, options = {}) { } export default { - setTimeout: setTimeoutPromise, - setImmediate: setImmediatePromise, - setInterval: setIntervalPromise, + setTimeout, + setImmediate, + setInterval, scheduler: { - wait: (delay, options) => setTimeoutPromise(delay, undefined, options), - yield: setImmediatePromise, + wait: (delay, options) => setTimeout(delay, undefined, options), + yield: setImmediate, }, }; diff --git a/test/js/node/test/parallel/test-util-promisify-custom-names.mjs b/test/js/node/test/parallel/test-util-promisify-custom-names.mjs new file mode 100644 index 00000000000000..3ff05d907b5060 --- /dev/null +++ b/test/js/node/test/parallel/test-util-promisify-custom-names.mjs @@ -0,0 +1,40 @@ +import '../common/index.mjs'; +import assert from 'node:assert'; +import { promisify } from 'node:util'; + +// Test that customly promisified methods in [util.promisify.custom] +// have appropriate names + +import fs from 'node:fs'; +import readline from 'node:readline'; +import stream from 'node:stream'; +import timers from 'node:timers'; + + +assert.strictEqual( + promisify(fs.exists).name, + 'exists' +); + +assert.strictEqual( + promisify(readline.Interface.prototype.question).name, + 'question', +); + +assert.strictEqual( + promisify(stream.finished).name, + 'finished' +); +assert.strictEqual( + promisify(stream.pipeline).name, + 'pipeline' +); + +assert.strictEqual( + promisify(timers.setImmediate).name, + 'setImmediate' +); +assert.strictEqual( + promisify(timers.setTimeout).name, + 'setTimeout' +); From 0f0bf54d59d6057cf430a93321957ef0c23c18a3 Mon Sep 17 00:00:00 2001 From: Dylan Conway Date: Fri, 2 May 2025 14:18:28 -0700 Subject: [PATCH 3/4] add comment --- test/js/node/test/parallel/test-util-promisify.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/test/js/node/test/parallel/test-util-promisify.js b/test/js/node/test/parallel/test-util-promisify.js index f0e5eceefede01..203b3dd6f88d9d 100644 --- a/test/js/node/test/parallel/test-util-promisify.js +++ b/test/js/node/test/parallel/test-util-promisify.js @@ -80,6 +80,9 @@ const stat = promisify(fs.stat); ); } +// This test is commented out because Bun does not support getting the +// internal customPromisifyArgs symbol. + // { // const firstValue = 5; // const secondValue = 17; From 39b08a6c54f64f3dfeb52cbbf846ea94d7a48ede Mon Sep 17 00:00:00 2001 From: Dylan Conway Date: Fri, 2 May 2025 14:28:58 -0700 Subject: [PATCH 4/4] fix test --- test/js/node/util/util-promisify.test.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/js/node/util/util-promisify.test.js b/test/js/node/util/util-promisify.test.js index bc2b5c54aec536..34958be4299f2f 100644 --- a/test/js/node/util/util-promisify.test.js +++ b/test/js/node/util/util-promisify.test.js @@ -24,7 +24,7 @@ import fs from "node:fs"; // TODO: vm module not implemented by bun yet // import vm from 'node:vm'; import assert from "assert"; -import { promisify } from "util"; +import { promisify, inspect } from "util"; const stat = promisify(fs.stat); @@ -294,7 +294,7 @@ describe("util.promisify", () => { [undefined, null, true, 0, "str", {}, [], Symbol()].forEach(input => { expect(() => { promisify(input); - }).toThrow('The "original" argument must be of type Function'); + }).toThrow('The "original" argument must be of type function.' + invalidArgTypeHelper(input)); }); }); });