feat: Create hook for fetching history#1063
feat: Create hook for fetching history#1063Assem-Uber merged 16 commits intocadence-workflow:masterfrom
Conversation
Signed-off-by: Assem Hafez <assem.hafez@uber.com>
Signed-off-by: Assem Hafez <assem.hafez@uber.com>
Signed-off-by: Assem Hafez <assem.hafez@uber.com>
Signed-off-by: Assem Hafez <assem.hafez@uber.com>
| const unsubscribe = fetcherRef.current.onChange((state) => { | ||
| const pagesCount = state.data?.pages?.length || 0; | ||
| // immediately set if there is the first page without throttling other wise throttle | ||
| setHistoryQuery(() => state, pagesCount <= 1); |
There was a problem hiding this comment.
It took me a while to realize that pagesCount <= 1 is executeImmediately.
There was a problem hiding this comment.
Added it in a variable
| // Fetch first page | ||
| fetcherRef.current.start((state) => !state?.data?.pages?.length); |
There was a problem hiding this comment.
I wonder why we fetch the first page explicitly rather than relying on the flywheel / fetch-loop?
There was a problem hiding this comment.
Both works, in this case i went for being more explicit about loading the first page separately from the conditions for keep loading more. In any case we won't want to load and empty page, so it is some how safer to assume first page always loads. The other approach works too, i don't see issues with it.
There was a problem hiding this comment.
Btw fetchSingleNextPage can also be used here, but i went with that assuming this is more readable. Let me know if it is not the same for you.
src/views/workflow-history/hooks/use-workflow-history-fetcher.ts
Outdated
Show resolved
Hide resolved
| @@ -0,0 +1,88 @@ | |||
| import { useCallback, useEffect, useRef } from 'react'; | |||
There was a problem hiding this comment.
Is this file included in this PR because it's stacked on the prior PR?
There was a problem hiding this comment.
Yes, unfortunately both exists in this PR as they are dependant.
macrotim
left a comment
macrotim
left a comment
There was a problem hiding this comment.
I believe there is room for a few improvements (mainly, to help new readers like me follow along).
Approving to unblock and low risk.
Signed-off-by: Assem Hafez <assem.hafez@uber.com>
Signed-off-by: Assem Hafez <assem.hafez@uber.com>
Signed-off-by: Assem Hafez <assem.hafez@uber.com>
Signed-off-by: Assem Hafez <assem.hafez@uber.com>
Signed-off-by: Assem Hafez <assem.hafez@uber.com>
…2/create-hook-for-history-fetcher
Signed-off-by: Assem Hafez <assem.hafez@uber.com>
Signed-off-by: Assem Hafez <assem.hafez@uber.com>
2 participants
Summary
Creating a hook to be responsible of creating an instance of
WorkflowHistoryFetcherand managing its state updates. This going to be used in the Workflow History Page.Changes
WorkflowHistoryFetcher