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

Conversation

lukehorvat
Copy link

Hi. The return types of add and addAll are currently a bit funky. I think there was a regression.

For example, the following code raises a type error for b because it thinks a is number | void:

const a = await this.queue.add(async () => 1);
const b: number = a * 2;

That doesn't seem right. It should only return number | void when both timeout=number and throwOnTimeout=false|undefined. But in the example above I haven't even specified timeout, so the void behavior should never surface.

This PR fixes the return types:

  • When timeout=number and throwOnTimeout=true, return type is Thing.
  • When timeout=number and throwOnTimeout=false|undefined, return type is Thing | void.
  • When timeout=undefined and throwOnTimeout=true|false|undefined, return type is Thing.

To further emphasize the relationship between timeout and throwOnTimeout I also added a note in the readme and a warning message in the console.

source/index.ts Outdated
@@ -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!

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants