Open vezaynk opened 5 months ago
Hi @vezaynk 👋
Can it be expanded to cover all query types?
We agree skipToken
is a powerful API and have considered adding support for it in useQuery
, but we don't have any official plans there yet. I'm not sure if you had something else/in addition in mind? Thanks for the feedback here.
@alessbell Is there a reason against it? Currently I'm writing a lot of code that looks like this:
const maybeVar: string | null;
useQuery({
variables: {
myVar: maybeVar as string // this cast shouldn't be necessary
},
skip: !maybeVar
})
code like this is better:
const maybeVar: string | null;
useQuery({
variables: maybeVar ? {
myVar: maybeVar
} : skipToken,
})
We're big fans of skipToken
too and wholeheartedly agree this pattern has significant DX benefits :)
We first introduced skipToken
in the Suspense-enabled data fetching hooks in 3.8 as you noted, and we're about to ship 3.9 but this is useful signal as we plan 3.10 and beyond.
@alessbell Is there a reason against it? Currently I'm writing a lot of code that looks like this:
const maybeVar: string | null; useQuery({ variables: { myVar: maybeVar as string // this cast shouldn't be necessary }, skip: !maybeVar })
code like this is better:
const maybeVar: string | null; useQuery({ variables: maybeVar ? { myVar: maybeVar } : skipToken, })
What you could do to avoid the nasty cast, is conditionally change the variables object:
const maybeVar: string | null;
useQuery(QUERY, {
variables: maybeVar ? {
myVar: maybeVar
} : undefined,
skip: !maybeVar
})
// Or:
useQuery(QUERY, maybeVar ? {
variables: {
myVar: maybeVar
},
} : { skip: true })
It's definitely better if you have multiple variables, but I think the skipToken is a much better approach.
EDIT: See my next comment for a better workaround
I think a good discussion would be where skipToken
is accepted? Consider the following:
useQuery(QUERY, {
variables: maybeVar ? {
myVar: maybeVar
} : skipToken,
})
useQuery(QUERY, maybeVar ? {
variables: {
myVar: maybeVar
},
} : skipToken)
The reason the latter approach is better is because it would make non-variable queries skippable with skipToken
:
useQuery(QUERY, !maybeVar ? skipToken : undefined)
// Otherwise you end up with this nasty syntax for non-variable queries:
useQuery(QUERY, { variables: !maybeVar ? skipToken : {} }) // Doesn't make much sense
But that opens the question for the following pattern:
// Somewhat identical to RTK-Query.
useQuery(maybeVar ? QUERY : skipToken)
But then again, that's why we have the skip
option:
useQuery(QUERY, { skip: !maybeVar })
It's not realistic to add all the above accepts for skipToken, so my proposal would be either only as the whole options object (worse DX for non-variable queries), or both the options object and query object (good DX for both cases). For type-safety, having the options object accept skipToken is the only valid syntax, the query skipToken is just syntactic sugar.
If we support both, it should skip if at least any of the arguments have skipToken:
useQuery(skipToken, { variables: { ... } }) // Skip
useQuery(QUERY, skipToken) // Skip
useQuery(skipToken, skipToken) // Skip
useQuery(QUERY, { variables: { ... } })) // Don't skip
Perhaps unnecessary complexity, so for non-variable queries, stick with { skip: true }
.
This form opens up a good point:
useQuery(QUERY, maybeVar ? {
variables: {
myVar: maybeVar
},
} : { skip: true })
Because you can simply define skipToken
yourself and be on your way:
export const skipToken = { skip: true }
useQuery(QUERY, maybeVar ? {
variables: {
myVar: maybeVar
},
} : skipToken)
That said, skipToken
should be a symbol
, implemented by Apollo.
@AlexanderArvidsson for consistency reasons, we'll make skipToken
a valid value for the entire options object. That way it will mimic the API on the suspense hooks.
useQuery(QUERY, skipToken) // valid
useQuery(QUERY, id ? { variables: { id } } : skipToken) // valid
useQuery(skipToken) // invalid
useQuery(QUERY, { variables: skipToken }) // invalid
The reason we went with this approach is to provide better TypeScript errors when your query has required variables and you haven't provided them. We should require you to define variables
in options, otherwise your query is invalid. This is something the suspense hooks do, but unfortunately useQuery
just doesn't support super well today. We want to do this work before we add support for skipToken
to provide the best experience.
For that reason, we don't suggest this approach:
useQuery(QUERY, maybeVar ? {
variables: {
myVar: maybeVar
},
} : { skip: true })
Once we get this work done, the above would be invalid (assuming myVar
is required) since you aren't providing a variables
option in your "else" case.
We have already started this work in https://github.com/apollographql/apollo-client/pull/11241, but need some time to come back to it (6 million priorities don't help 😆). As @alessbell said, we're big fans of this pattern and its more of "when" not an "if" at this point.
@jerelmiller I absolutely agree! I definitely think the right path is the full options object, for type safety and consistency reasons.
My approach I added at the end was as a workaround that works today but shouldn't work in the future, and people could define that themselves in their code until we have proper skipToken support. Then, it would simply be switching the import to the proper skipToken.
For that reason, we don't suggest this approach:
useQuery(QUERY, maybeVar ? { variables: { myVar: maybeVar }, } : { skip: true })
Once we get this work done, the above would be invalid (assuming
myVar
is required) since you aren't providing avariables
option in your "else" case.
@jerelmiller could you further explain your point here? You still aren't providing a variables
option in your "else" case if you use skipToken
instead of { skip: true }
?
In a project I work on we use @graphql-codegen/typescript-react-apollo
, which currently doesn't support skipToken
, so I'm trying to think through the best way to accomplish type safety without skipToken
as an interim solution.
What is so wrong with the example above in the meantime?
Hey @jrnxf 👋
Great question! We want to do a bit of prework to the types on useQuery
so that if your query has required variables, we force you to provide them. useQuery
is unfortunately too permissive today doesn't error if you don't pass required variables. This results in a query failure at runtime. We'd love to catch this at the TypeScript level instead.
As a result of that change, this means that the variables
option is going to be required in options if your query has a required variable. When that change lands, the useQuery
hook in that code snippet is going to be invalid because { skip: true }
doesn't include variables
. Allowing variables
to be optional when skip
is true adds quite a bit of TypeScript complexity, so this is likely something we won't do and instead put this complexity into skipToken
instead.
I'm simply saying that we don't recommend that form of the ternary options for useQuery
because once we ship the updates to the types, you'll have to rewrite the hook. If you're ok with rewriting that hook when the change lands, then by all means, feel free to use that form for now. Hope that helps!
Currently the documentation says that skiptokens are for suspense queries: https://github.com/apollographql/apollo-client/blob/038629c62bf0d4665b52d9fade3fb88861ae1194/docs/source/api/react/hooks.mdx#L669
Can it be expanded to cover all query types?