Skip to content

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

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Jun 7, 2024
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3588856
reuse function to generate keys
nopitown May 3, 2024
6a7562d
Merge branch 'main' into nopitown-fix-prefetch-query-keys
7nohe May 26, 2024
9b0af8b
chore: Remove queryKey parameter from prefetch queries
7nohe May 26, 2024
7e6d53d
chore: Format code
7nohe May 26, 2024
b46eff7
chore: Remove unused imports
7nohe May 26, 2024
58668d2
chore: Format code
7nohe May 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 41 additions & 22 deletions src/createPrefetch.mts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,15 @@ import {
BuildCommonTypeName,
extractPropertiesFromObjectParam,
getNameFromMethod,
queryKeyConstraint,
queryKeyGenericType,
} from "./common.mjs";
import { type MethodDescription } from "./common.mjs";
import {
createQueryKeyFromMethod,
getRequestParamFromMethod,
hookNameFromMethod,
getQueryKeyFnName,
} from "./createUseQuery.mjs";
import { addJSDocToNode } from "./util.mjs";

Expand Down Expand Up @@ -42,7 +45,16 @@ function createPrefetchHook({
undefined,
ts.factory.createArrowFunction(
undefined,
undefined,
ts.factory.createNodeArray([
ts.factory.createTypeParameterDeclaration(
undefined,
"TQueryKey",
queryKeyConstraint,
ts.factory.createArrayTypeNode(
ts.factory.createKeywordTypeNode(ts.SyntaxKind.UnknownKeyword)
)
),
]),
[
ts.factory.createParameterDeclaration(
undefined,
Expand All @@ -54,6 +66,13 @@ function createPrefetchHook({
)
),
...requestParams,
ts.factory.createParameterDeclaration(
Copy link
Collaborator

@seriouslag seriouslag May 3, 2024 •
edited
Loading

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 •
edited
Loading

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.

undefined,
undefined,
ts.factory.createIdentifier("queryKey"),
ts.factory.createToken(ts.SyntaxKind.QuestionToken),
queryKeyGenericType
),
],
undefined,
ts.factory.createToken(ts.SyntaxKind.EqualsGreaterThanToken),
Expand All @@ -64,28 +83,28 @@ function createPrefetchHook({
ts.factory.createObjectLiteralExpression([
ts.factory.createPropertyAssignment(
ts.factory.createIdentifier("queryKey"),
ts.factory.createArrayLiteralExpression(
[
BuildCommonTypeName(queryKey),
method.getParameters().length
? ts.factory.createArrayLiteralExpression([
ts.factory.createObjectLiteralExpression(
method
.getParameters()
.map((param) =>
extractPropertiesFromObjectParam(param).map(
(p) =>
ts.factory.createShorthandPropertyAssignment(
ts.factory.createIdentifier(p.name)
)
)
ts.factory.createCallExpression(
BuildCommonTypeName(getQueryKeyFnName(queryKey)),
undefined,

method.getParameters().length
? [
ts.factory.createObjectLiteralExpression(
method
.getParameters()
.map((param) =>
extractPropertiesFromObjectParam(param).map(
(p) =>
ts.factory.createShorthandPropertyAssignment(
ts.factory.createIdentifier(p.name)
)
)
.flat()
),
])
: ts.factory.createArrayLiteralExpression([]),
],
false
)
.flat()
),
ts.factory.createIdentifier("queryKey"),
]
: []
)
),
ts.factory.createPropertyAssignment(
Expand Down
2 changes: 1 addition & 1 deletion src/createUseQuery.mts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -466,7 +466,7 @@ export const createUseQuery = ({
};
};

function getQueryKeyFnName(queryKey: string) {
export function getQueryKeyFnName(queryKey: string) {
return `${capitalizeFirstLetter(queryKey)}Fn`;
}

Expand Down
10 changes: 5 additions & 5 deletions tests/__snapshots__/createSource.test.ts.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,17 +180,17 @@ import { Pet, NewPet, Error, $OpenApiTs } from "../requests/types.gen";
* @returns Error unexpected error
* @throws ApiError
*/
export const prefetchUseDefaultServiceFindPets = (queryClient: QueryClient, { limit, tags }: {
export const prefetchUseDefaultServiceFindPets = <TQueryKey extends Array<unknown> = unknown[]>(queryClient: QueryClient, { limit, tags }: {
limit?: number;
tags?: string[];
} = {}) => queryClient.prefetchQuery({ queryKey: [Common.useDefaultServiceFindPetsKey, [{ limit, tags }]], queryFn: () => DefaultService.findPets({ limit, tags }) });
} = {}, queryKey?: TQueryKey) => queryClient.prefetchQuery({ queryKey: Common.UseDefaultServiceFindPetsKeyFn({ limit, tags }, queryKey), queryFn: () => DefaultService.findPets({ limit, tags }) });
/**
* @deprecated
* This path is not fully defined.
* @returns unknown unexpected error
* @throws ApiError
*/
export const prefetchUseDefaultServiceGetNotDefined = (queryClient: QueryClient) => queryClient.prefetchQuery({ queryKey: [Common.useDefaultServiceGetNotDefinedKey, []], queryFn: () => DefaultService.getNotDefined() });
export const prefetchUseDefaultServiceGetNotDefined = <TQueryKey extends Array<unknown> = unknown[]>(queryClient: QueryClient, queryKey?: TQueryKey) => queryClient.prefetchQuery({ queryKey: Common.UseDefaultServiceGetNotDefinedKeyFn(), queryFn: () => DefaultService.getNotDefined() });
/**
* Returns a user based on a single ID, if the user does not have access to the pet
* @param data The data for the request.
Expand All @@ -199,8 +199,8 @@ export const prefetchUseDefaultServiceGetNotDefined = (queryClient: QueryClient)
* @returns Error unexpected error
* @throws ApiError
*/
export const prefetchUseDefaultServiceFindPetById = (queryClient: QueryClient, { id }: {
export const prefetchUseDefaultServiceFindPetById = <TQueryKey extends Array<unknown> = unknown[]>(queryClient: QueryClient, { id }: {
id: number;
}) => queryClient.prefetchQuery({ queryKey: [Common.useDefaultServiceFindPetByIdKey, [{ id }]], queryFn: () => DefaultService.findPetById({ id }) });
}, queryKey?: TQueryKey) => queryClient.prefetchQuery({ queryKey: Common.UseDefaultServiceFindPetByIdKeyFn({ id }, queryKey), queryFn: () => DefaultService.findPetById({ id }) });
"
`;
10 changes: 5 additions & 5 deletions tests/__snapshots__/generate.test.ts.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,17 +54,17 @@ import * as Common from "./common";
* @returns Error unexpected error
* @throws ApiError
*/
export const prefetchUseDefaultServiceFindPets = (queryClient: QueryClient, { limit, tags }: {
export const prefetchUseDefaultServiceFindPets = <TQueryKey extends Array<unknown> = unknown[]>(queryClient: QueryClient, { limit, tags }: {
limit?: number;
tags?: string[];
} = {}) => queryClient.prefetchQuery({ queryKey: [Common.useDefaultServiceFindPetsKey, [{ limit, tags }]], queryFn: () => DefaultService.findPets({ limit, tags }) });
} = {}, queryKey?: TQueryKey) => queryClient.prefetchQuery({ queryKey: Common.UseDefaultServiceFindPetsKeyFn({ limit, tags }, queryKey), queryFn: () => DefaultService.findPets({ limit, tags }) });
/**
* @deprecated
* This path is not fully defined.
* @returns unknown unexpected error
* @throws ApiError
*/
export const prefetchUseDefaultServiceGetNotDefined = (queryClient: QueryClient) => queryClient.prefetchQuery({ queryKey: [Common.useDefaultServiceGetNotDefinedKey, []], queryFn: () => DefaultService.getNotDefined() });
export const prefetchUseDefaultServiceGetNotDefined = <TQueryKey extends Array<unknown> = unknown[]>(queryClient: QueryClient, queryKey?: TQueryKey) => queryClient.prefetchQuery({ queryKey: Common.UseDefaultServiceGetNotDefinedKeyFn(), queryFn: () => DefaultService.getNotDefined() });
/**
* Returns a user based on a single ID, if the user does not have access to the pet
* @param data The data for the request.
Expand All @@ -73,9 +73,9 @@ export const prefetchUseDefaultServiceGetNotDefined = (queryClient: QueryClient)
* @returns Error unexpected error
* @throws ApiError
*/
export const prefetchUseDefaultServiceFindPetById = (queryClient: QueryClient, { id }: {
export const prefetchUseDefaultServiceFindPetById = <TQueryKey extends Array<unknown> = unknown[]>(queryClient: QueryClient, { id }: {
id: number;
}) => queryClient.prefetchQuery({ queryKey: [Common.useDefaultServiceFindPetByIdKey, [{ id }]], queryFn: () => DefaultService.findPetById({ id }) });
}, queryKey?: TQueryKey) => queryClient.prefetchQuery({ queryKey: Common.UseDefaultServiceFindPetByIdKeyFn({ id }, queryKey), queryFn: () => DefaultService.findPetById({ id }) });
"
`;

Expand Down
Loading