rtk-incubator / rtk-query

Data fetching and caching addon for Redux Toolkit
https://redux-toolkit.js.org/rtk-query/overview
MIT License
626 stars 31 forks source link

make `arg: ... | undefined` if `{skip: true}` in useQuery #220

Closed StefanBRas closed 3 years ago

StefanBRas commented 3 years ago

As discussed with @phryneas on discord, it could make sense to allow the type of arg to be undefined when {skip: true}, to make it easier for useQuery hooks to conditionally depend on some other state.

My initial question/use case/context directly copied from Discord because i'm lazy like that was:

"Using RTK-query, what's the correct way to conditionally use a useQuery hook in TypeScript? Say a hook uses a string as input, but I would like it to depend on another useQuery. I came up with two solutions, neither of which seems very nice:

const ConditionalQuery = (userName: string) => {
  const {data: userId} = useBingoQuery(userName)
  const {data: userSessionToken} = usePapayaQuery(userId ?? 'anyStringHereIGuess?', {skip: userId === undefined})
  return (
    <>
    {userSessionToken && <ComponentDependentOnUserSessionToken token={userSessionToken}/>}
    </>
  )
}

Or:

const ConditionalQueryToken = (userId: string) => {
  const {data: userSessionToken} = usePapayaQuery(userId)
  return (<> {userSessionToken && <ComponentDependentOnUserSessionToken token={userSessionToken} />} </>)
}

const ConditionalQuery = (userName: string) => {
  const {data: userId} = useBingoQuery(userName)
  return (<> {userId && <ConditionalQueryToken userId={userId}>} </>)
}

In Solution 1 it seems kinda arbitrary with just using a random value to satisfy the type and Solution 2 can quickly become super nested if you need to do several (classic use case from where I also stole the query names: https://www.youtube.com/watch?v=y8OnoxKotPQ)

Or would it just be better to force the typechecker to ignore these cases?"

To which @phryneas responded:

pretty much the first might actually be a good idea to make the arg an |undefined if {skip:true} is provided.

omerman commented 3 years ago

@StefanBRas I was not present at the discord chat, but I think the easiest solution here would be to call usePapayaQuery(userId!, {skip: userId === undefined}) Or if we want to be perfectly strict, maybe ask the RTK team to make the query hook also accept a function as the first argument, like so: usePapayaQuery(() => userId!, {skip: userId === undefined}), and off course the function will not be executed when the skip option is set to true.

... import {skipArg} from 'rtk-query';
... usePapayaQuery(userId ?? skipArg) // when providing the skipArg, the {skip: true} is not required as it can be inferred by rtk easily.

What do you think?

phryneas commented 3 years ago

@omerman from runtime code, it is already completely okay to have useQuery(foo, skip: !foo), as the endpoint.query function will not be called in the skip case.

This is just a request for an adjustment of our typings and will not require any api changes :)

We'll add that in the next version of RTKQ.

phryneas commented 3 years ago

On further inspection, unfortunately TypeScript does not have the code flow inference that would be necessary for this feature, so we cannot implement it.

Might revisit it in a future TS version if something changes on their end.

image

StefanBRas commented 3 years ago

Hey again, thanks for trying to implement it. I tried it myself just now and ran in to the same problems.

I think my solution will be to just do arg! as @omerman suggested. This will come up a lot with the way i'm using RTK query - is it something that should be added to the docs or does it mean that I'm not following good practices in some way since it comes up so much? I can make a PR to add something to the docs if you think that's a good idea.

Additionally, would it make sense to allow arg to be undefined if the options object has the skip attribute? Then it's up to user themselves to make sure that arg is only undefined when skip === true.

type Arg = number
type UseQueryOptions = {option?: number}
type UseQueryOptionsWithSkip = UseQueryOptions & {skip: boolean}

function query<A extends Arg, B>(a: B extends UseQueryOptionsWithSkip ? A | undefined : A, b: B): A | undefined {
    return a
}

const testArg = 1 as number | undefined

query(testArg, {}) // error ✅
query(testArg, {skip: true}) // no error ✅
query(testArg, {skip: false}) // no error ❌ 

You could also tighten it and say that skip has to be true and then let the user force that themselves:

type UseQueryOptionsWithSkipAsTrue = UseQueryOptions & {skip: true}

function query<A extends Arg, B>(a: B extends UseQueryOptionsWithSkipAsTrue ? A | undefined : A, b: B): A | undefined {
    return a
}

const testArg = 1 as number | undefined

query(testArg, {}) // error ✅
query(testArg, {skip: true}) // error ❌
query(testArg, {skip: false}) // error ✅
query(testArg, {skip: true as true}) // no error ✅
query(testArg, {skip: true as const}) // no error ✅
omerman commented 3 years ago

@StefanBRas @phryneas I still think that RTK Query should support a sort of skip token, that can be placed as first argument.

... import {skipToken} from 'rtk-query';
... usePapayaQuery(userId ?? skipToken) // when providing the skipArg, the {skip: true} is not required as it can be inferred by rtk easily.

And maybe also when dealing with multiple args to accept that one arg will be the skip token.. and skip the execution as well..

usePapayaQuery(arg1 ?? skipToken, arg2 ?? skipToken, arg3 ?? skipToken) // If one of the args is skip, skip the entire execution as well
phryneas commented 3 years ago

This will be adressed in https://github.com/reduxjs/redux-toolkit/pull/1056