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

You should be able to pass a pageParam as a default and not rely on undefined #43

Closed
keegancruickshank opened this issue May 23, 2023 · 18 comments · Fixed by #47
Closed

Comments

@keegancruickshank
Copy link

When an initial request using useInfiniteQuery is sent the value for pageParamKey will always be undefined, you should be able to either accept what is in the initial request/not override it or pass an argument as a parameter to the useInfiniteQuery call.

@paul-sachs
Copy link
Collaborator

paul-sachs commented May 23, 2023

Hi @keegancruickshank, I'm sorry, I may not fully understand the question. Perhaps you could provide a simplified example so I can answer it better.

Is this behavior of how it will override this key in the input what you are talking about? If so: where would we get the value for that key (which, today, is context.pageParam.

We could default to the current value from the input:

{
  ...input,
- [pageParamKey]: context.pageParam,
+ [pageParamKey]: context.pageParam ?? input[pageParamKey],
}

note: in this case, wouldn't it be a problem if you happen to use null or undefined to mean the end of the list. Though if that was the case, it would have already been a problem, so probably a moot point.

@keegancruickshank
Copy link
Author

keegancruickshank commented May 23, 2023

Hi @paul-sachs thanks for the reply, I can explain my issue a little more in detail, first off the hook:

import { useState } from "react";

import {
    MockRequest,
    MockRequest_Direction,
    MockResponse,
} from "@mockorganisation/apis-connect-web/mock/service/v1/service_pb";
import { useInfiniteQuery } from "@tanstack/react-query";

import { mockQueryService } from "./index";

const { getItems } = mockQueryService;

const PAGE_SIZE = 25;

export const useFetchUserCourses = (userId: string) => {
     // State setter is commented until we implement search and sorting
     const [req, _setReq] = useState<MockRequest>(
        new MockRequest({
            userId,
            pageSize: PAGE_SIZE,
            page: 1,
            searchTerm: "",
            direction: MockRequest_Direction.ASC,
        }),
    );

    const getNextPageParam = (
        prevPage: MockResponse,
        pages: MockResponse[],
    ) => {
        const totalCount = prevPage.totalCount;
        const runningTotal =
            pages.flatMap((page) => page.items).length + prevPage.items.length;
        if (totalCount === runningTotal) {
            return;
        } else {
            if (runningTotal % PAGE_SIZE === 0) {
                return runningTotal / PAGE_SIZE;
            }
            return;
        }
    };

    const mockItemsQuery = getItems.useInfiniteQuery(req, {
        pageParamKey: "page",
        getNextPageParam,
    });

    return useInfiniteQuery({
        queryFn: mockItemsQuery.queryFn,
        queryKey: mockItemsQuery.queryKey,
        getNextPageParam,
    });
};

The main issue resides from our apis being "1" based rather then "0" based. This means that when no param is passed, we actually throw an error as our proto validator has a required field and the initial request for useInfiniteQuery overrides my state variable page: 1.

Your solution could work but I dare say we would need a pristine check to see if it is the first call for this infinite query and only use the input value if it is.

@BrRenat
Copy link

BrRenat commented May 24, 2023

  • [pageParamKey]: context.pageParam ?? input[pageParamKey],

it would be great! Or add extra option to useInfiniteQuery defaultPageParam

Now I can't define defaultPageParam for the first request. And i think that defaultPageParam prevent unnecessary mutation for queryKey.

For example, if pageParamKey = 'pagination'.

const request  = {
 ...input,
 pagination: {
    page: 0,
    pageSize: 2,
 }
}

We cannot define pageSize for the first request.

--

On the other hand, maybe it's better to add the ability to define a deep key? But it may affect query performance.

pageParamKey = 'pagination. page'

@BrRenat
Copy link

BrRenat commented May 24, 2023

@paul-sachs sorry for the spam, what do you think about this? I can help you if needed.

@paul-sachs
Copy link
Collaborator

paul-sachs commented May 24, 2023

I was thinking about the idea of a deep key. I think the whole pageParamKey might have been too simplistic a parameter to begin with. Perhaps a better option would be a callback that's given the pageParam and must return the input for the given RPC, so something like:

const mockItemsQuery = getItems.useInfiniteQuery(req, {
  applyPageParam: ({ pageParam }) => ({
      ...req,
      page: pageParam ?? 1,
  }),
  getNextPageParam,
});

That way, we can allow any level of transform and interpretation of the pageParam into the input message.

@BrRenat
Copy link

BrRenat commented May 24, 2023

I'm not sure about req in pageParam. maybe something like this

const mockItemsQuery = getItems.useInfiniteQuery(req, {
  applyPageParam: (pageParam) => ({
      ...pageParam,
      page: 1,
  }),
  getNextPageParam,
});

and

const result = await transport.unary(
  { typeName, methods: {} },
  methodInfo,
  (callOptions ?? context).signal,
  callOptions?.timeoutMs,
  callOptions?.headers,
  {
    ...input,
    [pageParamKey]: applyPageParam ? applyPageParam(context.pageParam) : context.pageParam,
  },
);

@keegancruickshank
Copy link
Author

I was thinking about the idea of a deep key. I think the whole pageParamKey might have been too simplistic a parameter to begin with. Perhaps a better option would be a callback that's given the pageParam and must return the input for the given RPC, so something like:

const mockItemsQuery = getItems.useInfiniteQuery(req, {
  applyPageParam: ({ pageParam }) => ({
      ...req,
      page: pageParam ?? 1,
  }),
  getNextPageParam,
});

That way, we can allow any level of transform and interpretation of the pageParam into the input message.

@paul-sachs I love this implementation, It may need to be worded or explained better, but it makes sense to give back control like this. +1 from me for this implementation. It solves both the deep key issue and mine so it's a win win really. Is it something you would seriously look at doing?

@BrRenat
Copy link

BrRenat commented May 24, 2023

more clearly:
applyPageParam rename to getPageParam (for change pageParam)
getNextPageParam (calculates the next pageParam and can then be changed by getPageParam if it is needed)

@paul-sachs
Copy link
Collaborator

I think my goal is to allow applyPageParam to be defined outside of the context you use it, so eventually it could be provided all necessary arguments. Basically so you can do something like:

function applyPageParam({ pageParam, incomingMessage }: ApplyPageParams<Message>) {
  return {
    ...incomingMessage,
    page: pageParam ?? 1
  }
}

const mockItemsQuery = getItems.useInfiniteQuery(req, {
  applyPageParam,
  getNextPageParam,
});

And it would look something like this internally:

const result = await transport.unary(
  { typeName, methods: {} },
  methodInfo,
  (callOptions ?? context).signal,
  callOptions?.timeoutMs,
  callOptions?.headers,
  applyPageParam({ pageParam: context.pageParam, incomingMessage: input }),
);

This gives the developers the highest control over how the page param is applied to change the outgoing message.

@keegancruickshank yeah I think this is something we can do easily enough. I'm going to apply this in stages, first I'll let it so we default to input[pageParamKey] since that isn't an api change, and then we can scope out the API to add this applyPageParam option.

@keegancruickshank
Copy link
Author

@paul-sachs Love your work, thank you.

@BrRenat
Copy link

BrRenat commented May 24, 2023

@paul-sachs Thank you for the quick response!

@dimitropoulos
Copy link
Contributor

hi @BrRenat @keegancruickshank :)

