0

I'm trying to implement Promise Class with JavaScript (currently only with resolve, no reject).
For some reason, when I use it without setTimeout around the resolve function, I get double console.logs from the 'then' methods , and otherwise it works fine.

That's my code

const STATUSES = {
  PENDING: 'PENDING',
  RESOLVED: 'RESOLVED',
  REJECTED: 'REJECTED',
}

class CustomPromise {
  #value = null
  #status = STATUSES.PENDING
  #thenCallbacks = []

  constructor(cb) {
    cb(this.#resolve)
  }

  #updateState = (status, value) => {
    this.#status = status
    this.#value = value
  }

  #resolve = (value) => {
    this.#updateState(STATUSES.RESOLVED, value)
    this.#thenCallbacks.reduce((acc, callback) => callback(acc), this.#value)
  }

  then = (cb) => {
    this.#thenCallbacks.push(cb)
    if (this.#status === STATUSES.RESOLVED) {
      this.#thenCallbacks.reduce((acc, callback) => callback(acc), this.#value)
    }

    return this
  }
}


new CustomPromise((resolve) => {
    resolve(1)
  })
  .then((value) => {
    console.log('first value', value)
    return 2
  })
  .then((value) => {
    console.log('second value', value)
    return null
  })

the result:

enter image description here

Wrapping the 'resolve' like so makes the code run as expected:

setTimeout(() => {
      resolve(1)
     }, [1000])

thanks!

Barmar
  • 741,623
  • 53
  • 500
  • 612
rio
  • 757
  • 5
  • 19
  • 32
  • 2
    [1000]?? Really? – Jaromanda X Sep 19 '22 at 14:30
  • 1
    You should remove the callbacks from the array when you execute them ... when the second one is added bothare executed again – Jaromanda X Sep 19 '22 at 14:32
  • 2
    By the way, this implementation does not allow promise chaining – Jaromanda X Sep 19 '22 at 14:35
  • 1
    `.then()` isn't supposed to return the same promise it was called on, it's supposed to return a new promise. So the promise doesn't need an array of callbacks. – Barmar Sep 19 '22 at 14:42
  • @JaromandaXwhy not? how would you implement it? – rio Sep 19 '22 at 15:07
  • @Barmar are you sure? `p.then(f); p.then(g)` both `.then` mutate `p` and `f` and `g` are both called when `p` resolves. `.catch` needs the same capability. – Mulan Sep 19 '22 at 15:15
  • @Mulan I did a simple test in the Chrome console. `newPromise = p.then(something); p == newPromise` and it returned `false`. – Barmar Sep 19 '22 at 15:19
  • @Mulan Yes, Barmar is right. You can easily test yourself that `p.then(f) !== p`. There is no "mutation" of the result value, all that `then` does is to register handlers. It creates a new promise and returns that. Both `f` and `g` are called with the same original result value of `p`, not something else. To chain, you use `p.then(f).then(g)`, which involves 3 separate promises. – Bergi Sep 19 '22 at 15:19
  • @Bergi thanks guys, I had never actually considered that `then` returns a new value. The Promise api is full of surprises :D – Mulan Sep 19 '22 at 15:22
  • @Mulan Immutability is king. You can't change the result of a promise, neither by resolving it multiple times nor by calling `.then` on it – Bergi Sep 19 '22 at 15:25

1 Answers1

1

The problem is in the then method: when you find that the promise already resolved, you should only call the newly provided callback -- not the others. In this case the other callbacks would already have been called.

Some other comments:

  • There is no accumulation like you suggest with reduce. All callbacks should be treated equally, each getting the value as argument.

  • It is not allowed for a promise to resolve more than once, so you should ignore such a request when you find that the promise status is no longer pending.

Here is a correction for the double output you got:

const STATUSES = {
  PENDING: 'PENDING',
  RESOLVED: 'RESOLVED',
  REJECTED: 'REJECTED',
}

class CustomPromise {
  #value = null;
  #status = STATUSES.PENDING;
  #thenCallbacks = [];

  constructor(cb) {
    cb(this.#resolve);
  }

  #updateState = (status, value) => {
    this.#status = status;
    this.#value = value;
  }

  #resolve = (value) => {
    if (this.#status !== STATUSES.PENDING) return; // Ignore!
    this.#updateState(STATUSES.RESOLVED, value);
    this.#thenCallbacks.forEach(callback => callback(value)); // No accumulation
  }

  then = (cb) => {
    this.#thenCallbacks.push(cb)
    if (this.#status === STATUSES.RESOLVED) {
      cb(this.#value); // Only this one. The other callbacks were already called
    }

    return this;
  }
}


new CustomPromise((resolve) => {
    resolve(1);
  })
  .then((value) => {
    console.log('first value', value);
    return 2;
  })
  .then((value) => {
    console.log('second value', value);
    return null;
  })

Note that you have much more work to do, including:

