-
Notifications
You must be signed in to change notification settings - Fork 125
feat: Fetcher updates #1090
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
feat: Fetcher updates #1090
Changes from 5 commits
cc38cb1
2a41fc6
1f54d95
3398d8a
fb1f87c
650a918
a9e9469
8dcf01c
573c195
7306663
e0a327a
73e9bd8
f8e080d
d9da99f
04ed13a
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 |
|---|---|---|
| @@ -1,3 +1,2 @@ | ||
| const WORKFLOW_HISTORY_PAGE_SIZE_CONFIG = 200; | ||
|
|
||
| export default WORKFLOW_HISTORY_PAGE_SIZE_CONFIG; | ||
| export const WORKFLOW_HISTORY_PAGE_SIZE_CONFIG = 1000; | ||
| export const WORKFLOW_HISTORY_FIRST_PAGE_SIZE_CONFIG = 200; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,6 +7,11 @@ import { | |
| } from '@/route-handlers/get-workflow-history/get-workflow-history.types'; | ||
| import request from '@/utils/request'; | ||
|
|
||
| import { | ||
| WORKFLOW_HISTORY_FIRST_PAGE_SIZE_CONFIG, | ||
| WORKFLOW_HISTORY_PAGE_SIZE_CONFIG, | ||
| } from '../config/workflow-history-page-size.config'; | ||
|
|
||
| import { | ||
| type WorkflowHistoryQueryResult, | ||
| type QueryResultOnChangeCallback, | ||
|
|
@@ -44,13 +49,17 @@ export default class WorkflowHistoryFetcher { | |
| if (shouldContinue) { | ||
| this.shouldContinue = shouldContinue; | ||
|
||
| } | ||
| // If already started, return | ||
| if (this.isStarted) return; | ||
|
|
||
| // remove current listener (if exists) to have fresh emits only | ||
| this.unsubscribe?.(); | ||
| this.unsubscribe = null; | ||
|
|
||
| this.isStarted = true; | ||
| let emitCount = 0; | ||
| const currentState = this.observer.getCurrentResult(); | ||
| const fetchedFirstPage = currentState.status !== 'pending'; | ||
| const shouldEnableQuery = !fetchedFirstPage && shouldContinue(currentState); | ||
| const shouldEnableQuery = | ||
| !fetchedFirstPage && this.shouldContinue(currentState); | ||
|
|
||
| if (shouldEnableQuery) { | ||
| this.observer.setOptions({ | ||
|
|
@@ -81,8 +90,6 @@ export default class WorkflowHistoryFetcher { | |
| emit(currentState); | ||
| } | ||
|
|
||
| // remove current listener (if exists) and add new one | ||
| this.unsubscribe?.(); | ||
| this.unsubscribe = this.observer.subscribe((res) => emit(res)); | ||
| } | ||
|
|
||
|
|
@@ -126,7 +133,9 @@ export default class WorkflowHistoryFetcher { | |
| url: `/api/domains/${params.domain}/${params.cluster}/workflows/${params.workflowId}/${params.runId}/history`, | ||
| query: { | ||
| nextPage: pageParam, | ||
| pageSize: params.pageSize, | ||
| pageSize: pageParam | ||
| ? WORKFLOW_HISTORY_PAGE_SIZE_CONFIG | ||
| : WORKFLOW_HISTORY_FIRST_PAGE_SIZE_CONFIG, | ||
| waitForNewEvent: params.waitForNewEvent ?? false, | ||
| } satisfies WorkflowHistoryQueryParams, | ||
| }) | ||
|
|
||
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.
This seems to assert behavior of
setupandmockEndpointsrather than the "unit under test".Is this intended?
Uh oh!
There was an error while loading. Please reload this page.
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.
Which part exactly is testing the setup method?
What the test does here is capturing what request msw received and make sure that the first request had a query param of 200. The query for rest APIs are sent from the fetcher but received and captured by the setup method.
Uh oh!
There was an error while loading. Please reload this page.
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.
It sounds like your intent is to introspect the inner workings of the fetcher?
To suggest an alternative approach:
pageSize.debugInfowhich provides detailed insights.I suggest this because the purpose of
CapturedPageSizeswasn't immediately clear.