Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a comprehensive set of utilities for converting between Promise-based and callback-based APIs in JavaScript/TypeScript. The implementation provides bidirectional conversion capabilities with options for bulk operations, making it easier to work with legacy Node.js code or support both API styles simultaneously.
Key changes:
- Adds utilities to convert callback-based functions to Promises (
promisify,fromCallback) and vice versa (callbackify,asCallback) - Provides "All" variants for bulk conversion of object methods with customizable filtering options
- Implements
withCallbackfor creating dual-mode functions that support both Promise and callback patterns
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| src/promise/promisify.ts | Converts callback-style functions to Promise-based functions |
| src/promise/promisifyAll.ts | Bulk conversion of callback methods to Promise methods with "Async" suffix |
| src/promise/fromCallback.ts | Creates Promises from callback-based operations |
| src/promise/fromCallbackAll.ts | Bulk wrapper for fromCallback pattern |
| src/promise/callbackify.ts | Converts Promise-returning functions to callback-style functions |
| src/promise/callbackifyAll.ts | Bulk conversion of async methods to callback methods with "Callback" suffix |
| src/promise/asCallback.ts | Attaches Node.js-style callbacks to existing promises, includes nodeify alias |
| src/promise/asCallbackAll.ts | Bulk wrapper for asCallback pattern |
| src/promise/withCallback.ts | Creates dual-mode functions supporting both Promise and callback patterns |
| src/promise/withCallbackAll.ts | Bulk wrapper for withCallback pattern |
| src/promise/index.ts | Exports all new promise utilities and their types |
| docs/reference/promise/promisify.md | Documentation for the promisify function |
| *.spec.ts files | Comprehensive test suites for all new utilities |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| fn.apply(context, params) | ||
| .then(val => callback(null, val)) | ||
| .catch((err: unknown) => callback(err instanceof Error ? err : new Error(String(err)), undefined as Result)); |
There was a problem hiding this comment.
Calling callbackify on a non-Error value passes undefined as Result to the callback, which may cause type safety issues. While TypeScript allows this cast, the callback signature expects a Result type as the second parameter. Consider using a more type-safe approach or documenting this behavior clearly, as it could lead to runtime issues if the callback doesn't handle undefined properly.
| const lastArg = args[args.length - 1]; | ||
|
|
||
| if (typeof lastArg === 'function') { |
There was a problem hiding this comment.
The withCallback function uses typeof lastArg === 'function' to detect if a callback was provided. This approach has a potential issue: if the original function legitimately accepts a function as its last parameter (not as a callback), this detection logic will incorrectly treat it as a callback and change the behavior. Consider adding a more robust detection mechanism, such as checking the function's length/arity, or documenting this limitation clearly in the JSDoc.
| * | ||
| * @template Result - The type of the result value on success. | ||
| */ | ||
| export type NodeCallback<Result> = (err: Error | null, result: Result) => void; |
There was a problem hiding this comment.
There's an inconsistency in callback type naming. The callbackify module uses NodeCallback<Result> while other modules like asCallback and withCallback use NodeStyleCallback<Result> for the same purpose (both are (err: Error | null, result: Result) => void). This inconsistency can be confusing for users. Consider standardizing on a single type name across all modules, preferably NodeStyleCallback which is more descriptive.
| export function asCallback<Result>( | ||
| promise: Promise<Result>, | ||
| callback: NodeStyleCallback<Result> | ||
| ): Promise<Result> { |
There was a problem hiding this comment.
The asCallback function signature declares a parameter callback but the JSDoc references an options parameter that doesn't exist. The function should either accept an AsCallbackOptions parameter or the AsCallbackOptions interface should be removed since it's not used.
| const callback = args.pop() as NodeStyleCallback<Result>; | ||
| const promise = fn.apply(this, args as unknown as Args); |
There was a problem hiding this comment.
The dualMode function mutates the arguments array with args.pop(). This can lead to unexpected behavior if the same args array is reused or if the caller expects the args to remain unchanged. Consider using array slicing instead to avoid mutation, for example: const callback = lastArg; const fnArgs = args.slice(0, -1);
| const callback = args.pop() as NodeStyleCallback<Result>; | |
| const promise = fn.apply(this, args as unknown as Args); | |
| const callback = lastArg as NodeStyleCallback<Result>; | |
| const fnArgs = args.slice(0, -1) as unknown as Args; | |
| const promise = fn.apply(this, fnArgs); |
| }, | ||
| err => { | ||
| const error = err instanceof Error ? err : new Error(String(err)); | ||
| queueMicrotask(() => callback(error, undefined as Result)); |
There was a problem hiding this comment.
Similar issue: passing undefined as Result to the callback on error may cause type safety issues. The callback signature expects a Result type as the second parameter, but undefined is being forced through a type assertion. This could lead to runtime issues if callbacks don't properly handle undefined values.
| import { describe, it, expect } from 'vitest'; | ||
| import { fromCallback } from './fromCallback'; | ||
|
|
||
| describe('fromCallback', () => { | ||
| it('resolves with result on success', async () => { | ||
| const result = await fromCallback<string>(cb => cb(null, 'ok')); | ||
| expect(result).toBe('ok'); | ||
| }); | ||
|
|
||
| it('rejects with error on failure', async () => { | ||
| const error = new Error('test error'); | ||
| await expect(fromCallback(cb => cb(error, null))).rejects.toThrow('test error'); | ||
| }); | ||
|
|
||
| it('works with async callbacks', async () => { | ||
| const result = await fromCallback<number>(cb => { | ||
| setTimeout(() => cb(null, 42), 10); | ||
| }); | ||
| expect(result).toBe(42); | ||
| }); | ||
|
|
||
| it('properly types the result', async () => { | ||
| interface User { | ||
| name: string; | ||
| age: number; | ||
| } | ||
| const user = await fromCallback<User>(cb => cb(null, { name: 'John', age: 30 })); | ||
| expect(user.name).toBe('John'); | ||
| expect(user.age).toBe(30); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The test suite for fromCallback is missing a test case for handling non-Error rejections (e.g., when a callback is invoked with a non-Error value like a string). Other similar functions like callbackify and asCallback have tests for this scenario. Consider adding a test to ensure fromCallback properly handles cases where the callback receives non-Error types as the error parameter.
| * | ||
| * @template Result - The type of the result value on success. | ||
| */ | ||
| export type Callback<Result> = (err: Error | null, result: Result) => void; |
There was a problem hiding this comment.
Inconsistent callback type naming across modules. The same callback type (err: Error | null, result: Result) => void is named differently in different files: Callback in promisify.ts, NodeCallback in callbackify.ts, and NodeStyleCallback in asCallback.ts. This creates confusion and makes the API harder to learn. Consider standardizing on a single name across all modules, preferably NodeStyleCallback as it's the most descriptive.
| /** | ||
| * Options for the asCallback function. | ||
| */ | ||
| export interface AsCallbackOptions { | ||
| /** | ||
| * If true, errors thrown in the callback won't be re-thrown. | ||
| * @default false | ||
| */ | ||
| suppressErrors?: boolean; | ||
| } |
There was a problem hiding this comment.
The AsCallbackOptions interface defines a suppressErrors option, but this option is never used in the asCallback function implementation. The option should either be implemented or removed from the interface to avoid confusing users who might try to use this non-functional feature.
| /** | |
| * Options for the asCallback function. | |
| */ | |
| export interface AsCallbackOptions { | |
| /** | |
| * If true, errors thrown in the callback won't be re-thrown. | |
| * @default false | |
| */ | |
| suppressErrors?: boolean; | |
| } |
Fixes: #1549
This PR adds a set of utilities for converting between Promise-based and callback-based APIs. These are especially useful when working with legacy Node.js code or when you need to support both styles.
Inspired on:
http://bluebirdjs.com/docs/api/promisification.html
https://googleapis.dev/nodejs/promisify/latest/global.html
What's included
Converting callbacks to promises:
promisify(fn)- Takes a function that uses a callback and returns one that returns a PromisepromisifyAll(obj)- Does the same for all methods in an object, adding*AsyncsuffixfromCallback(fn)- Creates a Promise from a one-shot callback operationfromCallbackAll(obj)- Adds*Asyncmethods for callback-based objectsConverting promises to callbacks:
callbackify(fn)- Takes an async function and returns one that accepts a callbackcallbackifyAll(obj)- Does the same for all async methods, adding*CallbacksuffixasCallback(promise, cb)- Attaches a callback to an existing promise (also exported asnodeify)asCallbackAll(obj)- Adds*Callbackmethods for async objectsSupporting both styles at once:
withCallback(fn)- Wraps an async function so it can be called either waywithCallbackAll(obj)- Does the same for all async methods in an objectWhy these exist
When maintaining libraries or working with mixed codebases, you often need to support both Promise and callback patterns. Instead of writing boilerplate for each conversion, these utilities handle it in a type-safe way.
The "All" variants are particularly useful for wrapping entire modules (like
fs) or class instances without touching each method individually.This is still a WIP and I'm still testing this.