diff --git a/cli/tests/unit_node/util_test.ts b/cli/tests/unit_node/util_test.ts index 2e2bb00210..a88128ee93 100644 --- a/cli/tests/unit_node/util_test.ts +++ b/cli/tests/unit_node/util_test.ts @@ -292,3 +292,26 @@ Deno.test({ fn(); }, }); + +Deno.test({ + name: "[util] callbackify() works", + fn() { + const fn = util.callbackify(() => Promise.resolve("foo")); + fn((err, value) => { + assert(err === null); + assert(value === "foo"); + }); + }, +}); + +Deno.test({ + name: "[util] callbackify(undefined) throws", + fn() { + assertThrows( + // @ts-expect-error: testing runtime error + () => util.callbackify(undefined), + TypeError, + 'The "original" argument must be of type function', + ); + }, +}); diff --git a/ext/node/lib.rs b/ext/node/lib.rs index de56285fd6..9d92f7663c 100644 --- a/ext/node/lib.rs +++ b/ext/node/lib.rs @@ -386,7 +386,7 @@ deno_core::extension!(deno_node, "_stream.mjs", "_tls_common.ts", "_tls_wrap.ts", - "_util/_util_callbackify.ts", + "_util/_util_callbackify.js", "_util/asserts.ts", "_util/async.ts", "_util/os.ts", diff --git a/ext/node/polyfills/_util/_util_callbackify.ts b/ext/node/polyfills/_util/_util_callbackify.js similarity index 50% rename from ext/node/polyfills/_util/_util_callbackify.ts rename to ext/node/polyfills/_util/_util_callbackify.js index d6b54486d2..cb30f79150 100644 --- a/ext/node/polyfills/_util/_util_callbackify.ts +++ b/ext/node/polyfills/_util/_util_callbackify.js @@ -26,105 +26,48 @@ import { primordials } from "ext:core/mod.js"; const { ArrayPrototypePop, - ReflectApply, Error, FunctionPrototypeBind, + ReflectApply, ObjectDefineProperties, ObjectGetOwnPropertyDescriptors, + ObjectSetPrototypeOf, + ObjectValues, PromisePrototypeThen, - TypeError, } = primordials; import { nextTick } from "ext:deno_node/_next_tick.ts"; +import { validateFunction } from "ext:deno_node/internal/validators.mjs"; class NodeFalsyValueRejectionError extends Error { - public reason: unknown; - public code = "ERR_FALSY_VALUE_REJECTION"; - constructor(reason: unknown) { + code = "ERR_FALSY_VALUE_REJECTION"; + constructor(reason) { super("Promise was rejected with falsy value"); this.reason = reason; } } -class NodeInvalidArgTypeError extends TypeError { - public code = "ERR_INVALID_ARG_TYPE"; - constructor(argumentName: string) { - super(`The ${argumentName} argument must be of type function.`); - } -} -type Callback = - | ((err: Error) => void) - | ((err: null, result: ResultT) => void); +function callbackify(original) { + validateFunction(original, "original"); -function callbackify( - fn: () => PromiseLike, -): (callback: Callback) => void; -function callbackify( - fn: (arg: ArgT) => PromiseLike, -): (arg: ArgT, callback: Callback) => void; -function callbackify( - fn: (arg1: Arg1T, arg2: Arg2T) => PromiseLike, -): (arg1: Arg1T, arg2: Arg2T, callback: Callback) => void; -function callbackify( - fn: (arg1: Arg1T, arg2: Arg2T, arg3: Arg3T) => PromiseLike, -): (arg1: Arg1T, arg2: Arg2T, arg3: Arg3T, callback: Callback) => void; -function callbackify( - fn: ( - arg1: Arg1T, - arg2: Arg2T, - arg3: Arg3T, - arg4: Arg4T, - ) => PromiseLike, -): ( - arg1: Arg1T, - arg2: Arg2T, - arg3: Arg3T, - arg4: Arg4T, - callback: Callback, -) => void; -function callbackify( - fn: ( - arg1: Arg1T, - arg2: Arg2T, - arg3: Arg3T, - arg4: Arg4T, - arg5: Arg5T, - ) => PromiseLike, -): ( - arg1: Arg1T, - arg2: Arg2T, - arg3: Arg3T, - arg4: Arg4T, - arg5: Arg5T, - callback: Callback, -) => void; - -function callbackify( - original: (...args: unknown[]) => PromiseLike, -): (...args: unknown[]) => void { - if (typeof original !== "function") { - throw new NodeInvalidArgTypeError('"original"'); - } - - const callbackified = function (this: unknown, ...args: unknown[]) { + // We DO NOT return the promise as it gives the user a false sense that + // the promise is actually somehow related to the callback's execution + // and that the callback throwing will reject the promise. + function callbackified(...args) { const maybeCb = ArrayPrototypePop(args); - if (typeof maybeCb !== "function") { - throw new NodeInvalidArgTypeError("last"); - } - const cb = (...args: unknown[]) => { - ReflectApply(maybeCb, this, args); - }; + validateFunction(maybeCb, "last argument"); + const cb = FunctionPrototypeBind(maybeCb, this); + // In true node style we process the callback on `nextTick` with all the + // implications (stack, `uncaughtException`, `async_hooks`) PromisePrototypeThen( - ReflectApply(this, args), - (ret: unknown) => { - nextTick(FunctionPrototypeBind(cb, this, null, ret)); - }, - (rej: unknown) => { + ReflectApply(original, this, args), + (ret) => nextTick(cb, null, ret), + (rej) => { rej = rej || new NodeFalsyValueRejectionError(rej); - nextTick(FunctionPrototypeBind(cb, this, rej)); + return nextTick(cb, rej); }, ); - }; + } const descriptors = ObjectGetOwnPropertyDescriptors(original); // It is possible to manipulate a functions `length` or `name` property. This @@ -135,6 +78,12 @@ function callbackify( if (typeof descriptors.name.value === "string") { descriptors.name.value += "Callbackified"; } + const propertiesValues = ObjectValues(descriptors); + for (let i = 0; i < propertiesValues.length; i++) { + // We want to use null-prototype objects to not rely on globally mutable + // %Object.prototype%. + ObjectSetPrototypeOf(propertiesValues[i], null); + } ObjectDefineProperties(callbackified, descriptors); return callbackified; } diff --git a/ext/node/polyfills/util.ts b/ext/node/polyfills/util.ts index 3d449f8dcf..0c150301a4 100644 --- a/ext/node/polyfills/util.ts +++ b/ext/node/polyfills/util.ts @@ -28,7 +28,7 @@ const { } = primordials; import { promisify } from "ext:deno_node/internal/util.mjs"; -import { callbackify } from "ext:deno_node/_util/_util_callbackify.ts"; +import { callbackify } from "ext:deno_node/_util/_util_callbackify.js"; import { debuglog } from "ext:deno_node/internal/util/debuglog.ts"; import { format,