Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Immediately call onCancel handlers when a promise is canceled #24

Merged
merged 2 commits into from
Mar 10, 2021

Conversation

jonschlinkert
Copy link
Contributor

Fixes #15

index.js Outdated
@@ -46,7 +48,12 @@ class PCancelable {
throw new Error('The `onCancel` handler was attached after the promise settled.');
}

this._cancelHandlers.push(handler);
const fn = (...args) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you undo this change? A much better solution is to move the this._isCanceled = true; before the if here:

https://github.com/jonschlinkert/p-cancelable/blob/deaeee112adc3d105aaec2074de971af5dd48c3a/index.js#L90-L100

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

index.js Outdated
@@ -32,8 +32,10 @@ class PCancelable {
this._reject = reject;

const onResolve = value => {
this._isPending = false;
resolve(value);
if (this._isCanceled !== true || onCancel.shouldReject === false) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (this._isCanceled !== true || onCancel.shouldReject === false) {
if (!this._isCanceled || !onCancel.shouldReject) {

@szmarczak
Copy link
Contributor

Add a test:

test('throws immediately as soon as .cancel() is called', async t => {
	const cancelablePromise = new PCancelable((resolve, reject, onCancel) => {
	const timeout = setTimeout(() => {
		resolve(true);
	}, 10);
	
	onCancel.shouldReject = true;
	onCancel(() => {
		clearTimeout(timeout);
		resolve(false);
	  });
	});

	cancelablePromise.cancel();

	await t.throwsAsync(cancelablePromise, {
		message: 'Promise was canceled'
	});
});

@ifiokjr
Copy link

ifiokjr commented Sep 4, 2020

Is this still being worked on?

I'm happy to help out if not.

@szmarczak
Copy link
Contributor

@ifiokjr That would be great! I don't think the author is active on GitHub anymore (I just guess by looking at his contributions chart).

Base automatically changed from master to main January 24, 2021 06:15
@sindresorhus
Copy link
Owner

Friendly bump :)

@jonschlinkert
Copy link
Contributor Author

jonschlinkert commented Mar 8, 2021 via email

jonschlinkert added a commit to jonschlinkert/p-cancelable that referenced this pull request Mar 9, 2021
@sindresorhus sindresorhus changed the title Immediately call onCancel handlers when a promise is canceled. Immediately call onCancel handlers when a promise is canceled Mar 10, 2021
@sindresorhus sindresorhus merged commit d292dcc into sindresorhus:main Mar 10, 2021
@sindresorhus
Copy link
Owner

Thanks :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

onCancel handlers should be called immediately if the promise was cancelled already
4 participants