Skip to content

Feature/infinite queries #5004

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

Merged
merged 27 commits into from
Feb 27, 2023
Merged

Feature/infinite queries #5004

merged 27 commits into from
Feb 27, 2023

Conversation

TkDodo
Copy link
Collaborator

@TkDodo TkDodo commented Feb 19, 2023

No description provided.

@vercel
Copy link

vercel bot commented Feb 19, 2023

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

1 Ignored Deployment
Name Status Preview Comments Updated
query ⬜️ Ignored (Inspect) Feb 27, 2023 at 9:34AM (UTC)

@codesandbox-ci
Copy link

codesandbox-ci bot commented Feb 19, 2023

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 3bb0c34:

Sandbox Source
@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

@codecov-commenter
Copy link

codecov-commenter commented Feb 19, 2023

Codecov Report

Base: 90.50% // Head: 90.41% // Decreases project coverage by -0.09% ⚠️

Coverage data is based on head (3bb0c34) compared to base (ca2f697).
Patch coverage: 100.00% of modified lines in pull request are covered.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@            Coverage Diff             @@
##            alpha    #5004      +/-   ##
==========================================
- Coverage   90.50%   90.41%   -0.09%     
==========================================
  Files         104      104              
  Lines        3811     3788      -23     
  Branches      961      948      -13     
==========================================
- Hits         3449     3425      -24     
- Misses        329      330       +1     
  Partials       33       33              
Impacted Files Coverage Δ
packages/query-core/src/query.ts 99.02% <ø> (ø)
packages/react-query/src/useInfiniteQuery.ts 100.00% <ø> (ø)
packages/solid-query/src/createInfiniteQuery.ts 100.00% <ø> (ø)
packages/svelte-query/src/createInfiniteQuery.ts 0.00% <ø> (ø)
packages/vue-query/src/queryClient.ts 93.22% <ø> (ø)
packages/vue-query/src/useBaseQuery.ts 100.00% <ø> (ø)
packages/vue-query/src/useInfiniteQuery.ts 100.00% <ø> (ø)
packages/vue-query/src/useQuery.ts 100.00% <ø> (ø)
packages/query-core/src/infiniteQueryBehavior.ts 98.59% <100.00%> (-1.41%) ⬇️
packages/query-core/src/infiniteQueryObserver.ts 88.46% <100.00%> (-0.43%) ⬇️
... and 3 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

TkDodo and others added 4 commits February 19, 2023 13:29
* feat: add PageParam typing

* feat(infinite-query): more typing

* feat(infinite-query): make pageParam never by defaukt

* feat(infinite-query):  PageParam type unknown for infinite queries

* fix(infinite-query): fix tests

* feat(infinite-query): add previous end next return TPageParam type
@TkDodo TkDodo marked this pull request as ready for review February 19, 2023 15:01
@TkDodo TkDodo requested a review from DamianOsipiuk February 19, 2023 15:04
Copy link
Contributor

@DamianOsipiuk DamianOsipiuk left a comment

Choose a reason for hiding this comment

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

Hmmm, do we still need pageParams in return of useInfiniteQuery? 🤔

For what it can be usefull?

because it might be mistaken for a full set of options, which it is not
@TkDodo
Copy link
Collaborator Author

TkDodo commented Feb 22, 2023

@DamianOsipiuk

Hmmm, do we still need pageParams in return of useInfiniteQuery? 🤔

For what it can be usefull?

I outlined the problems in the initial RFC. We still need the pageParams to trigger refetches in some cases. As an example, suppose we have the following query:

useInfiniteQuery({
  queryKey,
  queryFn: ({ pageParam }) => Promise.resolve({ num: pageParam }),
  defaultPageParam: 0
  getNextPage: (lastPage) => lastPage.num + 1,
  getNextPage: (firstPage) => firstPage.num - 1,
})

this will start out with:

{
  pages: [{ num: 0 }],
  pageParam: [0]
}

now let's fetch some more in the forward direction:

{
  pages: [{ num: 0 }, { num: 1 }, { num: 2 }],
  pageParam: [0, 1, 2]
}

so far, so good. If a refetch happens now, it always starts at the beginning and then refetches all pages in order. so we:

  • take 0 from the pageParams to initiate the refetches
  • for every subsequent page, we just call getNextPageParam again to re-compute it on the fly.

