isCanceled is not set to true when shouldReject is set to false and promise is rejected in onCancel

[Rychu-Pawel]

I'm doing some p-cancelable testing to get familiar with it and I noticed that isCanceled is not set to true on the promise when the promise is canceled with shouldReject set to false.

I briefly took a look at the library code but TBH I don't see any reason for this to happen so reporting this issue.

My code is (I know, ugly but as said it is my test code):

import PCancelable from 'p-cancelable';

const cancelablePromise = doJob();

try {
    await sleep(500);

    cancelablePromise.cancel();
}
catch (error) {
    console.log("First main catch error: " + error);
}

try {
    const result = await cancelablePromise;
    console.log("result", result);
} catch (error) {
    console.log("Second main catch error: " + error);

    if (cancelablePromise.isCanceled)
        console.log("Don't worry it was canceled")
}

function sleep(ms) {
    return new Promise((resolve) => {
        setTimeout(resolve, ms);
    });
}

function doJob() {
    return new PCancelable((resolve, reject, onCancel) => {
        try {
            const timeout = setTimeout(() => {
                const result = job();
                resolve(result);
            }, 1000);

            onCancel.shouldReject = false;

            onCancel(() => {
                console.log('canceled');
                clearTimeout(timeout);
                reject(new Error('canceled'));
            });
        }
        catch (error) {
            console.log('Promise catch error: ' + error);
            reject(error);
        }
    });
}

Current behavior:

> node .\index.js
canceled
Second main catch error: Error: canceled

Expected behavior:

> node .\index.js
canceled
Second main catch error: CancelError: Promise was canceled
Don't worry it was canceled

If I remove onCancel.shouldReject = false; then it is fine.

[Rychu-Pawel]

Ok, I see that it is because there is a reject(new Error('canceled')); line in the onCancel handler which overwrites the state of the promise. But actually, I find it a bit confusing that the promise reports it was not canceled when it actually was.

Maybe the promise should additionally rely on a new internal variable isCanceled rather than only on the state which looks like is overwritable?

[sindresorhus]

This looks like a bug. A canceled state should be immutable.

@jopemachine Thoughts?

[jopemachine]

I think onResolve and onReject function needs to be updated like below to prevent state change after canceling.

const onReject = error => {
  if (this.#state !== promiseState.canceled || !onCancel.shouldReject) {
    reject(error);

    if (this.#state !== promiseState.canceled) {
      this.#state = promiseState.rejected;
    }
  }
};

Then, the example code result will be like the below (expected result)

canceled
Second main catch error: Error: canceled
Don't worry it was canceled

I just made a PR fixing it in this way.