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

Follow es6 promise.all #51

Open
mwillbanks opened this issue Nov 17, 2017 · 2 comments
Open

Follow es6 promise.all #51

mwillbanks opened this issue Nov 17, 2017 · 2 comments

Comments

@mwillbanks
Copy link

This relates to issue #7 in that the expected / desirable result would be that the results of the promises would be passed to the then function. I have not checked other parameters for compatibility as .then(data => {}) is really the most necessary use case.

Since based on issue #7 I didn't want to create a pull request unless you deemed it fit, but for anyone else that might be looking a way of doing this can be as follows:

'use strict';

const Es6PromisePool = require('es6-promise-pool');

/**
 * Promise Pool
 *
 * Extends the es6-promise-pool class to enable it to function much
 * like Promise.all() functions by returning the array of results.
 */
class PromisePool extends Es6PromisePool {
  /**
   * Constructor
   *
   * @param {Function} source - function to generate data
   * @param {Number} concurrency - amount of concurrency
   * @param {Object} options - key value pairs of options
   */
  constructor(source, concurrency, options) {
    super(source, concurrency, options);
    this.resolves = [];
  }

  /**
   * Start
   *
   * @return {Promise}
   */
  start() {
    this.addEventListener('fulfilled', (event) => {
      this.resolves.push(event.data.result);
    });

    return super.start().then(() => {
      return Promise.resolve(this.resolves);
    });
  }
}

module.exports = PromisePool;
@timdp
Copy link
Owner

timdp commented Nov 17, 2017

I considered making this behavior opt-in, or possibly opt-out. I don't like the idea of always having it enabled. One of the goals here was to have something scalable; allocating an ever-growing array would not only increase the memory footprint with the array itself, but also prevent garbage collection.

Thanks for the example code though. I might at the very least add a section to the readme about it.

@timdp
Copy link
Owner

timdp commented Nov 17, 2017

Oh, and by the way, a small code review:

  • You don't need the Promise.resolve() wrapper. You can just return super.start().then(() => this.resolves).
  • It's cleaner if you remove the event listener (upon both success and failure).
  • The spec uses "values" as the name rather than "resolves".

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

No branches or pull requests

2 participants