-
Notifications
You must be signed in to change notification settings - Fork 33
[PB-5908]: add global retry with rate-limit notifications for SDK clients #1866
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
base: master
Are you sure you want to change the base?
Changes from all commits
330514d
3316660
8c7d129
c40281b
f56bd83
6d82edc
ef1934d
d7022fb
ebfd104
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,61 @@ | ||
| import { describe, it, expect, vi, beforeEach } from 'vitest'; | ||
| import { RetryOptions } from '@internxt/sdk/dist/shared'; | ||
| import { retryStrategies, SILENT_MAX_RETRIES, USER_NOTIFICATION_MAX_RETRIES } from './retryStrategies'; | ||
|
|
||
| const getOnRetry = (options: RetryOptions): ((attempt: number, delay: number) => void) => { | ||
| if (!options.onRetry) throw new Error('onRetry callback was not provided'); | ||
| return options.onRetry; | ||
| }; | ||
|
|
||
| describe('retryStrategies', () => { | ||
| beforeEach(() => { | ||
| vi.clearAllMocks(); | ||
| }); | ||
|
|
||
| describe('silent', () => { | ||
| it('When a silent strategy is created, then it retries up to the configured max', () => { | ||
| const options = retryStrategies.silent('Test'); | ||
|
|
||
| expect(options.maxRetries).toBe(SILENT_MAX_RETRIES); | ||
| }); | ||
|
|
||
| it('When a request is retried, then it logs a warning without notifying the user', () => { | ||
| const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}); | ||
| const notifyUser = vi.fn(); | ||
| const options = retryStrategies.silent('Test'); | ||
|
|
||
| getOnRetry(options)(1, 1000); | ||
|
|
||
| expect(warnSpy).toHaveBeenCalledWith('[SDK] Test retry attempt 1, waiting 1000ms'); | ||
| expect(notifyUser).not.toHaveBeenCalled(); | ||
| warnSpy.mockRestore(); | ||
| }); | ||
| }); | ||
|
|
||
| describe('withUserNotification', () => { | ||
| it('When a notification strategy is created, then it retries up to the configured max', () => { | ||
| const options = retryStrategies.withUserNotification('Test', vi.fn()); | ||
|
|
||
| expect(options.maxRetries).toBe(USER_NOTIFICATION_MAX_RETRIES); | ||
| }); | ||
|
|
||
| it('When a request is retried, then it calls the notifyUser callback', () => { | ||
| const notifyUser = vi.fn(); | ||
| const options = retryStrategies.withUserNotification('Test', notifyUser); | ||
|
|
||
| getOnRetry(options)(1, 1000); | ||
|
|
||
| expect(notifyUser).toHaveBeenCalledOnce(); | ||
| }); | ||
|
|
||
| it('When a request is retried, then it logs a warning', () => { | ||
| const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}); | ||
| const options = retryStrategies.withUserNotification('Test', vi.fn()); | ||
|
|
||
| getOnRetry(options)(1, 1000); | ||
|
|
||
| expect(warnSpy).toHaveBeenCalledWith('[SDK] Test retry attempt 1, waiting 1000ms'); | ||
| warnSpy.mockRestore(); | ||
| }); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,23 @@ | ||
| import { RetryOptions } from '@internxt/sdk/dist/shared'; | ||
|
|
||
| export const SILENT_MAX_RETRIES = 2; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same with those constants |
||
| export const USER_NOTIFICATION_MAX_RETRIES = 5; | ||
|
|
||
| export type NotifyUserCallback = () => void; | ||
|
|
||
| export const retryStrategies = { | ||
| silent: (label = 'Global'): RetryOptions => ({ | ||
| maxRetries: SILENT_MAX_RETRIES, | ||
| onRetry(attempt, delay) { | ||
| console.warn(`[SDK] ${label} retry attempt ${attempt}, waiting ${delay}ms`); | ||
| }, | ||
| }), | ||
|
|
||
| withUserNotification: (label: string, notifyUser: NotifyUserCallback): RetryOptions => ({ | ||
| maxRetries: USER_NOTIFICATION_MAX_RETRIES, | ||
| onRetry(attempt, delay) { | ||
| console.warn(`[SDK] ${label} retry attempt ${attempt}, waiting ${delay}ms`); | ||
| notifyUser(); | ||
| }, | ||
| }), | ||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2126,5 +2126,8 @@ | |
| "todayAt": "今天在 " | ||
| }, | ||
| "sharedWorkspace": "共用工作區" | ||
| }, | ||
| "sdk": { | ||
| "rateLimitToast": "這花費的時間比預期的長,請稍候..." | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2163,5 +2163,8 @@ | |
| "todayAt": "今天在 " | ||
| }, | ||
| "sharedWorkspace": "共享工作区" | ||
| }, | ||
| "sdk": { | ||
| "rateLimitToast": "这花费的时间比预期的长,请稍候..." | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -36,6 +36,9 @@ const getHoursUntilExpiration = (expiresAt: Date | string): number => { | |
| return Math.max(0, Math.ceil(dayjs(expiresAt).diff(dayjs(), 'hour', true))); | ||
| }; | ||
|
|
||
| const hasElapsed = (since: Dayjs, amount: number, unit: dayjs.ManipulateType): boolean => | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need a function for this? It's called in only one place and it's a one-line function
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For me its better that way:
wdyt? @TamaraFinogina |
||
| dayjs().diff(since, unit) >= amount; | ||
|
|
||
| const dateService = { | ||
| format, | ||
| isDateOneBefore, | ||
|
|
@@ -45,6 +48,7 @@ const dateService = { | |
| getDaysUntilExpiration, | ||
| getDaysSince, | ||
| getHoursUntilExpiration, | ||
| hasElapsed, | ||
| }; | ||
|
|
||
| export default dateService; | ||
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.
Maybe we should move all constants in src/app/core/constants.ts so that they are all in one place. @CandelR do we have a file with constants specific to sdk or is the one in core the only one?