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

fix: reuse helper to generate keys in prefetch hooks #103

Merged
merged 6 commits into from
Jun 7, 2024
Merged

fix: reuse helper to generate keys in prefetch hooks #103

merged 6 commits into from
Jun 7, 2024

Conversation

nopitown
Copy link
Contributor

@nopitown nopitown commented May 3, 2024

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.

@@ -54,6 +66,13 @@ function createPrefetchHook({
)
),
...requestParams,
ts.factory.createParameterDeclaration(
Copy link
Collaborator

@seriouslag seriouslag May 3, 2024

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)

Copy link
Contributor Author

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.

Copy link
Contributor Author

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?

Copy link
Owner

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.

Copy link
Collaborator

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.

Copy link
Owner

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>
  );
}

Copy link
Collaborator

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.

Copy link
Owner

@7nohe 7nohe May 4, 2024

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.

Copy link
Owner

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.

src/createPrefetch.mts Show resolved Hide resolved
@7nohe
Copy link
Owner

7nohe commented May 26, 2024

Since an issue (#121 ) regarding this problem has been created again, it seems better to address it quickly, so I will take over.

@7nohe 7nohe self-assigned this May 26, 2024
@7nohe 7nohe requested a review from seriouslag May 26, 2024 01:18
@7nohe
Copy link
Owner

7nohe commented May 26, 2024

@seriouslag
I removed the queryKey parameter from prefetch.ts. I have also confirmed that the queryKey matches between the prefetchQuery and useQuery generated in the Next.js sample app.

@7nohe 7nohe marked this pull request as ready for review May 26, 2024 01:26
Copy link
Collaborator

@seriouslag seriouslag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great

@TomWebbio
Copy link

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.

@7nohe
Copy link
Owner

7nohe commented Jun 7, 2024

@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.

@7nohe 7nohe merged commit 0529b6f into 7nohe:main Jun 7, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Prefetch hook keys aren't properly set
4 participants