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

.addAll() should return a promise #6

Closed
mawrkus opened this issue Oct 20, 2016 · 2 comments
Closed

.addAll() should return a promise #6

mawrkus opened this issue Oct 20, 2016 · 2 comments

Comments

@mawrkus
Copy link
Contributor

mawrkus commented Oct 20, 2016

Hi,
First of all, thx for the library! I'm using it for a web scraper pet project...
A small detail, for the sake of consistency/convenience, I think that .addAll() should return a promise:

const oneTwoThree = promiseThrottle.addAll([
  myFunction.bind(this, 1),
  myFunction.bind(this, 2),
  myFunction.bind(this, 3)
]);

oneTwoThree.then(r => console.log("Promises " + r.join(", ") + " done"));

This should do the trick:

PromiseThrottle.prototype.addAll = function (promises) {
  var addedPromises = promises.map(function(promise) {
    return this.add(promise);
  }.bind(this));

  return Promise.all(addedPromises);
};

I can submit a PR if you agree on the idea.

@JMPerez
Copy link
Owner

JMPerez commented Oct 20, 2016

@mawrkus Thanks for the feedback! I think it makes completely sense. Feel free to send a PR or I can make the change myself in a few days.

@JMPerez
Copy link
Owner

JMPerez commented Oct 22, 2016

Fixed through #7

@JMPerez JMPerez closed this as completed Oct 22, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants