Full Promise implementation

Question

I have attempted to implement the native Promise object provided by the browser in JavaScript. The code is pretty easy to understand. It supports most of the promise functionalities, such as chaining, error handling, etc. Here is my full implementation.

const promiseStates = Object.freeze({fulfilled: "fulfilled", rejected: "rejected", pending: "pending"});
class MyPromise {
 #callbacks = [];
 #promiseResolutionValue;
 #resolveBind = this.#resolve.bind(this);
 #rejectBind = this.#reject.bind(this);
 #promiseState = promiseStates.pending;
 constructor(promiseCallback) {
 if(typeof promiseCallback !== "function") throw new TypeError(`promise resolver ${promiseCallback} is not a function`);
 try {
 promiseCallback(this.#resolveBind, this.#rejectBind);
 } catch(error) {
 this.#rejectBind(error);
 }
 }
 #resolve(value) {
 let then;
 if(this.#promiseState !== promiseStates.pending) return;
 if(value instanceof MyPromise) {
 value.then(this.#resolveBind, this.#rejectBind);
 return;
 }
 if(typeof value === "object") {
 try {
 then = value.then;
 } catch(error) {
 this.#rejectBind(error);
 return;
 }
 if(typeof then === "function") {
 then.call(value, this.#resolveBind, this.#rejectBind);
 return;
 } else {
 this.#promiseState = promiseStates.fulfilled;
 this.#promiseResolutionValue = value;
 if(this.#callbacks.length !== 0) {
 this.#runCallbacks(this.#promiseResolutionValue);
 this.#callbacks = [];
 }
 return;
 }
 }
 this.#promiseState = promiseStates.fulfilled;
 this.#promiseResolutionValue = value;
 if(this.#callbacks.length !== 0) {
 this.#runCallbacks(this.#promiseResolutionValue);
 this.#callbacks = [];
 }
 }
 #reject(reason) {
 if(this.#promiseState !== promiseStates.pending) return;
 if(reason instanceof MyPromise) {
 reason.then(this.#resolveBind, this.#rejectBind);
 return;
 }
 this.#promiseState = promiseStates.rejected;
 this.#promiseResolutionValue = reason;
 if(this.#callbacks.length !== 0) {
 this.#runCallbacks(this.#promiseResolutionValue);
 this.#callbacks = [];
 }
 }
 then(successCallback, errorCallback) {
 return new MyPromise((resolve, reject) => {
 let thenHandler;
 this.#callbacks.push(promiseResult => {
 if(this.#promiseState === promiseStates.fulfilled) {
 thenHandler = successCallback;
 if(typeof thenHandler !== "function") {
 resolve(promiseResult);
 return;
 }
 }
 if(this.#promiseState === promiseStates.rejected) {
 thenHandler = errorCallback;
 if(typeof thenHandler !== "function") {
 reject(promiseResult);
 return;
 }
 }
 try {
 const nextPromiseValue = thenHandler.call(undefined, promiseResult); // 2.2.5 of the Promise A+ spec.
 resolve(nextPromiseValue);
 } catch(error) {
 reject(error);
 }
 });
 if(this.#promiseState !== promiseStates.pending) {
 this.#runCallbacks(this.#promiseResolutionValue);
 this.#callbacks = [];
 }
 });
 }
 #runCallbacks(promiseValue) {
 this.#callbacks.forEach(callback => {
 queueMicrotask(() => {
 callback(promiseValue);
 });
 });
 }
 catch(errorCallback) {
 if(typeof errorCallback !== "function") throw `Unhandled catch error`;
 return this.then(undefined, errorCallback);
 }
 finally(callback) {
 return this.then(callback, callback);
 }
 static resolve(value) {
 return new MyPromise(resolve => {
 resolve(value);
 });
 }
 static reject(reason) {
 return new MyPromise((resolve, reject) => {
 reject(reason);
 });
 }
 static withResolvers() {
 let reject, resolve;
 return {
 promise: new MyPromise(function(res, rej) {
 reject = rej, resolve = res;
 }),
 resolve, reject,};
 }
 static all(promises) {
 const promiseResults = [];
 let promiseCount = 0;
 return new MyPromise((resolve, reject) => {
 if(promises.length === 0) {
 resolve(promises);
 return;
 }
 promises.forEach((promise, index) => {
 if(!(promise instanceof MyPromise)) {
 promiseResults[index] = promise;
 promiseCount += 1;
 if(promiseCount === promises.length) resolve(promiseResults);
 return;
 }
 promise.then(function(value) {
 promiseResults[index] = value;
 promiseCount += 1;
 if(promiseCount === promises.length) resolve(promiseResults);
 }).catch(function(e) {
 reject(e);
 });
 });
 });
 }
 static race(promises) {
 return new MyPromise((resolve, reject) => {
 if(promises.length === 0) return;
 promises.forEach(promise => {
 if(!(promise instanceof MyPromise)) {
 resolve(promise);
 return;
 }
 promise.then(function(value) {
 resolve(value);
 }).catch(function(error) {
 reject(error);
 });
 });
 });
 }
 static allSettled(promises) {
 const promiseResults = [];
 let promiseCount = 0;
 return new MyPromise((resolve, reject) => {
 promises.forEach((promise, index) => {
 if(!(promise instanceof MyPromise)) {
 promiseResults[index] = {status: "fulfilled", value: promise};
 promiseCount += 1;
 if(promiseCount === promises.length) resolve(promiseResults);
 return;
 }
 promise.then(function(value) {
 promiseResults[index] = {status: "fulfilled", value};
 promiseCount += 1;
 if(promiseCount === promises.length) resolve(promiseResults);
 }).catch(function(error) {
 promiseResults[index] = {status: "rejected", reason: error};
 promiseCount += 1;
 if(promiseCount === promises.length) resolve(promiseResults);
 });
 });
 });
 }
 static any(promises) {
 let rejectedPromisesCount = 0;
 let rejectedPromises = [];
 return new MyPromise((res, rej) => {
 promises.forEach((promise, index) => {
 promise.then(result => {
 res(result);
 }).catch(error => {
 rejectedPromisesCount += 1;
 rejectedPromises[index] = error;
 if(rejectedPromisesCount === promises.length) rej(rejectedPromises);
 });
 });
 });
 }
}

Most of my tests have passed, but I am not sure if there are certain cases that I may have mishandled. In addition, some parts of the code are rather verbose. I don't know if there is a way for me to make my code more concise and cleaner. Any helpful comments concerning my work will be appreciated.

Here are some of my tests (I don't know what automated tests are and how to create one):

const a = new MyPromise((res, rej) => {
 res("Hi");
}).then(function(result) {
 console.log(result);
}).catch(function(error) {
 console.log(`Error: ${error}`);
}); // "Hi"
const a = new MyPromise((res, rej) => {
 rej("Error");
}).then(v => console.log(v))
.catch(e => console.log(e)); // "Error"
const a = new MyPromise((res, rej) => {
 rej("Not working");
}).then(() => console.log("In first then"))
.then(() => console.log("In second then"))
.then(() => console.log("In third then"))
.catch((e) => console.log(e)); // "Not working"
const a = new MyPromise((res, rej) => {
 res("Inside the promise callback");
}).then(function(v) {
 console.log(v);
 return "First then";
}).then(v => {
 console.log(v);
 return "Second then";
}).catch(console.log); // "Inside the promise callback" "First then"
const a = new MyPromise(res => setTimeout(res, 2000, "Resolved asynchronously"))
.then(console.log); // "Resolved asynchronously"
const a = new MyPromise(res => {
 res("Resolved");
 res("Not allowed");
}).then(console.log); // "Resolved"
const a = new MyPromise(res => {
 const b = new MyPromise(res => {
 res("Inner promise resolved");
 });
 res(b);
}).then(console.log); // "Inner promise resolved"
const a = new MyPromise(res => {
 const b = new MyPromise(resolve => setTimeout(resolve, 2000, "Inner promise resolved"));
 res(b);
}).then(console.log); // "Inner promise resolved"
const a = new MyPromise((res, rej) => {
 const b = MyPromise.reject("error");
 res(b);
}).then(console.log, "in then").catch(e => {
console.log(e);
}); // "error"
const promises = [1,2,3,4];
MyPromise.race(promises).then(console.log); // 1
const promises = [MyPromise.resolve(1), 
 new MyPromise(res => setTimeout(res, 500, "Second")),
 33];
MyPromise.race(promises).then(console.log); // 33
const promises = [MyPromise.resolve(1), 
 new MyPromise(res => setTimeout(res, 500, "Second")),];
MyPromise.race(promises).then(console.log); // 1
/**
 * passing thenables to MyPromise resolver
*/
const a = new MyPromise(res => {
res({
then(a,b) {
b("Not working");
b("Hi");
a("Hi from A");
a("Suck");
}
});
}).then(console.log).catch(console.log); // "Not working"

These are some of my tests. I know that there are more that my implementation of promise have to go through.

Edit: I have also added the Promise.any static method implementation. A huge performance problem that I have found in my implementation is that in some static methods, I continuously resolving or rejecting the promise returned by that method, even though the promise has been already settled. The reason for that, I suppose, is because the forEach loop never breaks - it just continuously running the callback for every promise in the array.

• J_H: "tests have passed" Show us your automated test suite, please.
• Bergi: Try running the Promises/A+ compliance tests
• Napoleon Bonaparte: @Bergi, the tests do not work for me. It throws adapter.defer is not a function (I never implemented the defer method into my MyPromise class). Have you run the test? It seems like that all of my tests are passing (as shown in my question).
• Bergi: You are not supposed to pass the MyPromise class itself directly, you are supposed to pass an adapter object. The test suite is meant to test the .then() method requirements, it doesn't care how your library allows to construct promises. It's not a test for spec compliance with the ECMAScript Promise.
• Napoleon Bonaparte: @Bergi, then it gives me this error: Error: timeout of 200ms exceeded. Ensure the done() callback is being called in this test. It shows me that about 209 test cases failed.

Answer 1

Style

@SᴀᴍOnᴇᴌᴀ already raised some good points. In particular, I take issue with the whitespace/indentation - not sure whether some of this was caused by mixing tabs and spaces which got lost in copying the code into the question, but I suggest you use the autoformatting tool that is built into most IDEs.

Naming

#promiseState and #promiseResolutionValue should loose the promise... prefix. The term "promise" is already in the class name, no need to repeat it in every property. Same for the promiseResults and promiseCount local variables further down.

resolveBind and rejectBind should be named resolveBound/rejectBound or boundResolve/boundReject respectively: they do not bind anything, they are the bound version of the resolve and reject methods.

To nitpick, the then and catch parameters should be named onFulfilled and onRejected, which is the standard terminology in the specification and documentation. The terms "success" and "error" are generally not used for promises. Similarly, promiseCallback should be named executor.

Bound methods

I would even recommend to get rid of resolveBind/rejectBind completely, the modern style to write this is to just use an arrow function instead:

#resolve = (value) => {
 ...
}

You may even go further and remove the fields completely - the resolver functions are used exclusively within the constructor, so you may even just declare them as local variables. This may save some memory when they are no longer needed (when the promise is settled) but the promise object is kept around.

Repetitiveness around runCallback

The test if (this.#callbacks.length !== 0) and the cleanup this.#callbacks = [] are repeated around (nearly) every call to this.#runCallbacks(). Just move them inside the method! The emptiness check is also fairly superfluous, I'd get rid of it - the loop does that check anyway.

Also you are passing this.#promiseResolutionValue as the argument to all calls, never a different value. Just remove the parameter and access the property inside the method (and in each callback). This would simplify the code a lot:

#runCallbacks(promiseValue) {
 this.#callbacks.forEach(queueMicrotask);
 this.#callbacks = [];
}

This is also inconsistent with how this.#promiseState is handled - you would want the same pattern there.

It might make some sense to pass the state and resolution value to #runCallbacks if that actually was named #settle, and would also take the responsibility to store them for later. You'd still need a way to handle the callback that then adds, though that could be solved with a different method:

#addCallback(callback) {
 if (this.#promiseState !== promiseStates.pending) {
 queueMicrotask(callback);
 } else {
 this.#callbacks.push(callback);
 }
}
#settle(state, value) {
 // assert(this.#state === promiseStates.pending);
 this.#state = state;
 // assert(this.#state !== promiseStates.pending);
 this.#resolutionValue = resolutionValue;
 this.#callbacks.forEach(queueMicrotask);
 this.#callbacks = null;
}

