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

feat: abortable retry delay #268

Closed
wants to merge 2 commits into from

Conversation

xpirt
Copy link

@xpirt xpirt commented Apr 1, 2024

Context

If a web application implements axios-retry and also a network change detection system, he may want to re-call the API requests even if in the middle of the retries.

Problem

Currently, there is no way to clear the setTimeout and reject the Promise that handles the retries' attempts.

Solution

Implement the possibility to pass an AbortController's signal, so that a developer is able to stop pending retries.

@mindhells
Copy link
Member

Hi @xpirt
sorry for the delay.
I have some questions/concerns about this PR:

  • changing the prettier rules should be part of a different PR, where you could explain the motivations for the change
  • it is important to cover review the tests, and add new ones if needed. Specially when adding new functionality
  • would't the axios instance cancellation mechanism work?

@xpirt
Copy link
Author

xpirt commented Apr 22, 2024

Hi @xpirt sorry for the delay. I have some questions/concerns about this PR:

  • changing the prettier rules should be part of a different PR, where you could explain the motivations for the change
  • it is important to cover review the tests, and add new ones if needed. Specially when adding new functionality
  • would't the axios instance cancellation mechanism work?
  1. I see, will revert that. Edit: reverted.
  2. Will try to come up with some tests
  3. Nope. The axios-retry retry mechanism uses a setTimeout which, despite the request being aborted, it is not cleared properly. This results into the timeout to expire and execute the request afterwards. Let me know if that's clear.

@mindhells
Copy link
Member

Hi @xpirt sorry for the delay. I have some questions/concerns about this PR:

  • changing the prettier rules should be part of a different PR, where you could explain the motivations for the change
  • it is important to cover review the tests, and add new ones if needed. Specially when adding new functionality
  • would't the axios instance cancellation mechanism work?
  1. I see, will revert that. Edit: reverted.
  2. Will try to come up with some tests
  3. Nope. The axios-retry retry mechanism uses a setTimeout which, despite the request being aborted, it is not cleared properly. This results into the timeout to expire and execute the request afterwards. Let me know if that's clear.

Is there a way we can honor the original abort signal (if any) from the axios instance instead extending this interceptor interface?

@michal-billtech
Copy link
Contributor

@mindhells @xpirt please take a look at #273. Is this what you had in mind?

@mindhells
Copy link
Member

closing this one in favor of #273

@mindhells mindhells closed this Jun 4, 2024
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.

None yet

3 participants