query icon indicating copy to clipboard operation
query copied to clipboard

queryOptions should to pass initialData with a callback that is potentially returning undefined

Open ShacharHarshuv opened this issue 1 year ago • 9 comments

Describe the bug

In angular-query When using queryOptions '@tanstack/angular-query-experimental' and passing an initial value function, typescript will not allow that typescript to return undefined. The following code will not compile:

interface Todo {
  id: string;
  title: string;
}

export const todoQueryOption = (id: string | null) =>
  queryOptions({
    queryKey: ['todo', id],
    queryFn: () =>
      Promise.resolve({
        id: '1',
        title: 'Do Laundry',
      }),
    initialData: (): Todo | undefined => undefined,
  });

This should be allowed because I might want to check if I have this particular "todo" item in the cache, and if I don't, I would want angular query to request that item. Since this is conditional, I have to put it in a function that might either have a todo or not (in the example I set it to undefined just for the example).

I believe this has to do with this type definition:

export type DefinedInitialDataOptions<
  TQueryFnData = unknown,
  TError = DefaultError,
  TData = TQueryFnData,
  TQueryKey extends QueryKey = QueryKey,
> = CreateQueryOptions<TQueryFnData, TError, TData, TQueryKey> & {
  initialData:
    | NonUndefinedGuard<TQueryFnData>
    | (() => NonUndefinedGuard<TQueryFnData>)
}

Which should be changed to:

export type DefinedInitialDataOptions<
  TQueryFnData = unknown,
  TError = DefaultError,
  TData = TQueryFnData,
  TQueryKey extends QueryKey = QueryKey,
> = CreateQueryOptions<TQueryFnData, TError, TData, TQueryKey> & {
  initialData:
    | NonUndefinedGuard<TQueryFnData>
    | (InitialDataFunction<TQueryFnData>)
}

Your minimal, reproducible example

https://stackblitz.com/edit/stackblitz-starters-jdj53x?file=src%2Fmain.ts

Steps to reproduce

  1. Write this code in angular query:
interface Todo {
  id: string;
  title: string;
}

export const todoQueryOption = (id: string | null) =>
  queryOptions({
    queryKey: ['todo', id],
    queryFn: () =>
      Promise.resolve({
        id: '1',
        title: 'Do Laundry',
      }),
    initialData: (): Todo | undefined => undefined,
  });
  1. It wouldn't compile.

Expected behavior

The code should compile

How often does this bug happen?

Every time

Screenshots or Videos

No response

Platform

  • OS: Windows
  • Runtime: Node 20.11.0
  • (No browser is used to reproduce this bug)

Tanstack Query adapter

angular-query

TanStack Query version

5.32.0

TypeScript version

5.3.0

Additional context

No response

ShacharHarshuv avatar Apr 26 '24 20:04 ShacharHarshuv

Opened a PR that fixes this: https://github.com/TanStack/query/pull/7351

ShacharHarshuv avatar Apr 29 '24 14:04 ShacharHarshuv

Hi @TkDodo I'm not sure about this one. Code is identical to React adapter and I don't think initialData is supposed to be able to undefined?

arnoud-dv avatar Apr 29 '24 16:04 arnoud-dv

The problematic types are defined in angular-query-experimental.

If an undefined return type is not supported than I'm not sure how the common use case will work. This example (from the docs) currently wouldn't work:

result = injectQuery(() => ({
  queryKey: ['todo', this.todoId()],
  queryFn: () => fetch('/todos'),
  initialData: () => {
    // Use a todo from the 'todos' query as the initial data for this todo query
    return this.queryClient
      .getQueryData(['todos'])
      ?.find((d) => d.id === this.todoId())
  },
}))

Also, this works in react-query:

const x = useQuery({
  initialData: () => undefined,
})

ShacharHarshuv avatar Apr 29 '24 16:04 ShacharHarshuv

it should work, but it also doesn't work in react query:

https://www.typescriptlang.org/play/?#code/JYWwDg9gTgLgBAbzgRwK4FMoE8DyYbAQB2AznAL5wBmUEIcA5AAIwCGpbAxgNYD0U6VpxgBaNJiwMAUFOBEYmKkPRwAKhAAmERFLhxgGgFxwSMKHIDmAbl1wCMADbpjp80WtTyM9AA9IsOE5iUztNCABFDGw8AmI4AF44AAoDFzNLOAAfOCJUBwcASgSAPltxaPxCUiSEWz1yrABpdCxjAG0GGDCGABp9DQBdHrqUKKwAMSJjJKL40r0FuAAFWhBgEnQAOgESCAcAN3QakcXUxgBGXpOF+ydjBgARbQAZVlQiDWwrxb1yAuHTkRgARWA4Hqw2NMCsZ1FosnB3hp0FQ5OgNCUER9kaiNACKAUrEA

I don't think your proposed solution is right though. I think we should rather extend UndefinedInitialDataOptions to be:

