`fetchNextPage` always triggering `onQueryStarted`, even when `queryFulfilled` already occured.

[JacobJaffe]

It looks like the entire lifecycle of onQueryStarted occurs when fetchNextPage is called.

However, often fetchNextPage is expected to often indicate end-of-list.

This seems like unexpected behavior, because no additional base-query usage is made, but any side effects of onQueryStarted DO occur.

Take the following example, with the code below.

In this example, patchCachedUser syncs some metadata between the query-notifications endpoints with the get-user endpoint (the total notifications count, to prevent flickering while states reconcile).

The behavior that occurs is:

1. Hook mounts with page of data. onQueryStarted does not fire unless this is the first time the hook is mounted, as expected.
2. The end of list is reached, triggering fetchNextPage.
3. fetchNextPage, via getNextPageParam, knows we're at the end-of-list, returning undefined.
4. onQueryStarted is fired, ALWAYS. however, queryFulfilled is already resolved, and so it does the remaining work (e.g. pessimistic updates), using the old data.

 getNotifications: build.infiniteQuery<
      {
        notifications: Notification[];
        user: Pick<User, "numUnreadNotifications">
      },
      {
        uid: string;
        limit: number;
        filter: "read" | "unread"
      },
      PaginationCursor | undefined
    >({
      onQueryStarted: async (args, { dispatch, queryFulfilled }) => {
        try {
          console.log("Fetching notifications for user:"); // THIS logs during a `fetchNextPage` call
          const { data } = await queryFulfilled; // Base query has its own logging -- this is already resolved during the calls.
          const lastPage = data.pages[data.pages.length - 1];
          const user = lastPage.user;
          dispatch(patchCachedUser(args, user)); // This now badly assumes that the data is the most-recent from the API, but its from a previous cached call.
        } catch (error) {
          void error;
        }
      },
     infiniteQueryOptions: {
        initialPageParam: undefined,
        getNextPageParam: ({
          notifications: lastPage,
        }): undefined | PaginationCursor => {
          const lastItem = lastPage?.at(-1);
          return lastItem && { createdAt: lastItem.createdAt };
        },
      },

---

This looks like unexpected behavior to me, and I don't see a clean way to get around this (to prevent side affects from occuring multiple times from the same resolved underlying queryFulfilled)

[JacobJaffe]

Demo:
https://github.com/JacobJaffe/rtk-infinite-next-page-demo

simple reproduction of this -- open up the console, and see that onQueryStarted keeps getting called when load more is pressed, after the faked end-of-list behavior

[JacobJaffe]

Right now, I'm working on hacking a fix by keeping track of already-handled queryFulfilled cases via a global scope map, and no-oping within onQueryStarted if so.

Curious if there's a better way to do this for now, since this requires updating the base query such that it tacks a unique id into meta (and generally feels brittle).

// global scope
const handled = new Set<string>();

...
// onQueryStarted 
const { data, meta } = await queryFulfilled;
if (handled.has(meta.id)) return;
handled.add(meta.id);
// continue with patching logic
...

[markerikson]

Yeah, this is presumably something we can fix internally. Haven't had a chance to look into this one, but will try to find time to figure it out. (Also feel free to poke around at the source and see if there's a good solution!)

[JacobJaffe]

Am digging around a little, and I see that this in buildHooks.ts:

export type LazyInfiniteQueryTrigger<
  D extends InfiniteQueryDefinition<any, any, any, any, any>,
> = {
  /**
   * Triggers a lazy query.
   *
   * By default, this will start a new request even if there is already a value in the cache.
   * If you want to use the cache value and only start a request if there is no cache value, set the second argument to `true`.
   *
   * @remarks
   * If you need to access the error or success payload immediately after a lazy query, you can chain .unwrap().
   *
   * @example
   * ```ts
   * // codeblock-meta title="Using .unwrap with async await"
   * try {
   *   const payload = await getUserById(1).unwrap();
   *   console.log('fulfilled', payload)
   * } catch (error) {
   *   console.error('rejected', error);
   * }
   * ```
   */
  (
    arg: QueryArgFrom<D>,
    direction: InfiniteQueryDirection,
  ): InfiniteQueryActionCreatorResult<D>
}

I'm not sure if this is just a docs issue with how it was adapted from LazyQueryTrigger, but that

If you want to use the cache value and only start a request if there is no cache value, set the second argument to true.

doesn't appear correct on this, since InfiniteQueryDefinition uses the direction argument instead of preferCacheValue.

This might just be a docs issue, but if other parts of the trigger chain infer behavior off of a truthy second arg, it may be related.

[markerikson]

Nah, that's a docs issue. I think the original contributed draft PR started by copy-pasting some of the lazy query trigger logic and setup, so pretty sure that's just a docs holdover that didn't get caught.

[JacobJaffe]

Taking a stab getting into the weeds on this.

I've got a better reproduction of this within the inifiniteQueries.test.ts test suite:
https://github.com/reduxjs/redux-toolkit/compare/master...JacobJaffe:redux-toolkit:infinite-query-fetch-next-lifecycle-bug?expand=1#diff-603de156f749730b38436153548990e6af8355cadd0d68f041a0aa78c9771337

I've confirmed here a more general case:

 function createFiniteDataApi() {
    const data = ['a', 'b', 'c', 'd', 'e']
    let queryFnCallCount = 0

    const finiteDataApi = createApi({
      baseQuery: fakeBaseQuery(),
      endpoints: (build) => ({
        list: build.infiniteQuery<string, void, number>({
          infiniteQueryOptions: {
            initialPageParam: 0,
            getNextPageParam: (
              lastPage,
              allPages,
              lastPageParam,
              allPageParams,
            ) => {
              const nextPage = lastPageParam + 1
              if (nextPage < data.length) return nextPage
              return undefined // When this is reached, the next invocation of 'forward' will cause the `expect` below to fail.
            },
            getPreviousPageParam: (
              firstPage,
              allPages,
              firstPageParam,
              allPageParams,
            ) => {
              return firstPageParam > 0 ? firstPageParam - 1 : undefined
            },
          },
          queryFn: async ({ pageParam }) => {
            queryFnCallCount++
            return {
              data: data[pageParam] ?? 'not-found',
            }
          },
          onQueryStarted: async (_, { queryFulfilled }) => {
            const queryFnCallCountAtStart = queryFnCallCount
            await queryFulfilled
            expect(queryFnCallCount).toBe(queryFnCallCountAtStart + 1)
          },
        }),
      }),
    })

    return finiteDataApi
  }

here, that expect(queryFnCallCount).toBe(queryFnCallCountAtStart + 1) is failing.

I'm gonna try to dig deeper into this; hopefully this test makes debugging easier for anyone else as well

[markerikson]

Yeah, off the top of my head, pretty sure the fetch page methods are dispatching the initiate() thunk, so it does make sense the resulting actions would trigger the lifecycle methods.

We probably need to do a check in the lifecycle handling to distinguish between "initial request" and "more pages" to skip triggering the lifecycle handlers. But, that also will get complicated with how we determine whether this is an initial request, full refetch, etc, inside of the actual thunk. Especially because we probably need to add that as metadata to the pending action, but we don't determine any of that info until we are in the middle of executing the thunk body.

[JacobJaffe]

It looks like the case is specifically hit in:
if (param == null && data.pages.length)
https://github.com/reduxjs/redux-toolkit/blob/master/packages/toolkit/src/query/core/buildThunks.ts#L546

Could meta be returned with a flag in that case?

This would allow a consumer to distinguish these cases, for pessimistic uses of the data at least

[JacobJaffe]

What if the hooks themselves,

        const fetchNextPage = () => {
          return trigger(stableArg, 'forward')
        }

        const fetchPreviousPage = () => {
          return trigger(stableArg, 'backward')
        }

(buildHooks.ts)

checked for the entry's hasNextPage / hasPreviousPage, and no-op'd completely?

...saying this, I think that this might just be the simplest consumer fix for now also:

const { fetchNextPage, hasNextPage, ... } = useInfiniteFooQuery();
const onEndReached = hasNextPage ? fetchNextPage : () => {};