Skip to content

feat: Add queryKey to QueryObserverResult #2690

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

feat: Add queryKey to QueryObserverResult #2690

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 7 additions & 1 deletion docs/src/pages/guides/query-functions.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,18 +47,24 @@ useQuery(['todos', todoId], async () => {

## Query Function Variables

Query keys are not just for uniquely identifying the data you are fetching, but are also conveniently passed into your query function and while not always necessary, this makes it possible to extract your query functions if needed:
Query keys are not just for uniquely identifying the data you are fetching, but are also conveniently passed into your query function and to the query result. While this is not always necessary, this makes it possible to extract your query functions if needed:

```js
function Todos({ status, page }) {
const result = useQuery(['todos', { status, page }], fetchTodoList)
processResult(result)
}

// Access the key, status and page variables in your query function!
function fetchTodoList({ queryKey }) {
const [_key, { status, page }] = queryKey
return new Promise()
}

// Access the key, status, page variables in your query result function!
function processResult(result) {
const [_key, { status, page }] = result.queryKey
}
```

## Using a Query Object instead of parameters
Expand Down
3 changes: 3 additions & 0 deletions docs/src/pages/reference/useQuery.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ const {
isRefetching,
isStale,
isSuccess,
queryKey,
refetch,
remove,
status,
Expand Down Expand Up @@ -228,6 +229,8 @@ const result = useQuery({
- The failure count for the query.
- Incremented every time the query fails.
- Reset to `0` when the query succeeds.
- `queryKey: string | unknown[]`
- The queryKey that was originally passed.
- `refetch: (options: { throwOnError: boolean, cancelRefetch: boolean }) => Promise<UseQueryResult>`
- A function to manually refetch the query.
- If the query errors, the error will only be logged. If you want an error to be thrown, pass the `throwOnError: true` option
Expand Down
4 changes: 4 additions & 0 deletions src/core/queryObserver.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -428,6 +428,9 @@ export class QueryObserver<
const prevQueryResult = queryChange
? this.currentResult
: this.previousQueryResult
const queryKey = prevQueryResult
Copy link
Collaborator

Choose a reason for hiding this comment

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

not sure I understand that. Why would we be interested in the key of the previous query?

Copy link
Author

@patrick-mcdougle patrick-mcdougle Sep 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Without this we lost some memoization because the queryKey array was new each time the hook was called.

Said another way: prevQueryResult.queryKey !== query.queryKey (but they are shallow equal).

The only reason I caught that was because a test failed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it important to keep referential stability between queryKeys? If yes, we should likely triple equal compare the query hashes.

What I don’t understand is how this can work with changing keys. For example: having ['todos', id] as key and id transitions from 1 to 2. The previous key would be ['todos', 1] and the new one is ['todos', 2] - but with the logic above we would get the old, wrong key returned, wouldn’t we..?

Copy link
Author

Choose a reason for hiding this comment

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

I need to read the code a little more closely, but it's my understanding that each time createResult on a QueryObserver is called, it's always the same "query" (that is, a unique queryKey [hash]) but the result is being updated with new information (retry, success, error, the hook being called again with no update, etc). Under this understanding, changed keys would result in two instances of a QueryObserver (one for id #1 and another for id #2 in your example above). Are QueryObservers pooled in the way you're suggesting, or is there one QueryObserver per unique queryHash?

I'm going to go read the code again and I'll check back.

Copy link
Author

Choose a reason for hiding this comment

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

So I think this needs to change to:

const queryKey = queryChange ? query.queryKey : prevQuery.queryKey

but this is causing a test failure that I'm going to need to figure out. Help would be great. I think it has to do with the shallow result checking in updateResult ~L623 since the queryKey is an array not a primitive like everything else.

? prevQueryResult.queryKey
: query.queryKey

const { state } = query
let { dataUpdatedAt, error, errorUpdatedAt, isFetching, status } = state
Expand Down Expand Up @@ -563,6 +566,7 @@ export class QueryObserver<
isPreviousData,
isRefetchError: status === 'error' && state.dataUpdatedAt !== 0,
isStale: isStale(query, options),
queryKey,
refetch: this.refetch,
remove: this.remove,
}
Expand Down
7 changes: 6 additions & 1 deletion src/core/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -284,7 +284,11 @@ export interface FetchPreviousPageOptions extends ResultOptions {

export type QueryStatus = 'idle' | 'loading' | 'error' | 'success'

export interface QueryObserverBaseResult<TData = unknown, TError = unknown> {
export interface QueryObserverBaseResult<
TData = unknown,
TError = unknown,
TQueryKey extends QueryKey = QueryKey
> {
data: TData | undefined
dataUpdatedAt: number
error: TError | null
Expand All @@ -303,6 +307,7 @@ export interface QueryObserverBaseResult<TData = unknown, TError = unknown> {
isRefetching: boolean
isStale: boolean
isSuccess: boolean
queryKey: TQueryKey
refetch: <TPageData>(
options?: RefetchOptions & RefetchQueryFilters<TPageData>
) => Promise<QueryObserverResult<TData, TError>>
Expand Down
2 changes: 2 additions & 0 deletions src/react/tests/useInfiniteQuery.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@ describe('useInfiniteQuery', () => {
isRefetching: false,
isStale: true,
isSuccess: false,
queryKey: key,
refetch: expect.any(Function),
remove: expect.any(Function),
status: 'loading',
Expand Down Expand Up @@ -120,6 +121,7 @@ describe('useInfiniteQuery', () => {
isRefetching: false,
isStale: true,
isSuccess: true,
queryKey: key,
refetch: expect.any(Function),
remove: expect.any(Function),
status: 'success',
Expand Down
5 changes: 5 additions & 0 deletions src/react/tests/useQuery.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,7 @@ describe('useQuery', () => {
isRefetching: false,
isStale: true,
isSuccess: false,
queryKey: key,
refetch: expect.any(Function),
remove: expect.any(Function),
status: 'loading',
Expand All @@ -206,6 +207,7 @@ describe('useQuery', () => {
isRefetching: false,
isStale: true,
isSuccess: true,
queryKey: key,
refetch: expect.any(Function),
remove: expect.any(Function),
status: 'success',
Expand Down Expand Up @@ -260,6 +262,7 @@ describe('useQuery', () => {
isRefetching: false,
isStale: true,
isSuccess: false,
queryKey: key,
refetch: expect.any(Function),
remove: expect.any(Function),
status: 'loading',
Expand All @@ -284,6 +287,7 @@ describe('useQuery', () => {
isRefetching: false,
isStale: true,
isSuccess: false,
queryKey: key,
refetch: expect.any(Function),
remove: expect.any(Function),
status: 'loading',
Expand All @@ -308,6 +312,7 @@ describe('useQuery', () => {
isRefetching: false,
isStale: true,
isSuccess: false,
queryKey: key,
refetch: expect.any(Function),
remove: expect.any(Function),
status: 'error',
Expand Down