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

feat(react-query): add typesafe query disabling via skipToken #6999

Merged
merged 40 commits into from
Mar 5, 2024

Conversation

Jaaneek
Copy link
Contributor

@Jaaneek Jaaneek commented Feb 29, 2024

This pull request is heavily inspired by RTK query skipToken.

https://redux-toolkit.js.org/rtk-query/usage/conditional-fetching#overview

We are introducing skipToken which allows typesafe disabling of queries.

  const { data } = useQuery({
      queryKey: ['todos', filter],
      // ⬇️ disabled as long as the filter is undefined or empty
      queryFn: filter ? () => fetchTodos(filter) : skipToken,
  })

Copy link

vercel bot commented Feb 29, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
query ⬜️ Ignored (Inspect) Visit Preview Mar 5, 2024 1:11pm

@Jaaneek Jaaneek changed the title feat(react-query): add typesafe react-query disabling via skipToken feat(react-query): add typesafe query disabling via skipToken Feb 29, 2024
Copy link

codesandbox-ci bot commented Mar 1, 2024

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit efb6b6d:

Sandbox Source
@tanstack/query-example-angular-basic Configuration
@tanstack/query-example-react-basic-typescript Configuration
@tanstack/query-example-solid-basic-typescript Configuration
@tanstack/query-example-svelte-basic Configuration
@tanstack/query-example-vue-basic Configuration

Copy link
Collaborator

@TkDodo TkDodo left a comment

Choose a reason for hiding this comment

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

two things to keep an eye on:

  • it shouldn't be possible to pass a skipToken to suspense versions of the hooks, like useSuspenseQuery, because those always set enabled: true. We should likely forbid this on type level, and at runtime, only set enabled: false if we have a skipToken and enabled isn't already true
  • we should watch out how this influences places where we try to infer the type of what the queryFn returns, because with skipToken, it might not be a function. queryOptions is one of those places, I think we should add a type-test for that.

Copy link

nx-cloud bot commented Mar 1, 2024

☁️ Nx Cloud Report

CI is running/has finished running commands for commit efb6b6d. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution


✅ Successfully ran 2 targets

Sent with 💌 from NxCloud.

@Jaaneek
Copy link
Contributor Author

Jaaneek commented Mar 1, 2024

two things to keep an eye on:

  • it shouldn't be possible to pass a skipToken to suspense versions of the hooks, like useSuspenseQuery, because those always set enabled: true. We should likely forbid this on type level, and at runtime, only set enabled: false if we have a skipToken and enabled isn't already true
  • we should watch out how this influences places where we try to infer the type of what the queryFn returns, because with skipToken, it might not be a function. queryOptions is one of those places, I think we should add a type-test for that.

Is it only for react-query package or should this be implemented in other implementations of query as well? Once we accept queryFn as skipToken it will work like that everywhere

Update: I implemented this on type level for useSuspenseQuery, I will implement type-test, why should we implement this for runtime? To help out people who are doing raw JS?

@Jaaneek Jaaneek requested a review from TkDodo March 1, 2024 21:07
@Jaaneek
Copy link
Contributor Author

Jaaneek commented Mar 2, 2024

@TkDodo

  • useSuspenseQuery is handled
  • added some tests
  • Types are correctly inferred based on function.

Problems:

  • Some tests are failing :D
  • queryOptions function, I could use help with making the return type be accepted by useSuspenseQuery.
image

To consider:
Implementing this only in react-query, will be much easier & a lot less changes. When we pass props we would set enabled to false and that's it. Very similar to what I have done in trpc pull request.
Other packages could implement it in the same way, but core package wouldn't have changes

Copy link
Collaborator

@TkDodo TkDodo left a comment

Choose a reason for hiding this comment

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

regarding queryOptions - does this still work?

const options = queryOptions({
  queryKey: ['key'],
  queryFn: () => Math.random() > 0.5 ? skipToken : Promise.resolve(5),
})

const data = queryClient.getQueryData(options.queryKey)

here, data should be of type number | undefined - inferred from the queryFn. I just want to make sure that the skipToken existence doesn't destroy that.

examples/react/basic/src/index.jsx Outdated Show resolved Hide resolved
packages/query-core/src/infiniteQueryBehavior.ts Outdated Show resolved Hide resolved
packages/query-core/src/query.ts Outdated Show resolved Hide resolved
packages/query-core/src/queryObserver.ts Outdated Show resolved Hide resolved
packages/query-core/src/queryObserver.ts Outdated Show resolved Hide resolved
packages/query-core/src/queryClient.ts Outdated Show resolved Hide resolved
packages/query-core/src/utils.ts Outdated Show resolved Hide resolved
packages/react-query/src/useSuspenseQuery.ts Outdated Show resolved Hide resolved
@Jaaneek
Copy link
Contributor Author

Jaaneek commented Mar 2, 2024

regarding queryOptions - does this still work?

const options = queryOptions({
  queryKey: ['key'],
  queryFn: () => Math.random() > 0.5 ? skipToken : Promise.resolve(5),
})

const data = queryClient.getQueryData(options.queryKey)

here, data should be of type number | undefined - inferred from the queryFn. I just want to make sure that the skipToken existence doesn't destroy that.

This works:

image

Should we also handle returning the skipToken from a function? I think it's not "needed"?

@TkDodo
Copy link
Collaborator

TkDodo commented Mar 2, 2024

Oh Great 👍
No I also don't think this is needed

@TkDodo
Copy link
Collaborator

TkDodo commented Mar 4, 2024

the should respect skipToken test is constantly failing in CI, but it works fine locally. I don't know why :/

@github-actions github-actions bot added the documentation Improvements or additions to documentation label Mar 4, 2024
@Jaaneek
Copy link
Contributor Author

Jaaneek commented Mar 5, 2024

@TkDodo I found potential problem when fixing the should respect skipToken test

The queryKeys I used were already declared in another test.
I fixed the test by simply changing the queryKeys to unique name.
image

Maybe we should do cleanup after every test to ensure that the queryKeys are not shared? I don't think there is a use in sharing them

@TkDodo
Copy link
Collaborator

TkDodo commented Mar 5, 2024

Maybe we should do cleanup after every test to ensure that the queryKeys are not shared? I don't think there is a use in sharing them

yeah that's what the queryKey() function is for. it increments a value. no test should use hardcoded keys

@TkDodo TkDodo merged commit 77d5b2e into TanStack:main Mar 5, 2024
5 checks passed
@hichemfantar
Copy link

hichemfantar commented Mar 20, 2024

I can't find the documentation for skipToken.
I only discovered this thanks to TRPC
image

@Jaaneek
Copy link
Contributor Author

Jaaneek commented Mar 21, 2024

I can't find the documentation for skipToken.

I only discovered this thanks to TRPC

image

https://tanstack.com/query/latest/docs/framework/react/guides/disabling-queries

@hichemfantar
Copy link

hichemfantar commented Mar 21, 2024

@Jaaneek thanks! any idea as to why skipToken isn't search indexed?

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

Successfully merging this pull request may close these issues.

None yet

5 participants