What is the problem with my implementation of Promise.all()?

user_12541277 0
Viewed 805

I am trying to write my Promise.all() function. It should repeat the functionality of the native Promise.all() method.

If there are only promises in the array, the code works correctly, probably the problem appears if the array contains something other than a promise.

Here is an example of execution:

const p1 = Promise.resolve(3);

const p2 = 1337;

const p3 = new Promise((resolve, reject) => {
  setTimeout(resolve, 10000, 'foo');
});

promiseAll([p1, p3, p3]).then(values => console.log(values)); // [3, 1337, "foo"]

Here is my code:

const promiseAll = promises => new Promise((resolve, reject) => {
  if (promises.length === 0) { resolve([]); }
  const results = [];

  let resolved = 0;

  promises.forEach(promise => {
    if (!(promise instanceof Promise)) {
      Promise.resolve(promise).then(data => results.push(data));
    }
    promise.then(result => {
      results.push(result);
      resolved += 1;
      if (resolved === promises.length) {
        resolve(results);
      }
    }, error => reject(error));
  });
});

What is the problem?

promise javascript
3 Answers

Two main problems with your implementation:

  1. Inside the forEach() method, you have a check: promise instanceof Promise. If this check is false, you call Promise.resolve() BUT then you also call promise.then(...); you probably meant to call then() method inside an else block

  2. Promise.all() maintains order - your implementation doesn't. This is because you just push the promise result in to the results array in the order in which the promises are resolved

You can change your implementation as shown below to fix the above mentioned problems:

const promiseAll = promises => {
    return new Promise((resolve, reject) => {
        if (promises.length === 0) {
          resolve([]);
          return;
        }

        const results = [];
        let resolved = 0;

        function collectResult(result, index) {
          results[index] = result;
          resolved += 1;
          if (resolved === promises.length) {
            resolve(results);
          }
        }

        promises.forEach((value, index) => {
          if (
            typeof value === 'object' &&
            'then' in value &&
            typeof value.then === 'function'
          ) {
            value.then(res => collectResult(res, index)).catch(reject);
          } else {
            Promise.resolve(value).then(res => collectResult(res, index));
          }
        });
    });
};
user_60943480

EDIT: following @Ivan Kleshnin comment here is the parallel solution according to the spec:

Promise.all = async function (array) {
    const results = [];

     parllalLoopFinish = () =>{
        return results.length === array.length;
    }

    return new Promise((resolve,reject) =>{
        for (let index = 0; index < array.length; index++) {
            const promise = array[index];
            if(promise instanceof Promise){
                promise.then((value)=>{
                    results[index]=value;
                    if(parllalLoopFinish()) {
                        resolve(results);
                      }
                }).catch(reject)
            }
            else{
                results[index]=promise;
            }
        }
        if(parllalLoopFinish()) {
            resolve(results);
          }
    });

  };
  
  
  const promise1 = Promise.resolve(3);
  const promise2 = 42;
  const promise3 = new Promise((resolve, reject) => {
    setTimeout(resolve, 100, "foo");
  });
  
  Promise.all([promise1, promise2, promise3]).then((res) => console.log(res));

Because you are trying to invoke then() on p2 which is a number.

Here is how I implement Promise.all()

EDIT: old iterative solution

Promise.all = async function (array) {
  const results = [];
  for (let index = 0; index < array.length; index++) {
      try {
        const value = await Promise.resolve(array[index]);
        results.push(value);
      } catch (error) {
        return Promise.reject(error);
      }
  }
  return Promise.resolve(results);
};


const promise1 = Promise.resolve(3);
const promise2 = 42;
const promise3 = new Promise((resolve, reject) => {
  setTimeout(resolve, 100, "foo");
});

Promise.all([promise1, promise2, promise3]).then((res) => console.log(res)); 
// print [3, 42, 'foo']
user_48664650

You are creating a new promise with Promise.resolve(promise) but you don't change the original promise var. Do

if (!(promise instanceof Promise)) {
  promise = Promise.resolve(promise);
}
user_88159260
Related