-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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: usePrefetchQuery #7582
feat: usePrefetchQuery #7582
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
☁️ Nx Cloud ReportCI is running/has finished running commands for commit dd7cc03. As they complete they will appear below. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this CI Pipeline Execution ✅ Successfully ran 1 targetSent with 💌 from NxCloud. |
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit dd7cc03:
|
will do in a separate PR
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #7582 +/- ##
===========================================
+ Coverage 43.88% 86.03% +42.14%
===========================================
Files 184 24 -160
Lines 7029 315 -6714
Branches 1539 80 -1459
===========================================
- Hits 3085 271 -2814
+ Misses 3578 37 -3541
+ Partials 366 7 -359
|
…7586) * chore: add tests for usePrefetchQuery and usePrefetchInfiniteQuery * chore: update tests to assert the alternative spy is not called * chore: add some new tests * chore: remove it.only whoops * chore: call mockClear after fetching * chore: improve waterfall test by asserting fallback calls instead of loading node query * chore: improve code repetition * chore: add some generics to helper functions
* chore: add type tests and docs * chore: update hooks to use FetchQueryOptions and FetchInfiniteQueryOptions * chore: update tests * chore: update docs * chore: remove .md extension from link * chore: add unknown default value to TQueryFnData * Apply suggestions from code review --------- Co-authored-by: Dominik Dorfmeister <[email protected]>
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 left a few NITs on the tests, but I think the implementation is fine to ship as is. 🎉
const rendered = renderWithClient(queryClient, <App />) | ||
|
||
await waitFor(() => rendered.getByText('data: Prefetch is nice!')) | ||
expect(queryOpts.queryFn).toHaveBeenCalledTimes(1) |
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 this testing what it says? It might also be the case that usePrefetchQuery
didn't fetch, but useSuspenseQuery
did. Not sure if it necessarily needs fixing, but thought I should point it out.
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.
yeah we should probably use a different queryFn
for prefetching and the component so that we can assert which one ran. @brunolopesr can you add that?
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.
}) | ||
|
||
function App() { | ||
usePrefetchQuery(queryOpts) |
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 think to replicate the bug we saw with endless loops, the usePrefetchQuery
would have to be inside the Suspense boundary so it would rerender->refetch when unsuspended? I haven't tested so not entirely sure though.
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.
@Ephem , I'm not sure, but this endless loops isn't about the usage of ensureQueryData
instead of prefetchQuery
? But I will add a test covering the usage of the hook inside the Suspense Boundary.
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.
Added new tests to ensure that endless loops don't occur at #7614
await waitFor(() => | ||
rendered.getByText('data: Prefetch does not create waterfalls!!'), | ||
) | ||
expect(Fallback).toHaveBeenCalledTimes(1) |
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 this what tests for waterfalls? Seems a bit brittle to me. Maybe we could inspect the queryClient
right after the renderWithClient
call to see that all three queries are fetching?
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.
Added a test to assert the query state just after rendering in #7614
Co-authored-by: Fredrik Höglund <[email protected]>
* chore: add new tests * Apply suggestions from code review --------- Co-authored-by: Dominik Dorfmeister <[email protected]>
missing: