-
Notifications
You must be signed in to change notification settings - Fork 6
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 retry logic with exponential backoff to API client #30
Conversation
nodejs/src/index.ts
Outdated
@@ -28,7 +28,8 @@ export class API { | |||
baseURL: hackmdAPIEndpointURL, | |||
headers:{ | |||
"Content-Type": "application/json", | |||
} | |||
}, | |||
timeout: 30000, // Increased timeout for low bandwidth |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you provide as an option of APIClient options?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
APIClient options type has been expand and methods have been made options of it.
nodejs/src/index.ts
Outdated
} | ||
this.createRetryInterceptor(this.axios, 3); // Add retry interceptor with maxRetries = 3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The maxRetries should also be an option
return ( | ||
!error.response || | ||
(error.response.status >= 500 && error.response.status < 600) || | ||
error.response.status === 429 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please also check out the headers? If there are no user remaining API credits, prevent retrying.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a mechanism to check API rate limits (x-ratelimit-userlimit
, x-ratelimit-userremaining
) and trigger retries or failures based on remaining credits.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comments Addressed
nodejs/src/index.ts
Outdated
@@ -28,7 +28,8 @@ export class API { | |||
baseURL: hackmdAPIEndpointURL, | |||
headers:{ | |||
"Content-Type": "application/json", | |||
} | |||
}, | |||
timeout: 30000, // Increased timeout for low bandwidth |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
APIClient options type has been expand and methods have been made options of it.
} | ||
|
||
export class API { | ||
private axios: AxiosInstance | ||
|
||
constructor (readonly accessToken: string, public hackmdAPIEndpointURL: string = "https://api.hackmd.io/v1", public options: APIClientOptions = { wrapResponseErrors: true }) { | ||
constructor ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Constructor is easier to read this way
return ( | ||
!error.response || | ||
(error.response.status >= 500 && error.response.status < 600) || | ||
error.response.status === 429 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a mechanism to check API rate limits (x-ratelimit-userlimit
, x-ratelimit-userremaining
) and trigger retries or failures based on remaining credits.
Add retry logic with exponential backoff to API client:
#382