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

Add concurrency option on forEach and map #11

Closed
wants to merge 1 commit into from

Conversation

BenjD90
Copy link

@BenjD90 BenjD90 commented Sep 14, 2018

Add possibility to limit number of concurrency requests on foreach and map

@toniov
Copy link
Owner

toniov commented Sep 15, 2018

Thanks for the PR! This is a good feature I think, but I have some concerns about the implementation:

  • You are adding the promise-pool-executor module, but I'd like to keep this library as simple as possible and dependence free at the moment. Also I took a look to the repo, and that module doesn't have been tested enough by the community maybe.
  • Instead of adding a concurrency parameter, I'd prefer to add a general options object parameter, so in the future more options can be added.
  • I don't think it's working properly. When you call pool.addEachTask all the promises are already being executed so there's not real concurrency control. Also the tests don't reflect if concurrency is working properly (I think that testing this could be kind of difficult).

@BenjD90
Copy link
Author

BenjD90 commented Sep 17, 2018

  • It's difficult to manage the promise pool I think, that's why I used another library to do that.
  • Ok for the options, but it should merge with the thisArg parameter too, no ?
  • You're completely right for the last point, I'll have to change that.

I don't have a lot of time right now to fix this 😞 I'll check that later.

@toniov
Copy link
Owner

toniov commented Sep 18, 2018

I think that maybe adding this option without changing too much code is going to be difficult.

I'm planning to release a version 2, so I think introducing concurrency at that point would be better.

@toniov
Copy link
Owner

toniov commented Nov 29, 2018

Closing this, thanks anyway :)

@toniov toniov closed this Nov 29, 2018
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