Avoid .then(...).catch(...)

In the combinator methods all/race/allSettled/any, there are occurrences of

 promise.then(function (value) {
 resolve(value);
 }).catch(function (error) {
 reject(error);
 });

I would

1. use arrow functions instead of function expressions

2. do eta reduction by simply passing resolve/reject directly

3. use .then(..., ...) instead:

    promise.then(resolve, reject);
   

This is simpler (shorter), more efficient (creates less promise objects), and more correct - as the behaviour would actually be different if the onFulfilled callbacks were to throw an exception (which yours don't).

Correctness

Much more important than style issues, your implementation doesn't work as advertised. It is supposed to be an ES6 drop-in, but does not follow the specification.

Apart from getting an expert review, you should start with running automated tests. There is an official ECMAScript test suite at https://github.com/tc39/test262, however running those Promise tests is a bit hard as the tooling is geared towards whole runtimes. Another very useful test suite is found with the Promises/A+ compliance tests, although they test "only" the then method. This still covers most behaviour of any promise though, with great details and involved edge cases. You can run it by putting

module.exports = {
 resolved: MyPromise.resolve,
 rejected: MyPromise.reject,
 deferred: MyPromise.withResolvers,
};

in your file (making it a common.js module) and running npm exec promises-aplus-tests my-promise.js: it'll find lots of problems!

then and promise resolution procedure

Rejecting with a promise

The reject resolver function should ignore whether its reason argument is a promise or not. Drop the

if (reason instanceof MyPromise) {
 reason.then(this.#resolveBind, this.#rejectBind);
 return;
}

part from your #reject implementation.

Object type

if (typeof value === "object") {

is not a sufficient check for an object - the value might still be null! And even more annoyingly, even a callable object (a function!) might be a thenable. You need

if (typeof value === "object" && value !== null || typeof value === "function") {

or

if (Object(value) === value) {

.call(undefined)

When the spec writes "onFulfilled and onRejected must be called as functions (i.e. with no this value)", it means that they should just be called normally and not as a method. Use thenHandler(promiseResult) - which still passes no value (undefined) for the this argument.

This is a correctness issue insofar as you only tested that thenHandler is a function, not that it has a .call(...) method. (To be fair, all normal functions do. And for then.call(value, ...), where you need to pass a this argument, you rely on this method anyway - you'd need Reflect.apply or a new operator to avoid it)

Recursive resolution

You'll need to handle a promise being resolved with itself. This should not lead to an infinite recursion... Promises/A+ §2.3.1 requires rejection with a TypeError.

Error handling

You catch exceptions only from accessing the value.then property, but not from the actual then(...) call! You'll need to extend the try block around it:

try {
 const then = value.then;
 if (typeof then === "function") {
 then.call(value, this.#resolveBind, this.#rejectBind);
 } else {
 this.#settle(promiseStates.fulfilled, value);
 }
} catch (error) {
 this.#reject(error);
}

Race conditions!

Your code does not handle multiple calls to resolve/reject correctly. You do have a few checks like if (this.#promiseState !== promiseStates.pending), but that is not sufficient when your promise is being resolved with a pending promise (or thenable). It must lock in to that promise, and ignore further calls to resolve or reject, although still being pending itself until the resolution promise settles itself.

The underlying problem is that in then.call(value, this.#resolveBind, this.#rejectBind), you are passing the exact same resolver functions to the thenable's then method again. You will need to create a new set of resolver functions, which wait for the thenable, while the original set does no longer accept calls. This is typically modelled with closures instead of an extra #state.

Another sort of race condition can happen when a thenable's then method calls resolve/reject again. Afaics the Promises/A+ tests don't check this, but ES6 guards against this reentrancy by simply putting the then call itself in an extra microtask.

Other methods

constructor

When the executor argument that is passed to the constructor is not a function, it should not throw an exception but rather return a rejected promise.

catch

When the onRejected argument that is passed to the catch method is not a function, it should still be forwarded to then as-is and not cause an exception. .catch() should be equivalent to .then().

finally

A .finally(cb) call is not equivalent to .then(cb, cb). The returned promise waits for the callback result, but should still resolve to the same result value as the original promise, not to the return value of cb(). You'll need something a bit more elaborate.

Iterable arguments

The all/race/allSettled/any methods should treat their arguments as iterables, so that you can pass any object implementing that interface and not just arrays. In particular, do not use forEach but rather a for ... of loop.

This also means not using .length but counting the number of values yourself, which avoids another race condition: when the promises array is mutated after the call to your methods, your asynchronous if (promiseCount === promises.length) checks would fail.

Thenable distinction

Each of those methods also attempts to detect (non-)promise values using promise instanceof MyPromise. The correct approach would be handle arbitrary thenables as well here, not just instances of your own implementation. This can easily be done by passing every value to Promise.resolve - and it also avoids some code duplication for the fulfillment case.

Handling empty inputs

You correctly handled the edge case Promise.all([]), and similarly thought of if (promises.length === 0) return; in Promise.race([]) (which is superfluous but correct).

However you forgot about this edge case for Promise.allSettled([]) and Promise.any([]).

Error type in any

Your Promise.any method rejects the resulting promise with an array of errors when all input promises are rejected. The proper way to do this is to use an AggregateError.

Subclassing

A faithful implementation of the ECMAScript specification will also need to consider someone subclassing Promise. While in hindsight this is considered a mistake by many spec authors, you'd need to support this by using this instead of MyPromise in the static methods.

• Napoleon Bonaparte: Thanks for your review! This is very helpful. I hope this attempt at least shows some understanding of how promises works. Thank you.

Answer 2

Review

It is good that strict equality is used, as well as ES-6 features like private member variables. There are some critiques below which should hopefully improve readability.

Variable declaration could come after conditional lines

The first thing I notice is that in the #resolve definition the first line is:

 let then;

Yet the subsequent lines contain conditional return statements which have no impact on the assignment or usage of then - thus that declaration could come after those conditional lines.

Array can be truncated in a simpler way

Within the else block there is this line:

 this.#callbacks = [];

Instead of reassigning the array, one could simply set the length to 0. This empties the array.

Indentation could be more consistent

In the resolve method is this block under a conditional:

 if(this.#callbacks.length !== 0) {
 this.#runCallbacks(this.#promiseResolutionValue);
 this.#callbacks = [];
 }

Similarly in the same method are these lines:

 if(this.#callbacks.length !== 0) {
 this.#runCallbacks(this.#promiseResolutionValue);
 this.#callbacks = [];
 }

Notice how that closing bracket/brace does not line up with the if statement. Lines like these can reduce readability.

Incrementing by one can be simplified with the plus-plus operator

There are some places where counter variables are incremented using the plus-equals assignment operator - for example- in the all() method:

 promiseCount += 1;

One could simply use the postfix increment operator ++

promiseCount++;

It could also be used as a prefix operator - e.g. on the next line:

if(++promiseCount === promises.length) resolve(promiseResults);

That way the increment of promiseCount occurs before it is compared with promises.length so the line above can be removed.

Consider looping strategies for performance reasons

A huge performance problem that I have found in my implementation is that in some static methods, I continuously resolving or rejecting the promise returned by that method, even though the promise has been already settled. The reason for that, I suppose, is because the forEach loop never breaks - it just continuously running the callback for every promise in the array.

If there is a hope of focusing on performance then consider that for loops are often more performant than functional approaches like using the Array.forEach() method. One main reason for this is that methods like forEach() execute a function for each iteration, which requires adding a function to the callstack. For more information on this topic, refer to posts like this one. Not only would a for loop typically be more efficient it would also allow simpler means breaking out of the loop when necessary.

As the documentation for the forEach() method states:

There is no way to stop or break a forEach() loop other than by throwing an exception. If you need such behavior, the forEach() method is the wrong tool. ¹

One might also be tempted to use a for...of loop for simplicity but bear in mind how it works:

When a for...of loop iterates over an iterable, it first calls the iterable's \[Symbol.iterator\]() method, which returns an iterator, and then repeatedly calls the resulting iterator's next() method to produce the sequence of values to be assigned to variable.²

Hence those are often not as performant as regular for loops.

• Napoleon Bonaparte: Thanks for your review! Can you provide some reviews concerning my code performance and possible bugs?
• Sᴀᴍ Onᴇᴌᴀ: You're welcome! I've added a section near the end about performance