-
Notifications
You must be signed in to change notification settings - Fork 17
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
Comments
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 {
...input,
- [pageParamKey]: context.pageParam,
+ [pageParamKey]: context.pageParam ?? input[pageParamKey],
}
|
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 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. |
it would be great! Or add extra option to useInfiniteQuery defaultPageParam Now I can't define For example, if
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.
|
@paul-sachs sorry for the spam, what do you think about this? I can help you if needed. |
I was thinking about the idea of a deep key. I think the whole 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. |
I'm not sure about
and
|
@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? |
more clearly: |
I think my goal is to allow 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 |
@paul-sachs Love your work, thank you. |
@paul-sachs Thank you for the quick response! |
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! |
Note that the 0.2.3 release only includes the default. I'll have a different PR for the |
@dimitropoulos @paul-sachs yep, default's works great. Thank you guys! Maybe you should omit |
Connect-Query is really great! We've been using |
@BrRenat Thanks, really appreciate you saying so.
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. |
@paul-sachs it's just a bit confusing that we have a dynamic parameter like
|
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. |
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.
The text was updated successfully, but these errors were encountered: