Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix timeout-dependent return types of add and addAll #206

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions readme.md
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,8 @@ Concurrency limit.

##### timeout

Type: `number`
Type: `number`\
Default: `undefined` (i.e. no timeout is enforced)

Per-operation timeout in milliseconds. Operations fulfill once `timeout` elapses if they haven't already.

Expand All @@ -72,7 +73,9 @@ Per-operation timeout in milliseconds. Operations fulfill once `timeout` elapses
Type: `boolean`\
Default: `false`

Whether or not a timeout is considered an exception.
Whether or not a timeout is considered an exception. When `false`, promises will resolve without a value on timeout.

Obviously, this option only has an effect when `timeout` is defined.

##### autoStart

Expand Down
35 changes: 28 additions & 7 deletions source/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import {EventEmitter} from 'eventemitter3';
import pTimeout, {TimeoutError} from 'p-timeout';
import {type Queue, type RunFunction} from './queue.js';
import PriorityQueue from './priority-queue.js';
import {type QueueAddOptions, type Options, type TaskOptions} from './options.js';
import {type QueueAddOptions, type Options, type TaskOptions, type WithoutTimeout, type WithoutTimeoutThrow, type WithTimeoutThrow} from './options.js';

type Task<TaskResultType> =
| ((options: TaskOptions) => PromiseLike<TaskResultType>)
Expand Down Expand Up @@ -231,15 +231,32 @@ export default class PQueue<QueueType extends Queue<RunFunction, EnqueueOptionsT
/**
Adds a sync or async task to the queue. Always returns a promise.
*/
async add<TaskResultType>(function_: Task<TaskResultType>, options: {throwOnTimeout: true} & Exclude<EnqueueOptionsType, 'throwOnTimeout'>): Promise<TaskResultType>;
async add<TaskResultType>(function_: Task<TaskResultType>, options?: Partial<EnqueueOptionsType>): Promise<TaskResultType | void>;
async add<TaskResultType>(function_: Task<TaskResultType>, options: Partial<EnqueueOptionsType> = {}): Promise<TaskResultType | void> {
async add<TaskResultType>(
function_: Task<TaskResultType>,
options?: WithoutTimeout<Partial<EnqueueOptionsType>>
): Promise<TaskResultType>;
async add<TaskResultType>(
function_: Task<TaskResultType>,
options: WithTimeoutThrow<Partial<EnqueueOptionsType>>
): Promise<TaskResultType>;
async add<TaskResultType>(
function_: Task<TaskResultType>,
options: WithoutTimeoutThrow<Partial<EnqueueOptionsType>>
): Promise<TaskResultType | void>;
async add<TaskResultType>(
function_: Task<TaskResultType>,
options: Partial<EnqueueOptionsType> = {},
): Promise<TaskResultType | void> {
options = {
timeout: this.timeout,
throwOnTimeout: this.#throwOnTimeout,
...options,
};

if (!options.timeout && options.throwOnTimeout) {
console.warn('You specified `throwOnTimeout=true` without defining `timeout`.');
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should throw an error here, and make the options mutually exclusive.
This means we would avoid user confusion when an option has no effect, and help future maintainers of dependants keep their code clean by explicitly flagging (as an error) when the option is unnecessary.

}

return new Promise((resolve, reject) => {
this.#queue.enqueue(async () => {
this.#pending++;
Expand Down Expand Up @@ -287,15 +304,19 @@ export default class PQueue<QueueType extends Queue<RunFunction, EnqueueOptionsT
*/
async addAll<TaskResultsType>(
functions: ReadonlyArray<Task<TaskResultsType>>,
options?: {throwOnTimeout: true} & Partial<Exclude<EnqueueOptionsType, 'throwOnTimeout'>>,
options?: WithoutTimeout<Partial<EnqueueOptionsType>>,
): Promise<TaskResultsType[]>;
async addAll<TaskResultsType>(
functions: ReadonlyArray<Task<TaskResultsType>>,
options: WithTimeoutThrow<Partial<EnqueueOptionsType>>
): Promise<TaskResultsType[]>;
async addAll<TaskResultsType>(
functions: ReadonlyArray<Task<TaskResultsType>>,
options?: Partial<EnqueueOptionsType>,
options: WithoutTimeoutThrow<Partial<EnqueueOptionsType>>,
): Promise<Array<TaskResultsType | void>>;
async addAll<TaskResultsType>(
functions: ReadonlyArray<Task<TaskResultsType>>,
options?: Partial<EnqueueOptionsType>,
options: Partial<EnqueueOptionsType> = {},
): Promise<Array<TaskResultsType | void>> {
return Promise.all(functions.map(async function_ => this.add(function_, options)));
}
Expand Down
20 changes: 20 additions & 0 deletions source/options.ts
Original file line number Diff line number Diff line change
Expand Up @@ -109,3 +109,23 @@ export type TaskOptions = {
*/
readonly signal?: AbortSignal;
};

/**
Helper type to target the case where timeout=number.
*/
export type WithTimeout<Options> = Options & {timeout: number};

/**
Helper type to target the case where timeout=undefined.
*/
export type WithoutTimeout<Options> = Omit<Options, 'timeout'> | {timeout: undefined};

/**
Helper type to target the case where timeout=number and throwOnTimeout=true.
*/
export type WithTimeoutThrow<Options> = WithTimeout<Options> & {throwOnTimeout: true};

/**
Helper type to target the case where timeout=number and throwOnTimeout=false/undefined.
*/
export type WithoutTimeoutThrow<Options> = WithTimeout<Options> & {throwOnTimeout?: false};
11 changes: 10 additions & 1 deletion test-d/index.test-d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,5 +3,14 @@ import PQueue from '../source/index.js';

const queue = new PQueue();

expectType<Promise<string | void>>(queue.add(async () => '🦄'));
expectType<Promise<string>>(queue.add(async () => '🦄'));
expectType<Promise<string>>(queue.add(async () => '🦄', {}));
expectType<Promise<string>>(queue.add(async () => '🦄', {throwOnTimeout: undefined}));
expectType<Promise<string>>(queue.add(async () => '🦄', {throwOnTimeout: false}));
expectType<Promise<string>>(queue.add(async () => '🦄', {throwOnTimeout: true}));
expectType<Promise<string>>(queue.add(async () => '🦄', {timeout: undefined}));
expectType<Promise<string>>(queue.add(async () => '🦄', {timeout: 1, throwOnTimeout: true}));
expectType<Promise<string | void>>(queue.add(async () => '🦄', {timeout: 1}));
expectType<Promise<string | void>>(queue.add(async () => '🦄', {timeout: 1, throwOnTimeout: undefined}));
expectType<Promise<string | void>>(queue.add(async () => '🦄', {timeout: 1, throwOnTimeout: false}));
expectType<Promise<string>>(queue.add(async () => '🦄', {priority: 1}));