feat: Create fetcher utility #1062
Conversation
Signed-off-by: Assem Hafez <assem.hafez@uber.com>
Signed-off-by: Assem Hafez <assem.hafez@uber.com>
| const shouldEnableQuery = | ||
| (!fetchedFirstPage && shouldContinue(currentState)) || fetchedFirstPage; | ||
|
|
||
| if (shouldEnableQuery) { |
There was a problem hiding this comment.
Should the function return early if shouldEnableQuery === false?
There was a problem hiding this comment.
Good question, the observer logic below is still needed to continue fetching next pages once the first one is done. So the steps are:
- Enable the query to fetch the first page
- listen to query updates to fetch the next page
|
|
||
| // Auto stop when there are no more pages (end of history) or when there is a fresh error happens after the start. | ||
| // isError is true when the request failes and retries are exhausted. | ||
| if (res.hasNextPage === false || (res.isError && emitCount > 1)) { |
There was a problem hiding this comment.
I believe the intent is if emitCount > MAX_RETRIES?
Is that the meaning of 1?
There was a problem hiding this comment.
Retries are handled in the configuration. This condition is responsible about this part when there is a fresh error happens after the start. Basically if you are just starting the fetcher and there was an existing Error from previous start, you don't want to stop. This means you only stop when emitCount>1 (not fresh started). I updated the comment to make it more clear.
| // only start emit (fetching next pages) after the initial fetch is complete | ||
| // first page is already fetched on the first subscription below |
There was a problem hiding this comment.
I updated the comment above let me know if it still unclear
| fetchSingleNextPage(): void { | ||
| const state = this.getCurrentState(); | ||
|
|
||
| if (state.status === 'pending') { | ||
| this.observer.setOptions({ | ||
| ...this.buildObserverOptions(this.params), | ||
| enabled: true, | ||
| }); | ||
| } else if (!state.isFetchingNextPage && state.hasNextPage) | ||
| state.fetchNextPage(); | ||
| } |
There was a problem hiding this comment.
The fetchSingleNextPage gives more control for fetching by allowing users to request fetching a single page. It handles this requriment for two cases:
1- query is not enabled yet: in this case it enables the query and the first page is fetched by default.
2- query is already enabled: in this case it would manually request the second page.
Added a comment for explaination
There was a problem hiding this comment.
I think understanding InfiniteQueryObserver behavior is key for understanding this utility. I'm going to add a section in the PR description to cover that. Please let me know if it helps.
| expect(callback1.mock.calls.length).toBeGreaterThan(1); | ||
| }); | ||
|
|
||
| const countBeforeUnsubscribe = callback1.mock.calls.length; |
There was a problem hiding this comment.
Should countBeforeUnsubscribe be hardcoded rather than derived?
Can the value change between test runs?
There was a problem hiding this comment.
It can be argued which is better here. The intent is not to test how much the Observer calls onChange as this depends on multiple implementation details within the class and how they batch the updates. While the test case is making sure whatever the count the observer calls onChange the second callback should recieve more updates.
There was a problem hiding this comment.
For more context, the count before is 3 and after is 5
| const countBeforeUnsubscribe = callback1.mock.calls.length; | ||
| unsubscribe1(); | ||
|
|
||
| fetcher.fetchSingleNextPage(); |
There was a problem hiding this comment.
I'm not sure I follow this test and what unsubscribe has to do with this line.
There was a problem hiding this comment.
The test case is starting two listners at the same time. It fetch the first page and unsubsribe one of the listeners, then it fetch another page and make sure that the first subscriber didn't recieve any new updates
|
|
||
| await waitFor(() => { | ||
| const state = fetcher.getCurrentState(); | ||
| expect(state.isFetching).toBe(false); |
There was a problem hiding this comment.
It isn't clear how shouldContinue => false affects behavior.
There was a problem hiding this comment.
Basically in this case it wont start fetching as the condition is always false. I changed the test case text to make it more specific.
| // Fast-forward through retry delays (3 retries * 3000ms each) | ||
| await jest.advanceTimersByTimeAsync(3 * 3000); |
There was a problem hiding this comment.
Extracting constants would be appropriate and update all call sites to use the constants instead of hardcoded values.
There was a problem hiding this comment.
I guess by all call sites, you mean the test case values. There is one value, so i can extract as a const for readability.
But also maybe you mean share the constant between test and code, if so, this is what i was avoiding on purpose.
src/views/workflow-history/helpers/__tests__/workflow-history-fetcher.test.tsx
Outdated
Show resolved
Hide resolved
Signed-off-by: Assem Hafez <assem.hafez@uber.com>
Signed-off-by: Assem Hafez <assem.hafez@uber.com>
2 participants
Summary
Create a utility that allows start/stop loading history pages. The utility is not react APIs dependant so that it can be started and closed in a separate flow other than the render cycle. This allows fetching more than single page without having to render once for each fetch.
Changes
WorkflowHistoryFetcherInfiniteQueryObserverto keep similar API for data & status.onChangeBackground about
InfiniteQueryObserverUnderstanding InfiniteQueryObserver behaviour is key for understanding this utility. The
InfiniteQueryObserverworks in a similar way asuseInfiniteQuery, In fact it is used internally within the hook.The
InfiniteQueryObserverstarts fetching the first page once you subscribe to changes. In order to control this, the observer is started with a disabled state and we enable it when the user request starting the fetcher or request loading a single page. Afterwards loading next pages are done with a dedicated functionfetchNextPage.The Observer provides an api to listen for state updates which we depend on in order to start fetching next page once the current one is fetched. Also the Observer provides an API for retries that handles retry counts and delays for us and only consider the request failed after exhausting the retries.
Testing
The utility was tested integrated and tested with the history page. Recording available in #1064