Skip to content
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

fix: race condition with pagination token #105

Merged
merged 2 commits into from
Oct 16, 2020
Merged

Conversation

schottra
Copy link
Contributor

flyteorg/flyte#546

usePagination stores the pagination token in separate state hook. After the useFetchableData refactor to use state machines, the fetchable will now reset before we have a change to update the token state, causing the value to persist across API calls. This update usePagination to store the token in the fetchable data, so that it is part of the context used by the state machine and will remain in sync with the data it applies to.
Also removed the unnecessary use of a separate state hook for the moreItemsAvailable field, since that can be directly computed from the presence of the token.

Since usePagination returns an object that mostly looks like a FetchableData<T>, but using a different context shape under the hood, it was necessary to update the type for FetchableData so that the state field was a FetchableState<any>. This changes feels okay, because the intended usage of passing that state field along is to allow consumers to check which state we are in for conditional rendering (using state.matches), not necessarily to allow logic based on the context contained within the state. I may open a follow-up PR to refactor the usage of fetch state to pass an even more restricted object, or remove the visibility of that state field from consuming code altogether.

@codecov-io
Copy link

codecov-io commented Oct 15, 2020

Codecov Report

Merging #105 into master will increase coverage by 0.18%.
The diff coverage is 92.30%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #105      +/-   ##
==========================================
+ Coverage   69.18%   69.37%   +0.18%     
==========================================
  Files         383      383              
  Lines        6306     6299       -7     
  Branches      988      986       -2     
==========================================
+ Hits         4363     4370       +7     
+ Misses       1943     1929      -14     
Impacted Files Coverage Δ
src/components/hooks/types.ts 100.00% <ø> (ø)
src/components/hooks/usePagination.ts 94.73% <92.30%> (+6.27%) ⬆️
.../components/Executions/Tables/NodeExecutionRow.tsx 91.11% <0.00%> (+2.22%) ⬆️
src/components/common/ExpandableMonospaceText.tsx 86.36% <0.00%> (+36.36%) ⬆️
...nts/Executions/Tables/ExpandableExecutionError.tsx 100.00% <0.00%> (+42.85%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4aed79b...00b7279. Read the comment docs.

@service-github-lyft-semantic-release
Copy link
Contributor

🎉 This PR is included in version 0.13.2 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants