feat(query-core): add 'fetchedPages' option

[aliakbarazizi]

Related discussions, https://github.com/TanStack/query/discussions/4286.

Also i'm not sure the way I approach nist, I added a new state called meta, since fetchMeta will be overwrite in every fetch and there is no way to access to old value in behavior.

[codesandbox-ci[bot]]

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 79f462c0a7223d26d25e63622869540ddeb8f239:

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

[TkDodo]

hmm it seems that this breaks lots of existing tests 🤔

[aliakbarazizi]

I didn't test I just want share my idea, because I'm new to this library and I wasn't sure my way is a good way, I will check test and update test

[aliakbarazizi]

I did check some of the test there is two main issue one of them checking how many re-render call and it expect 2 but because of fetchedPages is 3 now I think I just need change 2 to 3 since as you say we tracked property for re-render and it should be 2 without getting fetchedPages, I will add proper test for it but i just want to be sure am i correct?

the another one is about size of states and I need to change it in all test becasue the size of state is increased by 1

[TkDodo]

one of them checking how many re-render call and it expect 2 but because of fetchedPages is 3 now I think I just need change 2 to 3 since as you say we tracked property for re-render and it should be 2 without getting fetchedPages, I will add proper test for it but i just want to be sure am i correct?

the way I see it, with tracked properties being on per default, there shouldn't be additional re-renders of the component even if we dispatch more events during fetching. But it seems we get one additional re-render per each update...

there is even one tests that has:

Expected: 8
Received: 14

[aliakbarazizi]

In this test It's expected, in this test we fetch 3 pages and after all of that we refetch all pages again, so we fetch 6 pages, and we have one more re-render pages.

[aliakbarazizi]

Actually you are right, in the test we don't call fetchedPages I will check it soon

[TkDodo]

In this test It's expected, in this test we fetch 3 pages and after all of that we refetch all pages again, so we fetch 6 pages, and we have one more re-render pages.

not sure I understand that. Unless the component is using the newly exposed counter, there shouldn't be any additional re-render ...

[aliakbarazizi]

one of them checking how many re-render call and it expect 2 but because of fetchedPages is 3 now I think I just need change 2 to 3 since as you say we tracked property for re-render and it should be 2 without getting fetchedPages, I will add proper test for it but i just want to be sure am i correct?

the way I see it, with tracked properties being on per default, there shouldn't be additional re-renders of the component even if we dispatch more events during fetching. But it seems we get one additional re-render per each update...

there is even one tests that has:

Expected: 8
Received: 14

I checked the example you provided

Expected: 8
Received: 14

I think thats because of this line, it broke tacked property feature https://github.com/TanStack/query/blob/main/packages/solid-query/src/tests/createInfiniteQuery.test.tsx#L445 this line will track to all property and fetchedPages property will cause additional render per fetching page not per fetch.

And I think I have to change await waitFor(() => expect(states.length).toBe(8)) to await waitFor(() => expect(states.length).toBe(14)) in these kind of situations. or change states.push({ ...state }) to states.push(state) but if I do that I need check all counter and in some situation it should be less than expected.

Can you check and see am I right? if so which is the proper way to fix it?

here is more example

https://github.com/TanStack/query/blob/main/packages/solid-query/src/tests/createInfiniteQuery.test.tsx#L63 https://github.com/TanStack/query/blob/main/packages/solid-query/src/tests/createInfiniteQuery.test.tsx#L209 https://github.com/TanStack/query/blob/main/packages/solid-query/src/tests/createInfiniteQuery.test.tsx#L295

Its only in solid-query and I'm not familiar with solid so maybe there is a something that I'm missing.

There is also one test with notifyOnChangeProps set to all https://github.com/TanStack/query/blob/main/packages/react-query/src/tests/useInfiniteQuery.test.tsx#L581

I fixed other tests.

[TkDodo]

yeah the one that has notifyOnChangeProps: 'all' will need to be updated, that's for sure.

I have to say I don't know about the solid tests. I hope that @ardeora can have a look?

[TkDodo]

there's one more react-query tests that needs changes:

FAIL packages/react-query/src/__tests__/useInfiniteQuery.test.tsx
  ● useInfiniteQuery › should return the correct states for a successful query

    expect(received).toEqual(expected) // deep equality

    - Expected  - 0
    + Received  + 1

    @@ -13,10 +13,11 @@
        "errorUpdatedAt": 0,
        "failureCount": 0,
        "fetchNextPage": Any<Function>,
        "fetchPreviousPage": Any<Function>,
        "fetchStatus": "idle",
    +   "fetchedPages": 1,
        "hasNextPage": true,
        "hasPreviousPage": undefined,
        "isError": false,
        "isFetched": true,
        "isFetchedAfterMount": true,

and there are some more failing tests that have notifyOnChangeProps: 'all', which would be fine to adapt

[aliakbarazizi]

there's one more react-query tests that needs changes:

FAIL packages/react-query/src/__tests__/useInfiniteQuery.test.tsx
  ● useInfiniteQuery › should return the correct states for a successful query

    expect(received).toEqual(expected) // deep equality

    - Expected  - 0
    + Received  + 1

    @@ -13,10 +13,11 @@
        "errorUpdatedAt": 0,
        "failureCount": 0,
        "fetchNextPage": Any<Function>,
        "fetchPreviousPage": Any<Function>,
        "fetchStatus": "idle",
    +   "fetchedPages": 1,
        "hasNextPage": true,
        "hasPreviousPage": undefined,
        "isError": false,
        "isFetched": true,
        "isFetchedAfterMount": true,

and there are some more failing tests that have notifyOnChangeProps: 'all', which would be fine to adapt

I did change all of them I just need confirmation about notifyOnChangeProps, I'll push it as soon as possible.

[aliakbarazizi]

I did merge meta with fetchMeta, I also fixed tests and add one test specifically for fetchedPages.

I have weird problem in local with userQuery tests.

I pushed my code to see the github testings result too, I think my problem is local but still I need to see.

[ardeora]

I took a look at this PR. It seems like most (all?) solid tests fail because solid-query resolves queryObserver updates a little differently than react-query. Take a look at this createInfiniteQuery function:

const query = createInfiniteQuery(
  () => ['pages'],
  ({ pageParam = 20 }): number => pageParam,
  {
    initialData: { pages: [10], pageParams: [undefined] },
    getNextPageParam: () => undefined,
  },
)

When the query is mounted. These are the results queryObserver initialises and updates with consecutively -

// Initial Result
{ data:  {pages: [10], pageParams: [undefined] },  fetchStatus: 'fetching', fetchedPages: 0 }
// Update
{ data:  {pages: [10], pageParams: [undefined] },  fetchStatus: 'fetching', fetchedPages: 1 }
// Update
{ data:  {pages: [20], pageParams: [undefined] },  fetchStatus: 'idle', fetchedPages: 1 }

Now react-query batches the 'Updates'

// Update
{ data:  {pages: [10], pageParams: [undefined] },  fetchStatus: 'fetching', fetchedPages: 1 }
// Update
{ data:  {pages: [20], pageParams: [undefined] },  fetchStatus: 'idle', fetchedPages: 1 }

by using a using a setTimeout to defer the commit after all macrotasks have been executed. We can see the definition for it here https://github.com/TanStack/query/blob/main/packages/query-core/src/utils.ts#L413-L415.

solid-query however batches these updates queuing a microtask https://github.com/TanStack/query/blob/main/packages/solid-query/src/createBaseQuery.ts#L81-L87 which performs updates between macrotasks. This is the reason why we see 3 query state updates instead of 2 in solid-query. @TkDodo any idea why the fetchedPages property is updated differently than other state updates?

[aliakbarazizi]

I took a look at this PR. It seems like most (all?) solid tests fail because solid-query resolves queryObserver updates a little differently than react-query. Take a look at this createInfiniteQuery function:

const query = createInfiniteQuery(
  () => ['pages'],
  ({ pageParam = 20 }): number => pageParam,
  {
    initialData: { pages: [10], pageParams: [undefined] },
    getNextPageParam: () => undefined,
  },
)

When the query is mounted. These are the results queryObserver initialises and updates with consecutively -

// Initial Result
{ data:  {pages: [10], pageParams: [undefined] },  fetchStatus: 'fetching', fetchedPages: 0 }
// Update
{ data:  {pages: [10], pageParams: [undefined] },  fetchStatus: 'fetching', fetchedPages: 1 }
// Update
{ data:  {pages: [20], pageParams: [undefined] },  fetchStatus: 'idle', fetchedPages: 1 }

Now react-query batches the 'Updates'

// Update
{ data:  {pages: [10], pageParams: [undefined] },  fetchStatus: 'fetching', fetchedPages: 1 }
// Update
{ data:  {pages: [20], pageParams: [undefined] },  fetchStatus: 'idle', fetchedPages: 1 }

by using a using a setTimeout to defer the commit after all macrotasks have been executed. We can see the definition for it here https://github.com/TanStack/query/blob/main/packages/query-core/src/utils.ts#L413-L415.

solid-query however batches these updates queuing a microtask https://github.com/TanStack/query/blob/main/packages/solid-query/src/createBaseQuery.ts#L81-L87 which performs updates between macrotasks. This is the reason why we see 3 query state updates instead of 2 in solid-query. @TkDodo any idea why the fetchedPages property is updated differently than other state updates?

