`onCancel` handlers should be called immediately if the promise was cancelled already about p-cancelable HOT 8 CLOSED

I think the following example demonstrates the same issue (please correct me if I'm wrong):

const promise = new pCancelable((resolve, reject, onCancel) => {
  const timeout = setTimeout(() => {
    resolve(true);
  }, 1000);

  onCancel.shouldReject(true);
  onCancel(() => {
    clearTimeout(timeout);
    resolve(false);
  });
});

promise.shouldReject = true;
promise.cancel();
promise
  .then(value => console.log(value))
  .catch(console.error);

Essentially, since resolve() is called inside the onCancel() function, the cancellation never occurs, despite .cancel() being called before the promise actually resolving.

I think we can fix this by changing the following code:

To something like this:

const onCancel = handler => {
  if (!this._isPending) {
    throw new Error('The `onCancel` handler was attached after the promise settled.');
  }
  
  // we need to wrap the handler to set `_isCancelled` before
  // the handler function is actually invoked
  const fn = (...args) => {
    this._isCanceled = true;
    handler(...args);
  };

  this._cancelHandlers.push(fn);
};

And we would change the following:

To something like:

const onResolve = value => {
  // if the promise was already cancelled, we can stop
  if (!this._isCanceled) {
    this._isPending = false;
    resolve(value);
  }
};

If this seems like an acceptable approach and it solves the problem being described, I'd be happy to do a PR.

from p-cancelable.

Hi @szmarczak, you are defining the onCancel in the setTimeout, so when you do cancelable.cancel(); it is not yet executed. Move the call outside the setTimeout to make it works:

const PCancelable = require('p-cancelable');
const cancelable = new PCancelable((resolve, reject, onCancel) => {
	onCancel(() => console.log('Cancelled')); // It will be called

	setTimeout(() => {
        	console.log('Timeout');
	}, 1000);
});

cancelable.catch(() => {});
cancelable.cancel();

from p-cancelable.

It took me a bit to understand this too.

This is akin to adding an event listener after the event was fired, e.g.

document.click();
document.addEventListener('click', console.log);

Now in p-cancelable terms:

p.cancel();
...
onCancel(console.log);

I think this could be achieved by treating onCancel callbacks as .then callbacks of a private Promise, instead of manually creating an array (_cancelHandlers) and calling them with a loop.

from p-cancelable.

However, a failing onCancel callback currently throws its own error:

And if said onCancel callback is called after (as requested by this issue), it will no longer change the error, so the behavior would be different depending on when onCancel is called.

from p-cancelable.

What do you mean by "cancelled already"? Can you provide an example of what's not working as expected?

The onCancel handlers are called synchronously when use call .cancel():

from p-cancelable.

Sorry, here's the example:

const PCancelable = require('p-cancelable');
const cancelable = new PCancelable((resolve, reject, onCancel) => {
	setTimeout(() => {
        console.log('Timeout');
		onCancel(() => console.log('Cancelled')); // Should this be called?
	}, 1000);
});

cancelable.catch(() => {});
cancelable.cancel();

from p-cancelable.

@Spy-Seth IKR. This is not what I want. What I am trying to show, is that if onCancel callback should be called if the promise was cancelled already.

from p-cancelable.

@jonschlinkert Sounds good. PR welcome 👍

from p-cancelable.