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

feat(QueryObserver): track queries as default #2987

Merged
merged 13 commits into from Nov 20, 2021
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions docs/src/pages/guides/migrating-to-react-query-4.md
Expand Up @@ -14,6 +14,10 @@ With version [3.22.0](https://github.com/tannerlinsley/react-query/releases/tag/
+ import { dehydrate, hydrate, useHydrate, Hydrate } from 'react-query'
```

### `notifyOnChangePropsExclusion` has been removed

In v4, `notifyOnChangeProps` defaults to the `"tracked"` behavior of v3 instead of `undefined`. Now that `"tracked"` is the default behavior for v4, it no longer makes sense to include this config option.

### Consistent behavior for `cancelRefetch`

The `cancelRefetch` can be passed to all functions that imperatively fetch a query, namely:
Expand Down
2 changes: 1 addition & 1 deletion docs/src/pages/reference/QueryClient.md
Expand Up @@ -91,7 +91,7 @@ try {

**Options**

The options for `fetchQuery` are exactly the same as those of [`useQuery`](./useQuery), except the following: `enabled, refetchInterval, refetchIntervalInBackground, refetchOnWindowFocus, refetchOnReconnect, notifyOnChangeProps, notifyOnChangePropsExclusions, onSuccess, onError, onSettled, useErrorBoundary, select, suspense, keepPreviousData, placeholderData`; which are strictly for useQuery and useInfiniteQuery. You can check the [source code](https://github.com/tannerlinsley/react-query/blob/361935a12cec6f36d0bd6ba12e84136c405047c5/src/core/types.ts#L83) for more clarity.
The options for `fetchQuery` are exactly the same as those of [`useQuery`](./useQuery), except the following: `enabled, refetchInterval, refetchIntervalInBackground, refetchOnWindowFocus, refetchOnReconnect, notifyOnChangeProps, onSuccess, onError, onSettled, useErrorBoundary, select, suspense, keepPreviousData, placeholderData`; which are strictly for useQuery and useInfiniteQuery. You can check the [source code](https://github.com/tannerlinsley/react-query/blob/361935a12cec6f36d0bd6ba12e84136c405047c5/src/core/types.ts#L83) for more clarity.

**Returns**

Expand Down
5 changes: 0 additions & 5 deletions docs/src/pages/reference/useQuery.md
Expand Up @@ -35,7 +35,6 @@ const {
keepPreviousData,
meta,
notifyOnChangeProps,
notifyOnChangePropsExclusions,
onError,
onSettled,
onSuccess,
Expand Down Expand Up @@ -131,10 +130,6 @@ const result = useQuery({
- If set, the component will only re-render if any of the listed properties change.
- If set to `['data', 'error']` for example, the component will only re-render when the `data` or `error` properties change.
- If set to `"tracked"`, access to properties will be tracked, and the component will only re-render when one of the tracked properties change.
- `notifyOnChangePropsExclusions: string[]`
- Optional
- If set, the component will not re-render if any of the listed properties change.
- If set to `['isStale']` for example, the component will not re-render when the `isStale` property changes.
- `onSuccess: (data: TData) => void`
- Optional
- This function will fire any time the query successfully fetches new data or the cache is updated via `setQueryData`.
Expand Down
16 changes: 7 additions & 9 deletions src/core/queryObserver.ts
Expand Up @@ -596,27 +596,25 @@ export class QueryObserver<
return true
}

const { notifyOnChangeProps, notifyOnChangePropsExclusions } = this.options
const { notifyOnChangeProps } = this.options

if (!notifyOnChangeProps && !notifyOnChangePropsExclusions) {
if (notifyOnChangeProps === 'all') {
return true
}

if (notifyOnChangeProps === 'tracked' && !this.trackedProps.length) {
if (!notifyOnChangeProps && !this.trackedProps.length) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

let’s combine these two if statements that return true with an ||

return true
}

const includedProps =
notifyOnChangeProps === 'tracked'
? this.trackedProps
: notifyOnChangeProps
const includedProps = !notifyOnChangeProps
? this.trackedProps
: notifyOnChangeProps
Copy link
Collaborator

Choose a reason for hiding this comment

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

negated ifs are pretty hard to read imo, so is this the same as:

    const includedProps = notifyOnChangeProps ?? this.trackedProps

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Totally agree on using ??


return Object.keys(result).some(key => {
const typedKey = key as keyof QueryObserverResult
const changed = result[typedKey] !== prevResult[typedKey]
const isIncluded = includedProps?.some(x => x === key)
const isExcluded = notifyOnChangePropsExclusions?.some(x => x === key)
return changed && !isExcluded && (!includedProps || isIncluded)
return changed && (!includedProps || isIncluded)
})
}

Expand Down
6 changes: 1 addition & 5 deletions src/core/types.ts
Expand Up @@ -160,11 +160,7 @@ export interface QueryObserverOptions<
* When set to `['data', 'error']`, the component will only re-render when the `data` or `error` properties change.
* When set to `tracked`, access to properties will be tracked, and the component will only re-render when one of the tracked properties change.
Copy link
Collaborator

Choose a reason for hiding this comment

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

the comment is no longer accurate here :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yah - this is for sure part of the docs i need to update

*/
notifyOnChangeProps?: Array<keyof InfiniteQueryObserverResult> | 'tracked'
/**
* If set, the component will not re-render if any of the listed properties change.
*/
notifyOnChangePropsExclusions?: Array<keyof InfiniteQueryObserverResult>
notifyOnChangeProps?: Array<keyof InfiniteQueryObserverResult> | 'all'
/**
* This callback will fire any time the query successfully fetches new data or the cache is updated via `setQueryData`.
*/
Expand Down
79 changes: 2 additions & 77 deletions src/reactjs/tests/useQuery.test.tsx
Expand Up @@ -917,54 +917,12 @@ describe('useQuery', () => {
consoleMock.mockRestore()
})

it('should re-render when dataUpdatedAt changes but data remains the same', async () => {
const key = queryKey()
const states: UseQueryResult<string>[] = []

function Page() {
const state = useQuery(key, () => 'test', {
notifyOnChangePropsExclusions: [
'data',
'isFetching',
'isLoading',
'isRefetching',
'isSuccess',
'status',
],
})

states.push(state)

const { refetch } = state

React.useEffect(() => {
setActTimeout(() => {
refetch()
}, 5)
}, [refetch])

return null
}

renderWithClient(queryClient, <Page />)

await sleep(10)

expect(states.length).toBe(3)
expect(states[0]).toMatchObject({ data: undefined, isFetching: true })
expect(states[1]).toMatchObject({ data: 'test', isFetching: false })
expect(states[2]).toMatchObject({ data: 'test', isFetching: false })
expect(states[1]?.dataUpdatedAt).not.toBe(states[2]?.dataUpdatedAt)
})

it('should track properties and only re-render when a tracked property changes', async () => {
const key = queryKey()
const states: UseQueryResult<string>[] = []

function Page() {
const state = useQuery(key, () => 'test', {
notifyOnChangeProps: 'tracked',
})
const state = useQuery(key, () => 'test')

states.push(state)

Expand Down Expand Up @@ -992,46 +950,13 @@ describe('useQuery', () => {
expect(states[1]).toMatchObject({ data: 'test' })
})

it('should not re-render if a tracked prop changes, but it was excluded', async () => {
const key = queryKey()
const states: UseQueryResult<string>[] = []

function Page() {
const state = useQuery(key, () => 'test', {
notifyOnChangeProps: 'tracked',
notifyOnChangePropsExclusions: ['data'],
})

states.push(state)

return (
<div>
<h1>{state.data ?? 'null'}</h1>
</div>
)
}

const rendered = renderWithClient(queryClient, <Page />)

await waitFor(() => rendered.getByText('null'))
expect(states.length).toBe(1)
expect(states[0]).toMatchObject({ data: undefined })

await queryClient.refetchQueries(key)
await waitFor(() => rendered.getByText('null'))
expect(states.length).toBe(1)
expect(states[0]).toMatchObject({ data: undefined })
})

it('should always re-render if we are tracking props but not using any', async () => {
const key = queryKey()
let renderCount = 0
const states: UseQueryResult<string>[] = []

function Page() {
const state = useQuery(key, () => 'test', {
notifyOnChangeProps: 'tracked',
})
const state = useQuery(key, () => 'test')

states.push(state)

Expand Down
2 changes: 1 addition & 1 deletion src/reactjs/useBaseQuery.ts
Expand Up @@ -143,7 +143,7 @@ export function useBaseQuery<
}

// Handle result property usage tracking
if (defaultedOptions.notifyOnChangeProps === 'tracked') {
if (!defaultedOptions.notifyOnChangeProps) {
result = observer.trackResult(result)
}

Expand Down