fetchedPages change on every page fetched. this action will dispatch in Promise that suppose to run fetching and all of other state is changing outside of this Promise so that's why, but I don't see anyway to do that without this behavior

[aliakbarazizi]

@ardeora shouldn't tracked works fine in solid? if it's working then we shouldn't have any state changes because there is no subscribe to fetchedPages in those tests.

Also I don't think the result from react is true, batching should not work here. I also created a test for this behavior

https://github.com/TanStack/query/blob/7ae9cb238de26f3fc266f91d7d78d2b8ead1c67b/packages/react-query/src/tests/useInfiniteQuery.test.tsx#L1859

[TkDodo]

@ardeora I think you might be looking in the wrong direction. The scheduleMicrotask is done in the query-core, which solid-query also uses. The setTimeout is admittedly a bit hacky - ideally, we would just use the browser-native queueMicrotask, but a lot of tests failed (more in-between re-renders) so I didn't do that.

what react-query additionally does is that we are using our notifiyManager.setBatchNotifyFunction and set it to unstable_batchedUpdates from react-dom (or react-native respectively):

https://github.com/TanStack/query/blob/357ec041a6fcc4a550f3df02c12ecc7bcdefbc05/packages/react-query/src/setBatchUpdatesFn.ts#L1-L4

We use this function in the query-core when invoking callbacks, but also in useBaseQuery to batch updates coming from react:

https://github.com/TanStack/query/blob/34ca5581217dae4133046f6cfbc746a22be14dd9/packages/react-query/src/useBaseQuery.ts#L88

If the solid way to do this is via the queueMicrotask you've pointed towards, I would still suggest to call notifyManager.setBatchNotifyFunction somewhere to this custom function, and then use notifyManager.batchCalls when you create the observer subscription.

@ardeora shouldn't tracked works fine in solid? if it's working then we shouldn't have any state changes because there is no subscribe to fetchedPages in those tests.

This is a good question. property tracking is part of the query-core 🤔

[aliakbarazizi]

@ardeora @TkDodo any updates?

[TkDodo]

@aliakbarazizi there are still some open questions from my side: https://github.com/TanStack/query/pull/4295#pullrequestreview-1138764366

[aliakbarazizi]

@TkDodo I did answered all of them before. I don't know my answer is enough or need an action to fix them

[ardeora]

@ardeora @TkDodo any updates?

Yeah I have spent a few hours today trying to get notifyManager working with solid-query but it seems to break even more tests than without it. I think I might try something different tomorrow to switch to notifyManager.setBatchNotifyFunction which might help with this PR too

[aliakbarazizi]

@ardeora can you tell me why we using states.push({ ...state }) instead of states.push(state)? I think this break the tests. when we use ... its look like we set notifyOnChangeProps: 'all'

if you look at react test it use states.push(state) too.

https://github.com/TanStack/query/blob/main/packages/solid-query/src/tests/createInfiniteQuery.test.tsx#L63

[TkDodo]

I'm not sure how to continue with this PR. In the original discussion, we talked about how displaying a loading indicator for refetches of all pages (e.g. all 20 in the cache) would be too long. In v5, we've introduced the maxPages option, which can limit how many pages are kept in the cache. Maybe this is the better fix for the underlying problem? If you limit the number of pages in the cache, you can continue to display the loading spinner until all pages are refetched, because you can control what "all pages" means. Also, you can limit the number of refetches happening this way.

[aliakbarazizi]

I'm not sure how to continue with this PR. In the original discussion, we talked about how displaying a loading indicator for refetches of all pages (e.g. all 20 in the cache) would be too long. In v5, we've introduced the maxPages option, which can limit how many pages are kept in the cache. Maybe this is the better fix for the underlying problem? If you limit the number of pages in the cache, you can continue to display the loading spinner until all pages are refetched, because you can control what "all pages" means. Also, you can limit the number of refetches happening this way.

Sorry for late respond, the issue was when you want to show 2 different loading ( loading for first page refetching, and loading for other page ) The purpose of this pull request is not limit number of refetch pages, its provide a way to know which page is refetching currently.

I don't think maxPages is good alternative, because its only limit number of page to refetch, but what if you want to refetch all pages and still show different loading for first page refetching?

Also it may be better to tell why sometime you need 2 different loading, if you create pull down to refetch ability (something like twitter in webapp) you need a loading when fetching new data (not syncing old data) at the top of screen, but when user scrolling to see more pages you need a loading at bottom of screen to show we are fetching new pages

[aliakbarazizi]

Also its worse to mention this pull request was completely ready, the only issue remain is solid test case and as I mention above it seems the issue is how solid test are, and the issue is not because of my codes