Skip to content

Error boundary handling for useQueries #4177

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

Merged
merged 14 commits into from
Sep 24, 2022
Merged
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
2 changes: 2 additions & 0 deletions .all-contributorsrc
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,8 @@
"avatar_url": "https://avatars0.githubusercontent.com/u/5398733?v=4",
"profile": "https://github.com/zorzysty",
"contributions": [
"bug",
"code",
"doc"
]
},
Expand Down
4 changes: 4 additions & 0 deletions packages/query-core/src/queriesObserver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,10 @@ export class QueriesObserver extends Subscribable<QueriesObserverListener> {
return this.result
}

getQueries() {
return this.observers.map((observer) => observer.getCurrentQuery())
}

getOptimisticResult(queries: QueryObserverOptions[]): QueryObserverResult[] {
return this.findMatchingObservers(queries).map((match) =>
match.observer.getOptimisticResult(match.defaultedQueryOptions),
Expand Down
24 changes: 18 additions & 6 deletions packages/query-core/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,18 @@ export interface QueryOptions<
meta?: QueryMeta
}

export type UseErrorBoundary<
TQueryFnData,
TError,
TQueryData,
TQueryKey extends QueryKey,
> =
| boolean
| ((
error: TError,
query: Query<TQueryFnData, TError, TQueryData, TQueryKey>,
) => boolean)

export interface QueryObserverOptions<
TQueryFnData = unknown,
TError = unknown,
Expand Down Expand Up @@ -205,12 +217,12 @@ export interface QueryObserverOptions<
* If set to a function, it will be passed the error and the query, and it should return a boolean indicating whether to show the error in an error boundary (`true`) or return the error as state (`false`).
* Defaults to `false`.
*/
useErrorBoundary?:
| boolean
| ((
error: TError,
query: Query<TQueryFnData, TError, TQueryData, TQueryKey>,
) => boolean)
useErrorBoundary?: UseErrorBoundary<
TQueryFnData,
TError,
TQueryData,
TQueryKey
>
/**
* This option can be used to transform or select a part of the data returned by the query function.
*/
Expand Down
2 changes: 1 addition & 1 deletion packages/react-query/src/QueryErrorResetBoundary.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import * as React from 'react'

// CONTEXT

interface QueryErrorResetBoundaryValue {
export interface QueryErrorResetBoundaryValue {
clearReset: () => void
isReset: () => boolean
reset: () => void
Expand Down
124 changes: 124 additions & 0 deletions packages/react-query/src/__tests__/useQueries.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -1102,4 +1102,128 @@ describe('useQueries', () => {
await waitFor(() => rendered.getByText('error boundary'))
})
})

it("should throw error if in one of queries' queryFn throws and useErrorBoundary is in use", async () => {
const key1 = queryKey()
const key2 = queryKey()
const key3 = queryKey()
const key4 = queryKey()

function Page() {
useQueries({
queries: [
{
queryKey: key1,
queryFn: () =>
Promise.reject(
new Error(
'this should not throw because useErrorBoundary is not set',
),
),
},
{
queryKey: key2,
queryFn: () => Promise.reject(new Error('single query error')),
useErrorBoundary: true,
retry: false,
},
{
queryKey: key3,
queryFn: async () => 2,
},
{
queryKey: key4,
queryFn: async () =>
Promise.reject(
new Error('this should not throw because query#2 already did'),
),
useErrorBoundary: true,
retry: false,
},
],
})

return null
}

const rendered = renderWithClient(
queryClient,
<ErrorBoundary
fallbackRender={({ error }) => (
<div>
<div>error boundary</div>
<div>{error.message}</div>
</div>
)}
>
<Page />
</ErrorBoundary>,
)

await waitFor(() => rendered.getByText('error boundary'))
await waitFor(() => rendered.getByText('single query error'))
})

it("should throw error if in one of queries' queryFn throws and useErrorBoundary function resolves to true", async () => {
const key1 = queryKey()
const key2 = queryKey()
const key3 = queryKey()
const key4 = queryKey()

function Page() {
useQueries({
queries: [
{
queryKey: key1,
queryFn: () =>
Promise.reject(
new Error(
'this should not throw because useErrorBoundary function resolves to false',
),
),
useErrorBoundary: () => false,
retry: false,
},
{
queryKey: key2,
queryFn: async () => 2,
},
{
queryKey: key3,
queryFn: () => Promise.reject(new Error('single query error')),
useErrorBoundary: () => true,
retry: false,
},
{
queryKey: key4,
queryFn: async () =>
Promise.reject(
new Error('this should not throw because query#3 already did'),
),
useErrorBoundary: true,
retry: false,
},
],
})

return null
}

const rendered = renderWithClient(
queryClient,
<ErrorBoundary
fallbackRender={({ error }) => (
<div>
<div>error boundary</div>
<div>{error.message}</div>
</div>
)}
>
<Page />
</ErrorBoundary>,
)

await waitFor(() => rendered.getByText('error boundary'))
await waitFor(() => rendered.getByText('single query error'))
})
})
2 changes: 2 additions & 0 deletions packages/react-query/src/__tests__/useQuery.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2807,6 +2807,8 @@ describe('useQuery', () => {

await sleep(10)

await waitFor(() => expect(queryClient.isFetching()).toBe(0))

expect(result?.data).toBe('data')
})

