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

Question

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?

Answer 1

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));
          }
        });
    });
};

Answer 2

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);
}

Answer 3

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

Here is how I implement Promise.all()

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));