export type UndefinedInitialDataOptions<
  TQueryFnData = unknown,
  TError = DefaultError,
  TData = TQueryFnData,
  TQueryKey extends QueryKey = QueryKey,
> = UseQueryOptions<TQueryFnData, TError, TData, TQueryKey> & {
-  initialData?: undefined
-  initialData?: undefined | () => undefined
}

thoughts ?

TkDodo avatar Apr 29 '24 19:04 TkDodo

sorry, that doesn't work. This seems to be the correct diff:

- initialData?: undefined
+ initialData?: undefined | InitialDataFunction<TQueryFnData>

as shown in this reproduction: https://www.typescriptlang.org/play/?#code/JYWwDg9gTgLgBDAnmApnA3nAIgQxjgFRwHMAabFAMxwFcAbGAUSimnIEUaUpEBpFROQCSAO2AxgOOrnwAxGiIDGEiCLgBfOJVYg4AcgAC+EQGd8igNYB6AI5ceAWkXQUegFChIsBMjSYAqiYonNyIAPJgKqYaWjr6Rjim5tZQKDjKDnah7m4oAB5e8EiocP4iACZUwCIo5aLiktJ4OBFRJgA8bnBwBCE8siIyOHAAvHAKFiIQAO4ipF09zKxQoxTU9EwsbAsEQ6u99ogDQ-PdB6H8iHD5MCgVJnB9fAKrT5fzAHyrgcGHrcCqDrnfqDZrkAhLNg9E49N4CL4AMgwC2qDSkQwA-AAucYVKo1cpwAA+cHqEnRzXkSii7WBR1B+A+bnUbjcxTQADlVGVKpRqrUAOI0HBQcq0r5jAjXPK3e643n8wkYuA1ABu3DgOIIrPyhR8JSw+NqZMaQ3+gM6Zyex2aqwmU1mp0WWxWY0N6wYkKgTt2tsl1oZOB9cKuNzu5QeIdeh3ebglpSCT3NplpAZhEJd4PTIcRyO6qPJTXwWIW3RJXJEPKN5SFIrFdJtjNLxLgAApWwBKUZfCtVvkE2ui1OHRs4D4d5k6grQeCUBTKAFqLI8ZMdHZpv24yYzOY7L2rd20T2ZnZ7f0jwPBmMvMNyqNjEOfVsLCCRRcmHHuxUmin4VfD0JR3BL0szBWFr0QD55g7T9qx-IsWjfC0G0vZ1llA-BwRzOAkXQBZl2eRBP2aIhiAAnh3nAwDAyZFlcmnbw52pRc4AI-91wvM8twdXczn3N0qCPTZ0NPTcUJODiLhvGVw0jCDoykwQ42fbpXzaHE+2-MRCzNJCU3EsCM3Q6FDJzaCNLxftjW001mn-AzMLQqFfUculLlzPDugIy5iPwUjyMIrDOOaWipz1JiF1UVi-j0kxWzU98cXtHcu08uBUhgGgoDUBLAUnVlqluKBqEUNACAgcoIDzOBgHKHEzCgapiAAbgWCQYDoFB6pgRqRBa-LdRnOBnCSBAKogJM9NWVtau63riBbER6DoLsRiZLyYraVs0o2xScQAbT0GBxr0chaoAXSdXaQRxTtu2bboAAUdGAIIADpUhMCA6HVbaHu6Gq6v0ABGU7-u6drOpxPQsCqgAZWgKh4MGAYB9QOyumqbN-HBbpgnpxpbBQFQJbt5WrJ10eaoA

TkDodo avatar Apr 29 '24 19:04 TkDodo

okay interestingly, we have test that cover this case already, and they don't fail:

https://github.com/TanStack/query/blob/39b2f81bc782a8633bd601b6d2e35d2a8c2ec4c0/packages/react-query/src/tests/useQuery.test-d.tsx#L79-L91

🤔

TkDodo avatar Apr 29 '24 19:04 TkDodo

I don't think your proposed solution is right though. I think we should rather extend UndefinedInitialDataOptions

When looking at the types more closely I believe you are correct.

According to my understanding the idea is that UndefinedInitialDataOptions will return query result where data is possibly undefined, while DefinedInitialDataOptions returns query result where the data is required, so the consumer wouldn't need to handle the optional undefined value. According to that logic the option of () => T | undefined can result in a query that is possibly undefined, so it should be in the UndefinedInitlaDataOptions (Even though it's confusing, because the initialData is technically "defined").

I'll fix the PR accordingly.

ShacharHarshuv avatar Apr 29 '24 19:04 ShacharHarshuv

okay interestingly, we have test that cover this case already, and they don't fail:

It doesn't fail because you use useQuery directly. The bug (both in angular and react) reproduces when you use queryOptions

ShacharHarshuv avatar Apr 29 '24 19:04 ShacharHarshuv

I fixed the PR per feedback: https://github.com/TanStack/query/pull/7351

ShacharHarshuv avatar Apr 29 '24 19:04 ShacharHarshuv