rxaviers / async-pool

Run multiple promise-returning & async functions with limited concurrency using native ES6/ES7
MIT License
798 stars 52 forks source link

don't throw on empty array #12

Closed leviwheatcroft closed 4 years ago

leviwheatcroft commented 4 years ago

yaassertion throws an error when the array parameter is not an array with at least 1 item.

I think it's a fairly common convention in nodejs that iterators simply do nothing when called with an empty array or null.

leviwheatcroft commented 4 years ago

Actually, looking into this a little more the package behaves differently depending on NODE_ENV which I think is unexpected for a sub module.

For example, when I ran into this error I tried to write a failing test, and I couldn't get it to throw an error because my tests are run with NODE_ENV="test", so in my case it's pretty much not possible to write a failing test without reconfiguring my test suite just for this one minor issue.

If you feel this behaviour is appropriate, you should definitely include it in the readme.

I think the best way to avoid unexpected behaviour is to avoid changing behaviour based on NODE_ENV.

rxaviers commented 4 years ago

The assertions are meant to help during development (development only) by throwing exceptions on maluse instead of doing best effort to make it work. On the other environments, it should make best effort to make it work.

That being said, I don't recall why there's this check Parameterarraymust have at least one item. I agree it makes sense to support empty array. Can you submit a PR with a fix? Thank you

leviwheatcroft commented 4 years ago

Is the assertion during development thing a common convention ? I'm an amateur, but I've never encountered this before. I guess I'm just wondering whether you think it's worth a mention in the readme.

rxaviers commented 4 years ago

No. We see that in some libraries (e.g., react.js), but I'd say the common convention is typed JS solutions like TypeScript or Flow.

radicand commented 4 years ago

Now that this has been merged, can an updated package be published?