Skip to content

axios-retry added #545

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

Merged
merged 4 commits into from
Jan 18, 2023
Merged

axios-retry added #545

merged 4 commits into from
Jan 18, 2023

Conversation

himaniraghav3
Copy link
Collaborator

axios-retry added to the calls to projects-api

Copy link
Member

@eisbilir eisbilir left a comment

Choose a reason for hiding this comment

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

@himaniraghav3

  1. Passing axios object to axiosRetry adds interceptors to axios globally. So, instead of calling axiosRetry each time under helper methods, it is enough to call it for 1 time in helper.js global area. Also for example, calling axiosRetry under ensureProjectExist method doesn't make it scpecific to that method but axiosRetry will be applied to all axios calls. (Other than v5/projects)
  2. Please remove all axiosRetry calls and add it to the global area.
  3. In retry condition we should check if the called url belongs to project api; e.config.url.indexOf('v5/projects') > 0 && (axiosRetry.isNetworkOrIdempotentRequestError(e) || e.response.status === 429)
  4. Also we can add a logger to see if it was retried:
onRetry: (retryCount, error, requestConfig) => 
    logger.info(retryCount) // retry count and the called url can be logged here in a good format.
  
  1. axiosRetry.exponentialDelay's initial delay seemed a bit less to me. We can tweak their function and use the following instead:
function exponentialDelay (retryNumber = 0) {
  const delay = Math.pow(2, retryNumber) * 200
  const randomSum = delay * 0.2 * Math.random() // 0-20% of the delay
  return delay + randomSum
}
  1. Since you'll change the code now, let's revert the changes on helper.js and avoid changing lintings.
    Thanks

@eisbilir eisbilir changed the base branch from master to develop January 16, 2023 12:50
@eisbilir eisbilir changed the base branch from develop to master January 16, 2023 13:05
@himaniraghav3 himaniraghav3 removed the request for review from rakibansary January 17, 2023 14:53
@eisbilir eisbilir merged commit 2a79f57 into master Jan 18, 2023
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.

3 participants