Expand Down
72 changes: 72 additions & 0 deletions packages/react-query/src/errorBoundaryUtils.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
import type {
DefaultedQueryObserverOptions,
Query,
QueryKey,
QueryObserverResult,
UseErrorBoundary,
} from '@tanstack/query-core'
import type { QueryErrorResetBoundaryValue } from './QueryErrorResetBoundary'
import * as React from 'react'
import { shouldThrowError } from './utils'

export const ensurePreventErrorBoundaryRetry = <
TQueryFnData,
TError,
TData,
TQueryData,
TQueryKey extends QueryKey,
>(
options: DefaultedQueryObserverOptions<
TQueryFnData,
TError,
TData,
TQueryData,
TQueryKey
>,
errorResetBoundary: QueryErrorResetBoundaryValue,
) => {
if (options.suspense || options.useErrorBoundary) {
// Prevent retrying failed query if the error boundary has not been reset yet
if (!errorResetBoundary.isReset()) {
options.retryOnMount = false
}
}
}

export const useClearResetErrorBoundary = (
errorResetBoundary: QueryErrorResetBoundaryValue,
) => {
React.useEffect(() => {
errorResetBoundary.clearReset()
}, [errorResetBoundary])
}

export const getHasError = <
TData,
TError,
TQueryFnData,
TQueryData,
TQueryKey extends QueryKey,
>({
result,
errorResetBoundary,
useErrorBoundary,
query,
}: {
result: QueryObserverResult<TData, TError>
errorResetBoundary: QueryErrorResetBoundaryValue
useErrorBoundary: UseErrorBoundary<
TQueryFnData,
TError,
TQueryData,
TQueryKey
>
query: Query<TQueryFnData, TError, TQueryData, TQueryKey>
}) => {
return (
result.isError &&
!errorResetBoundary.isReset() &&
!result.isFetching &&
shouldThrowError(useErrorBoundary, [result.error, query])
)
}
32 changes: 14 additions & 18 deletions packages/react-query/src/useBaseQuery.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,12 @@ import { notifyManager } from '@tanstack/query-core'
import { useQueryErrorResetBoundary } from './QueryErrorResetBoundary'
import { useQueryClient } from './QueryClientProvider'
import type { UseBaseQueryOptions } from './types'
import { shouldThrowError } from './utils'
import { useIsRestoring } from './isRestoring'
import {
ensurePreventErrorBoundaryRetry,
getHasError,
useClearResetErrorBoundary,
} from './errorBoundaryUtils'

export function useBaseQuery<
TQueryFnData,
Expand Down Expand Up @@ -62,12 +66,9 @@ export function useBaseQuery<
}
}

if (defaultedOptions.suspense || defaultedOptions.useErrorBoundary) {
// Prevent retrying failed query if the error boundary has not been reset yet
if (!errorResetBoundary.isReset()) {
defaultedOptions.retryOnMount = false
}
}
ensurePreventErrorBoundaryRetry(defaultedOptions, errorResetBoundary)

useClearResetErrorBoundary(errorResetBoundary)

const [observer] = React.useState(
() =>
Expand All @@ -91,10 +92,6 @@ export function useBaseQuery<
() => observer.getCurrentResult(),
)

React.useEffect(() => {
errorResetBoundary.clearReset()
}, [errorResetBoundary])

