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

Introduce skipToken to useQuery #11524

Open
vezaynk opened this issue Jan 25, 2024 · 9 comments
Open

Introduce skipToken to useQuery #11524

vezaynk opened this issue Jan 25, 2024 · 9 comments

Comments

@vezaynk
Copy link
Contributor

vezaynk commented Jan 25, 2024

Currently the documentation says that skiptokens are for suspense queries:

While not a hook by itself, `skipToken` is designed to be used with `useSuspenseQuery` and `useBackgroundQuery`.

While not a hook by itself, skipToken is designed to be used with useSuspenseQuery and useBackgroundQuery.

Can it be expanded to cover all query types?

@alessbell
Copy link
Member

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.

@vezaynk
Copy link
Contributor Author

vezaynk commented Jan 25, 2024

@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,
})

@alessbell
Copy link
Member

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.

@AlexanderArvidsson
Copy link

AlexanderArvidsson commented Feb 3, 2024

@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 }.

@AlexanderArvidsson
Copy link

AlexanderArvidsson commented Feb 3, 2024

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.

@jerelmiller
Copy link
Member

@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 #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.

@AlexanderArvidsson
Copy link

AlexanderArvidsson commented Feb 6, 2024

@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.

@jrnxf
Copy link

jrnxf commented Apr 26, 2024

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.

@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?

@jerelmiller
Copy link
Member

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants