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

Add support for request cancellation using AbortController #47

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

melwyn95
Copy link

As per #11 added support for request cancellation using the abort controller exposed as CancelToken.

Added test for the same.

@developit I don't know if this the correct way to do this, please let me know what to if I'm wrong

@googlebot
Copy link
Collaborator

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@melwyn95
Copy link
Author

@googlebot I signed it!

@googlebot
Copy link
Collaborator

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

Copy link
Owner

@developit developit left a comment

Choose a reason for hiding this comment

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

Perfect!!

@n3tr
Copy link

n3tr commented Jul 24, 2020

Do you think it is better to add support cancelToken API similar to axios cancel token instead of passing abortController signal directly?

Example from axios

const CancelToken = axios.CancelToken;
const source = CancelToken.source();

axios.get('/user/12345', {
  cancelToken: source.token
}).catch(function (thrown) {
  if (axios.isCancel(thrown)) {
    console.log('Request canceled', thrown.message);
  } else {
    // handle error
  }
});

axios.post('/user/12345', {
  name: 'new name'
}, {
  cancelToken: source.token
})

// cancel the request (the message parameter is optional)
source.cancel('Operation canceled by the user.');

We can create a CancelToken function that wrap around the AbortController and return signal as a cancel token, what do you think?

@melwyn-people-interactive

@developit @n3tr To support the axios API I think we'll have to implement the CancelToken like this

function CancelToken(fn) {
	const ac = new AbortController();
	fn && fn(() => ac.abort());
	return ac.signal;
}
CancelToken.source = () => {
	const ac = new AbortController();
	return {
		token: ac.signal,
		cancel: () => ac.abort()
	};
};

By implementing CancelToken this we can support both the use-cases of cancel token
Use Case 1

const CancelToken = axios.CancelToken;
let cancel;

axios.get('/user/12345', {
  cancelToken: new CancelToken(function executor(c) {
    // An executor function receives a cancel function as a parameter
    cancel = c;
  })
});

// cancel the request
cancel();

Use Case 2

const CancelToken = axios.CancelToken;
const source = CancelToken.source();

axios.get('/user/12345', {
  cancelToken: source.token
}).catch(function (thrown) {
  if (axios.isCancel(thrown)) {
    console.log('Request canceled', thrown.message);
  } else {
    // handle error
  }
});

axios.post('/user/12345', {
  name: 'new name'
}, {
  cancelToken: source.token
})

// cancel the request (the message parameter is optional)
source.cancel('Operation canceled by the user.');

I have tested this locally So if you want I can push this implementation of CancelToken

@developit
Copy link
Owner

Looks good. I will have to golf it down a lot, but it's nice to see what the API is supposed to be.

@melwyn95
Copy link
Author

melwyn95 commented Jul 25, 2020

@developit I pushed the changes, thought a lot about golfing it down, but no breakthrough...

@melwyn95 melwyn95 closed this Aug 30, 2020
@melwyn95 melwyn95 reopened this Aug 30, 2020
@melwyn95
Copy link
Author

Merged master and fixed conflicts

@developit
Copy link
Owner

Thanks for putting in the effort to keep this moving forward @melwyn95, I'll take another look!

@davie-robertson
Copy link

Some great work here - will this be merged into main branch soon?

@donlion
Copy link
Contributor

donlion commented Sep 1, 2022

@developit Any idea when this will be merged? Or is there something we can help with in order to get this ready for merge?

quicoto added a commit to quicoto/redaxios that referenced this pull request Jan 24, 2023
@SergkeiM
Copy link

Hi @developit @melwyn95

Any plans to merge this?

@Sreejit7
Copy link

Sreejit7 commented Oct 9, 2023

@developit @melwyn95 bumping this

1 similar comment
@SergkeiM
Copy link

SergkeiM commented Feb 9, 2024

@developit @melwyn95 bumping this

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.

10 participants