1

I'm updating some old code (not mine originally) that uses Bluebird promises. I'd rather use native ES6 Promises instead, but the old code uses a function Promise doesn't have, one to check if promises have been settled.

This is related to a similar question (Is there a way to tell if an ES6 promise is fulfilled/rejected/resolved?), but the solution given there is very different, so I wanted to know if the following code is a reasonable approach to the problem.

export class QueryablePromise<T> extends Promise<T> {
  private _isRejected = false;
  private _isResolved = false;
  private _isSettled = false;

  then<TResult1 = T, TResult2 = never>(
    onResolved?: ((value: T) => TResult1 | PromiseLike<TResult1>) | undefined | null,
    onRejected?: ((reason: any) => TResult2 | PromiseLike<TResult2>) | undefined | null
  ): Promise<TResult1 | TResult2> {
    const newResolved = onResolved && ((value: T): TResult1 | PromiseLike<TResult1> => {
      this._isResolved = true;
      this._isSettled = true;
      return onResolved(value);
    });
    const newRejected = onRejected && ((reason: any): TResult2 | PromiseLike<TResult2> => {
      this._isRejected = true;
      this._isSettled = true;
      return onRejected(reason);
    });

    return super.then(newResolved, newRejected);
  }

  catch<TResult = never>(
    onRejected?: ((reason: any) => TResult | PromiseLike<TResult>) | undefined | null
  ): Promise<T | TResult> {
    const newRejected = onRejected && ((reason: any): TResult | PromiseLike<TResult> => {
      this._isRejected = true;
      this._isSettled = true;
      return onRejected(reason);
    });

    return super.catch(newRejected);
  }

  finally(
    onFinally?: (() => void) | undefined | null
  ): Promise<T> {
    const newFinally = onFinally && ((): void => {
      this._isSettled = true;
      return onFinally();
    });

    return super.finally(newFinally);
  }

  get isRejected(): boolean { return this._isRejected; }
  get isResolved(): boolean { return this._isResolved; }
  get isSettled(): boolean { return this._isSettled; }
}

Basically I'm wrapping each callback passed to then, catch, and finally in another function which sets the appropriate flags and then calls the original callback. Flags could easily be set redundantly many times this way, but I don't see that as being much of a problem.

I tried to think of a way to solve this problem using the constructor for my promise subclass, and somehow put a wrapper around the original executor argument to intercept invocations of resolve and reject, but couldn't quite put my head around a way to implement that type of solution.

I also tried simply adding my own separate then, catch, and finally callbacks in a constructor for this subclass, with each having nothing to do but set my status flags, but oddly enough that resulted in a stack overflow, the constructor being called recursively until it blew up.

Guilherme Lopes
  • 4,688
  • 1
  • 19
  • 42
kshetline
  • 12,547
  • 4
  • 37
  • 73
  • Hi, trying to evaluate the state of a Promise synchronously has come up time and time again. And at least when I was searching for the solution, was because the whole design I had around Promises was wrong. See this post for more info: https://softwareengineering.stackexchange.com/questions/309511/is-synchronously-inspecting-a-promise-an-anti-pattern – Guilherme Lopes Dec 23 '21 at 02:05
  • I haven't found any need to test the state of promises that I've created in my own code, and there are obviously plenty of ways to avoid the need to do that, but in this case I'm trying to stick closer to the approach of the original code I'm modifying. – kshetline Dec 23 '21 at 02:14
  • "*If the following code is a reasonable approach to the problem.*" - no. The proper solution is to rewrite the code that uses the promise to not synchronously inspect its state. Why do you think you need to do that? – Bergi Dec 23 '21 at 08:20
  • "*I also tried simply adding my own separate `then` callbacks in a constructor for this subclass, but oddly enough that resulted in a stack overflow*" - this is because `.then()` constructs a new promise of the same subclass – Bergi Dec 23 '21 at 08:30

1 Answers1

0

This code doesn't seem reliable as presented. You need to remember that Promises resolve automatically when created.

With the code above, this simple test would fail:

const a = new QueryablePromise((res, rej) => {
    setTimeout(() => { console.log('resolved'); res()}, 100)
});

setTimeout(() => {
console.log(a.isSettled)
}, 500);

Will output

// resolved
// false

All cases need to be considered, not only when then, catch or finally are called. I would probably override the constructor and wrap the resolve and reject functions like this:

class QueryablePromise<T> extends Promise<T> {
  private _isRejected: boolean | undefined;
  private _isResolved: boolean | undefined;
  private _isSettled: boolean | undefined;

  constructor (fn: (res: (value: T | PromiseLike<T>) => void, rej: (reason?: any) => void) => void) {
    super((resolve, reject) => fn(
      value => {
        resolve(value)
        if (value instanceof Promise || typeof value === 'object' && typeof (value as any).then === 'function') {
          (value as any).then(() => {
            this._isResolved = true;
            this._isSettled = true;    
          });
        } else {
          this._isResolved = true;
          this._isSettled = true;
        }
      },
      reason => {
        reject(reason)
        this._isRejected = true;
        this._isSettled = true;
      },
    ));

    this._isRejected = false;
    this._isResolved = false;
    this._isSettled = false;
  }

  get isRejected(): boolean { return this._isRejected!; }
  get isResolved(): boolean { return this._isResolved!; }
  get isSettled(): boolean { return this._isSettled!; }
}

Guilherme Lopes
  • 4,688
  • 1
  • 19
  • 42
  • Thanks! I didn't see any problem as you described with the promise automatically resolving, so I don't think my original code was buggy, but this approach of yours was what I had hoped to do in the first place. I kept getting errors for referencing `this` in the `super(...)` call. The way you did it here, however, gets around the problems I was having. Good stuff! – kshetline Dec 23 '21 at 04:02
  • Odd that you couldn't see the `isSettled` being false without calling `then`. But I'm glad the code worked for you. :) – Guilherme Lopes Dec 23 '21 at 04:06
  • This solution is still wrong. `QueryablePromise.resolve(new Promise()).isSettled` would return `true` although the promise is still pending. – Bergi Dec 23 '21 at 08:23
  • @Bergi I updated the class to cover for `thenable` values when resolving the Promise. A few adjustments might be needed to cover specific scenarios, however, if a throughout solution that covers all scenarios is needed, it might be better to stick with BluebirdJS. – Guilherme Lopes Dec 23 '21 at 18:33