Now it seems, we could just re-use the defaultPageParam and start with that, but we cannot, for two reasons:

  1. what if we have fetched in the previous direction before, and our state is:
{
  pages: [{ num: -1 }, { num: 0 }, { num: 1 }, { num: 2 }],
  pageParam: [-1, 0, 1, 2]
}

we now have to start the fetch chain with -1 and not with 0. We only have that info in pageParams.

  1. in the same way, if you use the new pageSize option and set it to 2, you might be in a situation where you no longer have the "initial" page in the cache, we might only have:
{
  pages: [{ num: 1 }, { num: 2 }],
  pageParam: [1, 2]
}

now, we would have to start with 1.

Grabbing this info from the pageParam works well, but yeah, it's the only reason we have this extra nesting structure :/ It happens right here and is the only place where we need pageParams:

promise = fetchPage([], oldPageParams[0])

Alternatively, we would have to store the information "what's the pageParam of the current first page" somewhere on the query. Would that be better?
I also thought about making defaultPageParam a function that is executed with allPages | undefined, but is there a guarantee that users can always compute that value just from looking at the pages ?

@DamianOsipiuk
Copy link
Contributor

@TkDodo But it seems that it's now more like an implementation detail, rather than something users could use.

If it's not usefull for users it would be good to hide this somewhere. And what we really need is something like firstPageParam internally.
Removing pageParams would make using select and cache manipulation easier.

But question is, how would we store this firstPageParam with persisters? Or do we even need to do that?

Base automatically changed from v5 to alpha February 26, 2023 10:01
@TkDodo
Copy link
Collaborator Author

TkDodo commented Feb 26, 2023

But it seems that it's now more like an implementation detail, rather than something users could use.

yes, I agree.

Removing pageParams would make using select and cache manipulation easier.

cache manipulation, yes. select, kinda. It should be possible to transform to anything via select. it works at runtime, but not on type level. I've added some failing type level tests. I think we should fix them first before changing the structure.

But question is, how would we store this firstPageParam with persisters? Or do we even need to do that?

yes, we would need to store it. it could be part of the query.state, as we store more than just the data anyways:

export interface QueryState<TData = unknown, TError = RegisteredError> {
data: TData | undefined
dataUpdateCount: number
dataUpdatedAt: number
error: TError | null
errorUpdateCount: number
errorUpdatedAt: number
fetchFailureCount: number
fetchFailureReason: TError | null
fetchMeta: any
isInvalidated: boolean
status: QueryStatus
fetchStatus: FetchStatus
}

I will need to look into it. We don't have an infinite version of that query. Maybe storing it on fetchMeta works, idk 😅

@TkDodo
Copy link
Collaborator Author

TkDodo commented Feb 26, 2023

hmm, it's mostly an implementation detail, but when working directly with the cache, you'd still need to set it. As an example, assume you have 3 pages:

{
  pages: [{}, {}, {}]
  pageParams: [5, 10, 15]
}

if we take away pageParams from data and store them somewhere else (on queryState), what happens if you call setQueryData and remove the first page? You will also need to adapt pageParams so that it only contains [10, 15]. We can only do that because pageParams are part of our Infinite Data structure. So it seems that exactly the use-cases where pageParams becomes annoying are actually the ones that might need it ...

TkDodo and others added 3 commits February 26, 2023 12:00
* feat(select): defer TData inference

* test(select): restore test

* fix(select): change InfiniteQuery Observer definition

* fix(infinite-queries): fix solid type inference select

* fix(infinite-queries): fix solid type inference select

* chore(infinite-queries): fix prettier

* fix(infinite-queries): fix vue and svelte types

* fix(infinite-queries): fix react type inference select

* chore(infinite-queries): fix imports

* style: fix prettier warnings
@TkDodo
Copy link
Collaborator Author

TkDodo commented Feb 27, 2023

@ardeora I'll merge this PR into alpha because it contains some good improvements on the API. If we find a way to get rid of the pageParams as part of data, we can still do this separately.

@TkDodo TkDodo merged commit 9be63f4 into alpha Feb 27, 2023
@TkDodo TkDodo deleted the feature/infinite-queries branch February 27, 2023 10:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants