feat(javascript): add waitForTask in search client #510
Conversation
✅ Deploy Preview for api-clients-automation ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
✗ The generated branch has been deleted.If the PR has been merged, you can check the generated code on the |
…ation into feat/wait-task
eunjae-lee
requested review from
a team,
damcou and
shortcuts
and removed request for
a team
| }: { | ||
| func: () => Promise<TResponse>; | ||
| validate: (response: TResponse) => boolean; | ||
| maxTrial?: number; | ||
| timeout?: (retryCount: number) => number; | ||
| }): Promise<TResponse> { |
There was a problem hiding this comment.
We can provide a type and export it, with its documentation
| @@ -0,0 +1,36 @@ | |||
| export function createRetryablePromise<TResponse>({ | |||
There was a problem hiding this comment.
Can we provide some docs here pls?
| if (isValid) { | ||
| resolve(response); | ||
| } else if (retryCount + 1 >= maxTrial) { | ||
| reject(new Error('The maximum number of trials exceeded.')); |
There was a problem hiding this comment.
We can provide the context in the error (nb retries etc.) just to give confidence it fully executed
clients/algoliasearch-client-javascript/packages/client-common/src/createRetryablePromise.ts
Show resolved
Hide resolved
Co-authored-by: Clément Vannicatte <[email protected]>
...s/algoliasearch-client-javascript/packages/client-common/src/types/CreateRetryablePromise.ts
Outdated
Show resolved
Hide resolved
| }: { | ||
| indexName: string; | ||
| taskID: number; | ||
| } & Omit<CreateRetryablePromiseOptions, 'func' | 'validate'>): Promise<GetTaskResponse> { |
There was a problem hiding this comment.
This function should returnPromise<void>, we already know that it will return status: 'published' right ?
There was a problem hiding this comment.
Yeah I just realized GetTaskResponse only includes status. In the scope of waitForTask, we can ignore the return value from createRetryablePromise.
| * @param waitForTaskProps.indexName - The index in which to perform the request. | ||
| * @param waitForTaskProps.taskID - The unique identifier of the task to wait for. | ||
| */ | ||
| waitForTask({ |
There was a problem hiding this comment.
Should we add a task to the backlog to write tests for those methods?
There was a problem hiding this comment.
I'm not sure if we need a test for waitForTask since it's a combination of getTask and createRetryablePromise and both of the methods have their own tests. We could, in the future, have integration tests with real APIs. WDYT?
🧭 What and Why
🎟 JIRA Ticket: https://algolia.atlassian.net/browse/APIC-472
Changes included:
waitForTaskmethod in the JS search clientcreateRetryablePromisemethod in theclient-commonpackage.We can use
createRetryablePromisefor other types of wait methods if we need any. Also, users can use it to create their ownwaitForTaskwith custom configuration.🧪 Test