es6-promise-pool icon indicating copy to clipboard operation
es6-promise-pool copied to clipboard

Follow es6 promise.all

Open mwillbanks opened this issue 8 years ago • 2 comments

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;

mwillbanks avatar Nov 17 '17 04:11 mwillbanks

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 avatar Nov 17 '17 11:11 timdp

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".

timdp avatar Nov 17 '17 11:11 timdp