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(react-query): non continuous suspense with useSuspenseQueries (#6298) #6303

Merged
merged 2 commits into from
Nov 5, 2023

Conversation

maciej-baruch
Copy link
Contributor

@maciej-baruch maciej-baruch commented Nov 3, 2023

Copy link

vercel bot commented Nov 3, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
query ⬜️ Ignored (Inspect) Visit Preview Nov 5, 2023 10:56am

Copy link

codesandbox-ci bot commented Nov 3, 2023

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 d3acc6f:

Sandbox Source
@tanstack/query-example-react-basic-typescript Configuration
@tanstack/query-example-solid-basic-typescript Configuration
@tanstack/query-example-svelte-basic Configuration
@tanstack/query-example-vue-basic Configuration

Copy link
Collaborator

@TkDodo TkDodo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had this fix in mind:

Subject: [PATCH] fix: useQueries suspense
---
Index: packages/react-query/src/useQueries.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/packages/react-query/src/useQueries.ts b/packages/react-query/src/useQueries.ts
--- a/packages/react-query/src/useQueries.ts	
+++ b/packages/react-query/src/useQueries.ts	
@@ -1,7 +1,11 @@
 'use client'
 import * as React from 'react'
 
-import { QueriesObserver, notifyManager } from '@tanstack/query-core'
+import {
+  QueriesObserver,
+  QueryObserver,
+  notifyManager,
+} from '@tanstack/query-core'
 import { useQueryClient } from './QueryClientProvider'
 import { useIsRestoring } from './isRestoring'
 import { useQueryErrorResetBoundary } from './QueryErrorResetBoundary'
@@ -262,9 +266,9 @@
   const suspensePromises = shouldAtLeastOneSuspend
     ? optimisticResult.flatMap((result, index) => {
         const opts = defaultedQueries[index]
-        const queryObserver = observer.getObservers()[index]
 
-        if (opts && queryObserver) {
+        if (opts) {
+          const queryObserver = new QueryObserver(client, opts)
           if (shouldSuspend(opts, result, isRestoring)) {
             return fetchOptimistic(opts, queryObserver, errorResetBoundary)
           } else if (willFetch(result, isRestoring)) {

I think we don't need to use the matching observer on the QueriesObserver - the fetch is fire-and-forget anyways, so I just created a new QueryObserver to do the fetch.

the fetchOptimistic method on the observer is just a small wrapper around query.fetch(), so we technically don't even need it ... but changing that could be considered breaking, so this is the most minimal fix I could find. Can you try if your tests also work with that solution?

Copy link

nx-cloud bot commented Nov 3, 2023

☁️ Nx Cloud Report

CI is running/has finished running commands for commit d3acc6f. 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 target

Sent with 💌 from NxCloud.

@maciej-baruch
Copy link
Contributor Author

maciej-baruch commented Nov 3, 2023

Nice and simple.
The tests run, I also fixed the failing eslint checks.

@TkDodo
Copy link
Collaborator

TkDodo commented Nov 4, 2023

There are some warnings with the way the tests are written:

Warning: The current testing environment is not configured to support act(...)

maybe relates to:

not sure if we'd need to update testing-library or vitest or both?

Maybe you could also change the tests to be written more like the other tests that we have? We basically don't test hooks, but wrap them in components like so:

it('should not call the queryFn twice when used in Suspense mode', async () => {
const key = queryKey()
const queryFn = vi.fn<Array<unknown>, string>()
queryFn.mockImplementation(() => {
sleep(10)
return 'data'
})
function Page() {
useSuspenseQuery({ queryKey: [key], queryFn })
return <>rendered</>
}
const rendered = renderWithClient(
queryClient,
<React.Suspense fallback="loading">
<Page />
</React.Suspense>,
)
await waitFor(() => rendered.getByText('rendered'))
expect(queryFn).toHaveBeenCalledTimes(1)
})

@maciej-baruch
Copy link
Contributor Author

Happy to comply.

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Files Coverage Δ
packages/react-query/src/useQueries.ts 87.50% <100.00%> (ø)

... and 67 files with indirect coverage changes

📢 Thoughts on this report? Let us know!.

@TkDodo TkDodo merged commit e4138ec into TanStack:main Nov 5, 2023
5 checks passed
OrlovAlexei pushed a commit to OrlovAlexei/query that referenced this pull request Nov 7, 2023
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.

3 participants