Implementing `Promise.all()`

Question

I took a stab at implementing Promise.all():

"use strict";

function all (...promises) {
   const promise = new Promise((resolve, reject) => {
      let counter = 0;
      const values = [];
      let rejected = false;
      promises.forEach(
         (promise, idx) => {
            promise.then(val => {
                  counter++;
                  values[idx] = val;
                  if (counter === promises.length)
                     resolve(values);
               },
               () => {
                  if (!rejected) {
                     reject(new Error(`${promise} was rejected!`));
                     rejected = true;
                  }
               }
            );
         }
      );
   });
   return promise;
};

Answer 1

Good job using const for values, as well as let for re-assignable values like counter and rejected.

Did you test this code? My presumption is that it didn't happen, because when I tried running it, I see the following error:

error promise.then is not a function

This is because the promises are spread out:

function all (...promises) {

Without the spread operator it appears to run as I would expect, as long as each entry in promises is a promise.

I must admit I compared with another implementation of Promise.all(). In comparison with that function, yours tracks rejected, whereas the other one simply calls reject whenever a promise is rejected.

Another thing I noticed is that the variable name promise is reused - both for the outer promise to be returned by all() as well as the callback to promises.forEach(). It would be wise to use different names to improve readability. In fact there is little need to assign the outer promise - it can simply be returned without being assigned to a variable since it isn't modified after being instantiated.

Another aspect to consider is that Promise.all() can accept promises or non-promises - e.g. the MDN documentation gives an example like this:

const promise1 = Promise.resolve(3);
const promise2 = 42; // <- not really a promise
const promise3 = new Promise(function(resolve, reject) {
  setTimeout(resolve, 100, 'foo');
});

Promise.all([promise1, promise2, promise3]).then(function(values) {
  console.log(values);
}).catch(error => {
   console.log('error: ', error.message); 
});
// expected output: Array [3, 42, "foo"]

With your code, it throws the error promise.then is not a function so it might be wise to check if each item is a promise before calling .then() on it.