Skip to content

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

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.

Sign up for GitHub

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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
+63 −10
Open

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

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
9dcb557
Fix timeout-dependent return types of .add()
lukehorvat Jun 21, 2024
c1d076b
Fix timeout-dependent return types of .addAll()
lukehorvat Jun 21, 2024
16791ca
Add clarity to readme about timeout default value
lukehorvat Jun 21, 2024
0d8fcb6
Remove completed todo
lukehorvat Jun 21, 2024
520dc1b
Warn in console when timeout=undefined and throwOnTimeout=true
lukehorvat Jun 21, 2024
8f20196
Revert todo removal
lukehorvat Jun 22, 2024
1dc9294
Further clarify timeout void behavior in readme
lukehorvat Jun 22, 2024
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
5 changes: 3 additions & 2 deletions readme.md
View file
Open in desktop
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,7 @@ 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. This option only has an effect when `timeout` is defined.

##### autoStart

Expand Down
36 changes: 28 additions & 8 deletions source/index.ts
View file
Open in desktop
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 @@ -50,7 +50,6 @@ export default class PQueue<QueueType extends Queue<RunFunction, EnqueueOptionsT
*/
timeout?: number;

// TODO: The `throwOnTimeout` option should affect the return types of `add()` and `addAll()`
Copy link
Author

Choose a reason for hiding this comment

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

Damn, as soon as I submitted the PR I realized we can't remove this just yet. The options type passed to the constructor will need to be "remembered" so we can reuse it when determining the return types for add and addAll. Because you could specify throwOnTimeout=true in the constructor and then only timeout=number when calling add. I'll see if I can figure out a solution...

Copy link
Author

Choose a reason for hiding this comment

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

Okay, I couldn't find a solution so I added the todo back. I've set up this simplified example in TS Playground that features my best attempt. I also tried adding a second generic type parameter to add but that didn't work either. It's unclear to me if I'm lacking the TS knowledge or if we are brushing up against the limits of TS. Maybe someone else is able to solve this; should I open a separate issue for this todo?

I still think there's value in this PR though, since it at least corrects the types for the add method when looked at in isolation. It just won't respect the options passed earlier to the constructor, which was already the case before this PR. However the void will now surface differently to before this PR which might come as a surprise to people defining constructor options:

const queue = new PQueue({throwOnTimeout: true});
const a: number = await queue.add(async () => 1, {timeout: 1});
// ❌ return type is `number | void`.
const queue = new PQueue({timeout: 1, throwOnTimeout: false});
const a: number | void = await queue.add(async () => 1);
// ❌ return type is `number`. there's no type error either because it's a subtype of `number | void`

So I guess it's a question of where you prefer the void type surprise to happen? Personally I think it's better to have add be correct in isolation and let the constructor options people deal with forcing the type to be void. But I can understand if you prefer add to incorrectly return void if that is a "safer" type surprise for people to deal with. Either way it's a mess!

constructor(options?: Options<QueueType, EnqueueOptionsType>) {
super();

Expand Down Expand Up @@ -231,15 +230,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 +303,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
View file
Open in desktop
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
View file
Open in desktop
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}));