  • then callbacks should be called asynchronously.
  • then should return a new promise. Because this is not happening in your code, and a promise value cannot be changed, the output is now always the same value.
  • Promises should implement the promise resolution procedure.

You probably want to test your implementation against the Promises/A+ Compliance Test Suite. This way you can gradually improve your implementation until it passes all tests. Be warned... there is a long road ahead, but worth the experience.

To completely fix your code so it complies with Promises/A+ specification, a lot of change is required to your code. As I once did this effort of implementing a compliant implementation, I just provide you with the class version of what I provided in this answer:

class CustomPromise {
    static Deferred = class {
        constructor() {
            this.state = 'pending';
            this.value = undefined;
            this.consumers = [];
            this.promise = Object.create(CustomPromise.prototype, {
                then: { value: this.then.bind(this) }
            });
        }
        // 2.1.1.1: provide only two ways to transition
        fulfill(value) {
            if (this.state !== 'pending') return; // 2.1.2.1, 2.1.3.1: cannot transition anymore
            this.state = 'fulfilled'; // 2.1.1.1: can transition
            this.value = value; // 2.1.2.2: must have a value
            this.broadcast();
        }
        reject(reason) {
            if (this.state !== 'pending') return; // 2.1.2.1, 2.1.3.1: cannot transition anymore
            this.state = 'rejected'; // 2.1.1.1: can transition
            this.value = reason; // 2.1.3.2: must have a reason
            this.broadcast();
        }
        // A promise’s then method accepts two arguments:
        then(onFulfilled, onRejected) {
            var consumer = new CustomPromise.Deferred();
            // 2.2.1.1 ignore onFulfilled if not a function
            consumer.onFulfilled = typeof onFulfilled === 'function' ? onFulfilled : null;
            // 2.2.1.2 ignore onRejected if not a function
            consumer.onRejected = typeof onRejected === 'function' ? onRejected : null;
            // 2.2.6.1, 2.2.6.2: .then() may be called multiple times on the same promise
            this.consumers.push(consumer);
            // It might be that the promise was already resolved... 
            this.broadcast();
            // 2.2.7: .then() must return a promise
            return consumer.promise;
        }
        broadcast() {
            var promise = this;
            // 2.2.2.1, 2.2.2.2, 2.2.3.1, 2.2.3.2 called after promise is resolved
            if (this.state === 'pending') return;
            // 2.2.6.1, 2.2.6.2 all respective callbacks must execute
            var callbackName = this.state == 'fulfilled' ? 'onFulfilled' : 'onRejected';
            var resolver = this.state == 'fulfilled' ? 'resolve' : 'reject';
            // 2.2.4 onFulfilled/onRejected must be called asynchronously
            setTimeout(function() {
                // 2.2.6.1, 2.2.6.2 traverse in order, 2.2.2.3, 2.2.3.3 called only once
                promise.consumers.splice(0).forEach(function(consumer) {
                    try {
                        var callback = consumer[callbackName];
                        // 2.2.1.1, 2.2.1.2 ignore callback if not a function, else
                        // 2.2.5 call callback as plain function without context
                        if (callback) {
                            // 2.2.7.1. execute the Promise Resolution Procedure:
                            consumer.resolve(callback(promise.value)); 
                        } else {
                            // 2.2.7.3 resolve in same way as current promise
                            consumer[resolver](promise.value);
                        }
                    } catch (e) {
                        // 2.2.7.2
                        consumer.reject(e);
                    };
                })
            });
        }
        // The Promise Resolution Procedure: will treat values that are thenables/promises
        // and will eventually call either fulfill or reject/throw.
        resolve(x) {
            var wasCalled, then;
            // 2.3.1
            if (this.promise === x) {
                throw new TypeError('Circular reference: promise value is promise itself');
            }
            // 2.3.2
            if (x instanceof CustomPromise) {
                // 2.3.2.1, 2.3.2.2, 2.3.2.3
                x.then(this.resolve.bind(this), this.reject.bind(this));
            } else if (x === Object(x)) { // 2.3.3
                try {
                    // 2.3.3.1
                    then = x.then;
                    if (typeof then === 'function') {
                        // 2.3.3.3
                        then.call(x, function resolve(y) {
                            // 2.3.3.3.3 don't allow multiple calls
                            if (wasCalled) return;
                            wasCalled = true;
                            // 2.3.3.3.1 recurse
                            this.resolve(y);
                        }.bind(this), function reject(reasonY) {
                            // 2.3.3.3.3 don't allow multiple calls
                            if (wasCalled) return;
                            wasCalled = true;
                            // 2.3.3.3.2
                            this.reject(reasonY);
                        }.bind(this));
                    } else {
                        // 2.3.3.4
                        this.fulfill(x);
                    }
                } catch(e) {
                    // 2.3.3.3.4.1 ignore if call was made
                    if (wasCalled) return;
                    // 2.3.3.2 or 2.3.3.3.4.2
                    this.reject(e);
                }
            } else {
                // 2.3.4
                this.fulfill(x);
            }
        }
    }
    constructor(executor) {
        // A Promise is just a wrapper around a Deferred, exposing only the `then`
        // method, while `resolve` and `reject` are available in the constructor callback
        var df = new CustomPromise.Deferred();
        // Provide access to the `resolve` and `reject` methods via the callback
        executor(df.resolve.bind(df), df.reject.bind(df));
        return df.promise;
    }
}

new CustomPromise((resolve) => {
    resolve(1);
  })
  .then((value) => {
    console.log('first value', value);
    return 2;
  })
  .then((value) => {
    console.log('second value', value);
    return null;
  });
trincot
  • 317,000
  • 35
  • 244
  • 286
  • I tried your code but got first and second value same values... @trincot – rio Sep 19 '22 at 16:40
  • Yes, that is because of the points I raised at the end. Your `then` method is returning the original promise, and since a promise can only resolve to one value, this is the consequence. If you want to see a completely working promise implementation, then please check the answer I have given [here](https://stackoverflow.com/questions/23772801/basic-javascript-promise-implementation-attempt/42057900#42057900). This answer has just addressed the double output problem, and fixed an issue that is not allowed in promises, and has as effect that your output is no longer as expected. – trincot Sep 19 '22 at 18:42
  • I added a promises/A+ compliant implementation to my answer. – trincot Sep 19 '22 at 19:17
  • Does this answer your question?. – trincot Sep 28 '22 at 08:03