sindresorhus / p-queue

Promise queue with concurrency control
MIT License
3.45k stars 185 forks source link

Timeout documentation and questions #74

Closed EricMCornelius closed 5 years ago

EricMCornelius commented 5 years ago

Hi there:

Thanks for this project, really appreciate it, and I'm getting some excellent use out of it.

Is it intended for throwOnTimeout to default to true as declared in the README?

I notice the default is actually false in the implementation and it must be explicitly enabled.

It would likely be a rather rough breaking change for some people to modify that to an opt-out instead of opt-in, so I'm guessing the documentation should be updated instead?

Also, I'm not clear how the "timeout per operation functionality is supposed to function - is it intended to be settable at queue time, e.g. schedule.add(() => <promise>, {timeout: <timeout>})

I believe there's likely something broken in the option overrides if that's the case, as currently it doesn't seem to have any impact whatsoever. It looks like a fairly easy change to introduce though.

EricMCornelius commented 5 years ago

For the second question, I believe if the intended implementation is to allow promise execution timeout overrides, we need to check against the optional options overrides passed in here:

https://github.com/sindresorhus/p-queue/blob/master/source/index.ts#L218

And not just the queue construction options as is currently happening. That said, I'm not entirely clear if that was the intention.

sindresorhus commented 5 years ago

@ltetzlaff Was your intention to have throwOnTimeout true or false by default?

ltetzlaff commented 5 years ago

false, as in the code: https://github.com/sindresorhus/p-queue/blob/master/source/index.ts#L90.

It's wrong in the docs: https://github.com/sindresorhus/p-queue/blame/master/readme.md#L70, I made that mistake, thanks for figuring out ✌️