-
Notifications
You must be signed in to change notification settings - Fork 9
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: add useInfiniteDataLoader #2282
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 6b32e34 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2282 +/- ##
==========================================
+ Coverage 91.26% 91.62% +0.35%
==========================================
Files 47 49 +2
Lines 2371 2592 +221
Branches 104 118 +14
==========================================
+ Hits 2164 2375 +211
- Misses 196 203 +7
- Partials 11 14 +3 ☔ View full report in Codecov by Sentry. |
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.
I don't have much time to fully deep dive into the code flow. But happy to test it with our use cases. LGTM
useEffect( | ||
() => () => { | ||
requestRefs.current.forEach(request => request.removeObserver(() => null)) | ||
}, | ||
[], | ||
) |
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.
Is it executed ?
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.
Yes on unmount
ErrorType extends Error, | ||
ParamsType, | ||
ParamsKey extends keyof ParamsType, | ||
> = Omit<UseDataLoaderConfig<ResultType, ErrorType>, 'initialData'> & { |
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.
Should we remove polling ?
req => req.status === StatusEnum.IDLE && !enabled, | ||
) | ||
|
||
const reload = useCallback(async () => { |
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.
What happen when a page doesn't exists anymore ?
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.
data will be null/undefined/empty array and be filtered out when user get the data from the hook. May be if it doesnt fit some edge cases I can cont how many requests we have and try to get each one sequentially then by using getNextPage the hook can try to fill the same number of pages but if it cant it will not make useless calls ? 🤔
Implement useInfiniteDataLoader to handle Load More on our UI