React.useEffect(() => {
// Do not notify on updates because of changes in the options because
// these changes should already be reflected in the optimistic result.
Expand Down Expand Up @@ -123,13 +120,12 @@ export function useBaseQuery<

// Handle error boundary
if (
result.isError &&
!errorResetBoundary.isReset() &&
!result.isFetching &&
shouldThrowError(defaultedOptions.useErrorBoundary, [
result.error,
observer.getCurrentQuery(),
])
getHasError({
result,
errorResetBoundary,
useErrorBoundary: defaultedOptions.useErrorBoundary,
query: observer.getCurrentQuery(),
})
) {
throw result.error
}
Expand Down
27 changes: 27 additions & 0 deletions packages/react-query/src/useQueries.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,12 @@ import { notifyManager, QueriesObserver } from '@tanstack/query-core'
import { useQueryClient } from './QueryClientProvider'
import type { UseQueryOptions, UseQueryResult } from './types'
import { useIsRestoring } from './isRestoring'
import { useQueryErrorResetBoundary } from './QueryErrorResetBoundary'
import {
ensurePreventErrorBoundaryRetry,
getHasError,
useClearResetErrorBoundary,
} from './errorBoundaryUtils'

// This defines the `UseQueryOptions` that are accepted in `QueriesOptions` & `GetOptions`.
// - `context` is omitted as it is passed as a root-level option to `useQueries` instead.
Expand Down Expand Up @@ -184,5 +190,26 @@ export function useQueries<T extends any[]>({
observer.setQueries(defaultedQueries, { listeners: false })
}, [defaultedQueries, observer])

const errorResetBoundary = useQueryErrorResetBoundary()

defaultedQueries.forEach((query) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we rename this from queries to options ? I think this is highly confusing right now 😅

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

sorry, I meant more globally that defaultedQueries should also probably be defaultedOptions. It's not a query 😅

Copy link
Collaborator

Choose a reason for hiding this comment

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

okay nevermind, this is actually a bigger refactoring. we have queries everywhere and also setQueries...

Copy link
Collaborator Author

@zorzysty zorzysty Sep 19, 2022

Choose a reason for hiding this comment

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

EDIT:
Just to make it clear, I'm responding to this comment:

sorry, I meant more globally that defaultedQueries should also probably be defaultedOptions. It's not a query

ORIGINAL COMMENT BELOW:
I was struggling to grasp the naming conventions here, and when I thought I finally got it, this comment got me confused again 😁

Let me explain how I understand it. First, here are 3 facts about the code in this PR its current state:

  1. useQueries takes queries array as an argument
export function useQueries<T extends any[]>({
  queries,
  context,
}: {
  queries: readonly [...QueriesOptions<T>]
  context?: UseQueryOptions['context']
})
  1. Each item of queries array is a query, but query is basically an options object
  2. These queries are then defaulted and stored as defaultedQueries:
const defaultedQueries = React.useMemo(
  () =>
    queries.map((options) => {
      const defaultedOptions = queryClient.defaultQueryOptions(options)

      // Make sure the results are already in fetching state before subscribing or updating options
      defaultedOptions._optimisticResults = isRestoring
        ? 'isRestoring'
        : 'optimistic'

      return defaultedOptions
    }),
  [queries, queryClient, isRestoring],
)

It would be strange if mapping with default values would suddenly convert queries to options. Furthermore in the code below QueriesObserver constructor has second parameter named queries and not options (although queries is typed as QueryObserverOptions, because why not xD).

const [observer] = React.useState(
  () => new QueriesObserver(queryClient, defaultedQueries),
)

So here are my thoughts to sum it up:
If we rename defaultedQueries to defaultedOptions it would be confusing because options are of a single query and not of queries array (useQueries does not have options on its own). To make it more consistent and less confusing, I would instead suggest to take the example from how queries are typed in useQueries arguments (type is called QueriesOptions) and do the name change this way:

defaultedQueries -> defaultedQueriesOptions

WDYT?

If that makes sense, maybe we could also do this to make it even more consistent:

export function useQueries<T extends any[]>({
  queries: queriesOptions,
  context,
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah the naming is weird, I agree. But we have a public method called setQueries that actually takes options in, but they are named queries :/

so for now, let's:

  • revert the lats commit (sorry) and just keep it queries for now. Being consistent is better I think.

there are also conflicts now, sorry. If those are solved we can go ahead and merge 🚀

ensurePreventErrorBoundaryRetry(query, errorResetBoundary)
})

useClearResetErrorBoundary(errorResetBoundary)

const firstSingleResultWhichShouldThrow = result.find((singleResult, index) =>
getHasError({
result: singleResult,
errorResetBoundary,
useErrorBoundary: defaultedQueries[index]?.useErrorBoundary ?? false,
query: observer.getQueries()[index]!,
}),
)

if (firstSingleResultWhichShouldThrow?.error) {
throw firstSingleResultWhichShouldThrow.error
}

return result as QueriesResults<T>
}