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 error on invalid concurrency #35

Merged
merged 5 commits into from
Jul 12, 2020

Conversation

sethp
Copy link
Contributor

@sethp sethp commented Jul 11, 2020

Else, passing in an invalid concurrency (in my case, because I wanted to see if -1 meant "unlimited") produces a confusing error message: both a limit is not a function at the point of use, and an UnhandledPromiseRejectionWarning from node itself.

An example:

const limit = pLimit(userSuppliedValue || someDefault);

...
.map(val => limit(() => doSomething()))
//               ^ TypeError: limit is not a function
...

sethp and others added 2 commits July 11, 2020 13:23
Else, passing in an invalid concurrency (in my case, because I wanted to see if `-1` meant "unlimited") produces a confusing error message: both a `limit is not a function` at the point of use, and an `UnhandledPromiseRejectionWarning` from node itself.
@sethp
Copy link
Contributor Author

sethp commented Jul 11, 2020

A question I had is whether it would make more sense to throw the type error straightaway rather than defer it until promise resolution time?

@sindresorhus
Copy link
Owner

A question I had is whether it would make more sense to throw the type error straightaway rather than defer it until promise resolution time?

Yes. That's better. Can you update the tests?

@sindresorhus sindresorhus changed the title fix: invalid concurrency should return a function Fix error on invalid concurrency Jul 12, 2020
@sindresorhus sindresorhus merged commit 4ab2813 into sindresorhus:master Jul 12, 2020
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