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

add ability to warn when suspense queries aren't prefetched #8309

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 8 additions & 8 deletions integrations/react-next-15/app/client-component.tsx
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
'use client'

import React from 'react'
import { useQuery } from '@tanstack/react-query'
import { useQuery, useSuspenseQuery } from '@tanstack/react-query'
import { Temporal } from '@js-temporal/polyfill'

export function ClientComponent() {
const query = useQuery({
const query = useSuspenseQuery({
queryKey: ['data'],
queryFn: async () => {
await new Promise((r) => setTimeout(r, 1000))
Expand All @@ -16,13 +16,13 @@ export function ClientComponent() {
},
})

if (query.isPending) {
return <div>Loading...</div>
}
// if (query.isPending) {
// return <div>Loading...</div>
// }

if (query.isError) {
return <div>An error has occurred!</div>
}
// if (query.isError) {
// return <div>An error has occurred!</div>
// }

return (
<div>
Expand Down
1 change: 1 addition & 0 deletions integrations/react-next-15/app/make-query-client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ export function makeQueryClient() {
},
queries: {
staleTime: 60 * 1000,
warnOnServerFetches: true,
},
dehydrate: {
serializeData: tson.serialize,
Expand Down
26 changes: 14 additions & 12 deletions integrations/react-next-15/app/page.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import React from 'react'
import React, { Suspense } from 'react'
import { HydrationBoundary, dehydrate } from '@tanstack/react-query'
import { Temporal } from '@js-temporal/polyfill'
import { ClientComponent } from './client-component'
Expand All @@ -9,21 +9,23 @@ const sleep = (ms: number) => new Promise((resolve) => setTimeout(resolve, ms))
export default async function Home() {
const queryClient = makeQueryClient()

void queryClient.prefetchQuery({
queryKey: ['data'],
queryFn: async () => {
await sleep(2000)
return {
text: 'data from server',
date: Temporal.PlainDate.from('2024-01-01'),
}
},
})
// void queryClient.prefetchQuery({
// queryKey: ['data'],
// queryFn: async () => {
// await sleep(2000)
// return {
// text: 'data from server',
// date: Temporal.PlainDate.from('2024-01-01'),
// }
// },
// })

return (
<main>
<HydrationBoundary state={dehydrate(queryClient)}>
<ClientComponent />
<Suspense fallback={<div>Loading...</div>}>
<ClientComponent />
</Suspense>
</HydrationBoundary>
</main>
)
Expand Down
6 changes: 6 additions & 0 deletions packages/query-core/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,12 @@ export interface QueryOptions<
* Maximum number of pages to store in the data of an infinite query.
*/
maxPages?: number
/**
* If set to `true`, the query will warn if it is fetched on the server.
* Use this if you intend to only consume prefetched queries.
* Defaults to `false`.
*/
warnOnServerFetches?: boolean
Copy link

Choose a reason for hiding this comment

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

bike shedding: discussed with co-workers, something like strictPrefetch/strictPrefetches? Happy with whatever the consensus is

Choose a reason for hiding this comment

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

We could also consider different levels, so for example strict might lead to a runtime exception when compared to passing a warn value to simply log it 🤔

  • given @TkDodo's recent talk, this should probably also accept a callback that gets the query(?) and not just a static boolean flag
  • warning log might be too specific, would it be better for this to be a function and allow the developer to control what should happen? Perhaps they want to log more/less info about the query to help track down the query that's missing a prefetch

I do like the callback approach more though, we could add an option called onPrefetchMiss or something along those lines.
This would give us more control over the outcome, e.g. could add observability, warning, throw an exception, etc.

Copy link
Contributor Author

@juliusmarminge juliusmarminge Nov 19, 2024

Choose a reason for hiding this comment

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

I do like the callback approach more though, we could add an option called
onPrefetchMiss or something along those lines.
This would give us more control over the outcome, e.g. could add observability, > warning, throw an exception, etc.

yea definetely see this useful for much more than just warning for un-prefetched queries. would love @TkDodo's thoughts on what API he prefers

}

export interface InitialPageParam<TPageParam = unknown> {
Expand Down
96 changes: 96 additions & 0 deletions packages/react-query/src/__tests__/useSuspenseQuery.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -903,4 +903,100 @@ describe('useSuspenseQuery', () => {
)
consoleErrorSpy.mockRestore()
})

describe.only("warnOnServerFetches should warn when query isn't prefetched", () => {
it('global default', async () => {
const queryClient = createQueryClient({
defaultOptions: {
queries: {
warnOnServerFetches: true,
},
},
})

const consoleWarnSpy = vi
.spyOn(console, 'warn')
.mockImplementation(() => {})

function App() {
useSuspenseQuery({
queryKey: queryKey(),
queryFn: () => Promise.resolve('data1'),
})

return null
}

renderWithClient(queryClient, <App />)

expect(consoleWarnSpy).toHaveBeenCalledWith(
'A new query suspended on the server without any cache entry.',
)
})

it('local override', async () => {
const queryClient = createQueryClient({
defaultOptions: {
queries: {
warnOnServerFetches: true,
},
},
})

const consoleWarnSpy = vi
.spyOn(console, 'warn')
.mockImplementation(() => {})

function App() {
useSuspenseQuery({
queryKey: queryKey(),
queryFn: () => Promise.resolve('data1'),
warnOnServerFetches: false,
})

return null
}

renderWithClient(queryClient, <App />)

expect(consoleWarnSpy).not.toHaveBeenCalled()
})

it('mix of both', async () => {
const queryClient = createQueryClient({
defaultOptions: {
queries: {
warnOnServerFetches: true,
},
},
})

const consoleWarnSpy = vi
.spyOn(console, 'warn')
.mockImplementation(() => {})

function Component(props: { warnOnServerFetches?: boolean }) {
useSuspenseQuery({
queryKey: queryKey(),
queryFn: () => Promise.resolve('data'),
warnOnServerFetches: props.warnOnServerFetches,
})

return null
}

function App() {
return (
<>
<Component />
<Component warnOnServerFetches={false} />
</>
)
}

renderWithClient(queryClient, <App />)

expect(consoleWarnSpy).not.toHaveBeenCalledTimes(1)
})
})
})
5 changes: 5 additions & 0 deletions packages/react-query/src/useBaseQuery.ts
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,11 @@ export function useBaseQuery<

// Handle suspense
if (shouldSuspend(defaultedOptions, result)) {
if (defaultedOptions.warnOnServerFetches && isNewCacheEntry) {
console.warn(
'A new query suspended on the server without any cache entry.',
)
}
throw fetchOptimistic(defaultedOptions, observer, errorResetBoundary)
}

Expand Down
Loading