-
-
Notifications
You must be signed in to change notification settings - Fork 31
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
fix: reuse helper to generate keys in prefetch hooks #103
Conversation
src/createPrefetch.mts
Outdated
@@ -54,6 +66,13 @@ function createPrefetchHook({ | |||
) | |||
), | |||
...requestParams, | |||
ts.factory.createParameterDeclaration( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why have the queryKey parameter?
I understand the want to make the prefetch match the query hooks.
Do you envision using this on prefetch requests?
Have you used the queryKey parameter on the query hooks?
I am wondering if we need that at all (even for query hooks)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, in the case of the prefetch, we need to mirror what the query hook does, so if the query hook supports it, we expect to also be able to prefetch queries with the same key. Having said that, I think I have used the custom query key
param just a few times when I found the autogenerated one not easy to read. I guess the idea was to give devs the flexibility to use the query keys based on the query string or set their custom one, which I think is fantastic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@7nohe any comments on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that having a queryKey is necessary for flexibility. Since normal queries accept a queryKey, we should conform to that as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am in agreement but I would like to understand use cases where we would need a specific queryKey outside of the generated one and the query request params.
My argument against the 'flexibility' is that it pollutes the code and pollutes the API.
If someone needs more flexibility they should make their own hook.
I say we move forward with copying what the queries do and think about next major patch to remove the option to supply a queryKey.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't come up with a specific case, but if I were to explain using the Next.js example I just added, using keys such as QueryA or QueryB rather than matching tags or limits makes it easier to manage the code.
app/page.tsx
import { prefetchUseDefaultServiceFindPets } from "@/openapi/queries/prefetch";
import {
HydrationBoundary,
QueryClient,
dehydrate,
} from "@tanstack/react-query";
import Pets from "./pets";
export default async function Home() {
const queryClient = new QueryClient();
await prefetchUseDefaultServiceFindPets(queryClient, {
limit: 10,
tags: [],
}, ['QueryA']);
await prefetchUseDefaultServiceFindPets(queryClient, {
limit: 100,
tags: ['tag1', 'tag2', 'tag3'],
}, ['QueryB']);
return (
<main className="flex min-h-screen flex-col items-center justify-between p-24">
<HydrationBoundary state={dehydrate(queryClient)}>
<Pets />
</HydrationBoundary>
</main>
);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the example.
I am failing to understand how it is 'easier' than using the generated key and properties.
When overriding, there will be queries and keys that do not match when these hooks are used in other places in code.
I feel it would be safer for the consumer to write their own hooks and preferences so they have complete control rather than us allowing them to configure keys that they would have to override every single use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the example.
I am failing to understand how it is 'easier' than using the generated key and properties.
When overriding, there will be queries and keys that do not match when these hooks are used in other places in code.
I feel it would be safer for the consumer to write their own hooks and preferences so they have complete control rather than us allowing them to configure keys that they would have to override every single use.
As you mentioned, that does seem to be the safer option. Since this library outputs pure TypeScript clients, it would be better to use them to build your own hooks.
It seems better to allow a queryKey option after finding some practical cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nopitown
Would you remove the queryKey
parameter?
The latest main branch includes the Next.js sample app. You can run it with the following command.
npm run build && pnpm --filter nextjs-app generate:api && pnpm --filter nextjs-app dev
If you're busy, I can make the changes instead.
Since an issue (#121 ) regarding this problem has been created again, it seems better to address it quickly, so I will take over. |
@seriouslag |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great
So when is this getting released? This issue makes it so prefetch is unusable right now for us. It's been approved for 2 weeks. |
@TomWebbio Sorry for the delay. I was sick for the past two weeks and couldn't find time to release it. I will release it now. |
Attempting to fix #102. I'll revisit it tomorrow since I'm still getting familiar with the code. The ultimate goal here is to allow prefetch hooks to be as flexible as query hooks are and create the same key structure so it is possible to use the cache.