3
\$\begingroup\$

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


\$\endgroup\$
  • 1
    \$\begingroup\$ What is it supposed to do? What made you write this? How will it be used? \$\endgroup\$ – Mast Jan 9 at 9:09
  • \$\begingroup\$ I was learning about promises. It would probably not be used since it's a reimplementation of something that already exists as a built in. \$\endgroup\$ – Tobi Alafin Jan 9 at 9:31
  • \$\begingroup\$ Whoa whoa hold your horses why is resolve suddenly called fulfill? \$\endgroup\$ – FreezePhoenix Jan 9 at 12:16
  • \$\begingroup\$ I guess I'll change that. I'm still learning promises, and the answer would be that I don't understand why it's called resolve() and not fulfill() in the first place. \$\endgroup\$ – Tobi Alafin Jan 9 at 12:30
  • 2
    \$\begingroup\$ When someone suggests a change to the code in a comment, don't edit your question, just note it, and maybe make a change to your actual code \$\endgroup\$ – FreezePhoenix Jan 9 at 12:33
4
\$\begingroup\$

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.

|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ I did test the function, we probably just used it differently. It's been a while, but IIRC I went with spread because it was how I used the function. \$\endgroup\$ – Tobi Alafin Feb 12 at 12:06

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.