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

[RFC] 🥅 Improve feedback for infinite query loops #1531

Closed
Closed
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
24 changes: 23 additions & 1 deletion docs/rtk-query/usage/queries.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,29 @@ The `queryArg` param will be passed through to the underlying `query` callback t

:::caution

The `queryArg` param is handed to a `useEffect` dependency array internally. RTK Query tries to keep the argument stable by performing a `shallowEquals` diff on the value, however if you pass a deeper nested argument, you will need to keep the param stable yourself, e.g. with `useMemo`.
The `queryArg` param is handed to a `useEffect` dependency array internally.
RTK Query tries to keep the argument stable by performing a `shallowEquals` diff on the value,
however if you pass a deeper nested argument, you will need to keep the param stable yourself,
e.g. with `useMemo`.

```diff
function FilterList({ kind, sortField = 'created_at', sortOrder = 'ASC' }) {
- // The `sort` object would fail the internal shallow stability check and be new every render
- const result = useMyQuery({
- kind,
- sort: { field: sortField, order: sortOrder },
- })

+ // `useMemo` can be used to maintain stability for the entire argument object
+ const queryArg = useMemo(
+ () => ({ kind, sort: { field: sortField, order: sortOrder } }),
+ [kind, sortField, sortOrder]
+ )
+ const result = useMyQuery(queryArg)

return <div>...</div>
}
```

:::

Expand Down
5 changes: 5 additions & 0 deletions packages/toolkit/src/query/react/buildHooks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ import type {
} from '@reduxjs/toolkit/dist/query/core/module'
import type { ReactHooksModuleOptions } from './module'
import { useShallowStableValue } from './useShallowStableValue'
import { useStabilityMonitor } from './useStabilityMonitor'
import type { UninitializedValue } from './constants'
import { UNINITIALIZED_VALUE } from './constants'

Expand Down Expand Up @@ -506,6 +507,10 @@ export function buildHooks<Definitions extends EndpointDefinitions>({
>
const dispatch = useDispatch<ThunkDispatch<any, any, AnyAction>>()
const stableArg = useShallowStableValue(skip ? skipToken : arg)
useStabilityMonitor(stableArg, {
errMsg:
'The Query argument has been detected as changing too many times within a short period. See https://redux-toolkit.js.org/rtk-query/usage/queries#query-hook-options for more information on how to stabilize the argument and fix this issue.',
})
const stableSubscriptionOptions = useShallowStableValue({
refetchOnReconnect,
refetchOnFocus,
Expand Down
42 changes: 42 additions & 0 deletions packages/toolkit/src/query/react/useStabilityMonitor.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
import { useRef, useEffect } from 'react'

/**
* Monitors the 'stability' of the provided value across renders.
* If the value changes more times than the threshold within the provided
* time delta, an error will be through.
*
* Defaults to throwing if changing 10 times in 10 consecutive renders within 1000ms
*/
export function useStabilityMonitor<T>(
value: T,
{
delta = 1000,
threshold = 10,
errMsg = 'Value changed too many times.',
} = {}
) {
const lastValue = useRef(value)
const consecutiveTimestamps = useRef<number[]>([])

useEffect(() => {
// where a render occurs but value didn't change, consider the value to be 'stable'
// and clear recorded timestamps.
// i.e. only keep timestamps if the value changes every render
if (lastValue.current === value) consecutiveTimestamps.current = []
lastValue.current = value
})

useEffect(() => {
const now = Date.now()
consecutiveTimestamps.current.push(now)
consecutiveTimestamps.current = consecutiveTimestamps.current.filter((timestamp) => {
return timestamp > now - delta
})

if (consecutiveTimestamps.current.length >= threshold) {
const err = new Error(errMsg)
Error.captureStackTrace(err, useStabilityMonitor)
throw err
}
}, [value, delta, threshold, errMsg])
}
65 changes: 63 additions & 2 deletions packages/toolkit/src/query/tests/buildHooks.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ const api = createApi({
amount += 1
}

if (arg?.body && 'forceError' in arg.body) {
if (arg?.body && 'forceError' in arg.body && arg.body.forceError) {
return {
error: {
status: 500,
Expand Down Expand Up @@ -65,7 +65,12 @@ const api = createApi({
query: (update) => ({ body: update }),
}),
getError: build.query({
query: (query) => '/error',
query: (query) => ({
url: '/error',
body: {
forceError: true,
},
}),
}),
}),
})
Expand Down Expand Up @@ -2029,3 +2034,59 @@ describe('skip behaviour', () => {
expect(subscriptionCount('getUser(1)')).toBe(0)
})
})

describe('query arg stability monitoring', () => {
type ErrorBoundaryProps = {
children: React.ReactNode
}
class ErrorBoundary extends React.Component<
ErrorBoundaryProps,
{ hasError: boolean; errMsg: string }
> {
constructor(props: ErrorBoundaryProps) {
super(props)
this.state = { hasError: false, errMsg: '' }
}
static getDerivedStateFromError(error: unknown) {
return {
hasError: true,
errMsg: error instanceof Error ? error.message : 'Unknown error',
}
}

render() {
if (this.state.hasError) {
return <div>{this.state.errMsg}</div>
}
return <>{this.props.children}</>
}
}

function FilterList() {
const nestedArg = {
kind: 'berries',
sort: { field: 'created_at', order: 'ASC' },
}

const result = api.endpoints.getError.useQuery(nestedArg)

return (
<div>
<div data-testid="isFetching">{String(result.isFetching)}</div>
</div>
)
}

test('Throws an error early upon an infinite loop due to nested args', async () => {
render(
<ErrorBoundary>
<FilterList />
</ErrorBoundary>,
{ wrapper: storeRef.wrapper }
)

await screen.findByText(
/The Query argument has been detected as changing too many times within a short period/i
)
})
})