We've just published 0.2.3 which contains a fix for this. Please let us know how it works for you.

Also... thanks for using Connect-Query! If you have any other feedback we'd love to hear it!

@paul-sachs
Copy link
Collaborator

Note that the 0.2.3 release only includes the default. I'll have a different PR for the applyPageParam option.

@BrRenat
Copy link

BrRenat commented May 24, 2023

@dimitropoulos @paul-sachs yep, default's works great. Thank you guys! Maybe you should omit pageParamKey from queryKey?

@BrRenat
Copy link

BrRenat commented May 24, 2023

Connect-Query is really great! We've been using impropable-eng/grpc-web for about 3 years and have now completely switched to the bufbuild ecosystem. Great tools!

@paul-sachs
Copy link
Collaborator

@BrRenat Thanks, really appreciate you saying so.

Maybe you should omit pageParamKey from queryKey?

This is a good point though I'm a little hesitant to apply it unless I've got a concrete use-case where it's causing issues. I suspect omitting it will have similar issues if your pagination entry is weirdly nested.

@BrRenat
Copy link

BrRenat commented May 24, 2023

@paul-sachs it's just a bit confusing that we have a dynamic parameter like pageSize in our queryKey. I understand how to prevent new queryKey creation if our pageSize changed. But I think that someone might have a potential bug if this behavior is ignored.

const req = { status: 'completed', pagination: { pageSize: 10 } }

const query1 = getItems.useInfiniteQuery(req)

;(() => {
 req.pagination.pageSize = 20
 
 const query2 = getItems.useInfiniteQuery(req)
 
 console.log(query1.queryKey !== query2.queryKey) // true, but we just want to load more items for the next page (infinity scroll) and use the same query :(
}){}

@paul-sachs
Copy link
Collaborator

Yeah that's fair. I've always found comparing queryKeys to be fairly fragile, but I can see where it has it's place. I think it probably warrants it's own issue though. This behaviour may also apply to other query methods where you may not want the full message inside the key.

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

Successfully merging a pull request may close this issue.

4 participants