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.

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

variables/options should be required if TVariables is not empty/default #11241

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
+149 −1
Open

variables/options should be required if TVariables is not empty/default #11241

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
cdc82ea
useQuery: `variables/options` should only be required if `TVariables`…
phryneas Sep 22, 2023
80b52fc
changeset
phryneas Sep 22, 2023
dbbbd10
fixup generic `Query` HOC types
phryneas Sep 22, 2023
f3aecc7
Update .changeset/slow-planets-design.md
phryneas Sep 22, 2023
08793f5
Merge branch 'release-3.10' into pr/fix-10857
phryneas Jan 8, 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
5 changes: 5 additions & 0 deletions .changeset/slow-planets-design.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@apollo/client": patch
---

useQuery: `variables/options` should only be required if `TVariables` is empty or default
phryneas marked this conversation as resolved.
Show resolved Hide resolved
2 changes: 1 addition & 1 deletion src/react/components/Query.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ export function Query<
TVariables extends OperationVariables = OperationVariables,
>(props: QueryComponentOptions<TData, TVariables>) {
const { children, query, ...options } = props;
const result = useQuery(query, options);
const result = useQuery<unknown, OperationVariables>(query, options);
Copy link
Member Author

@phryneas phryneas Sep 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Where this causes problems is in generic implementations like our Query HOC:
image

Similar errors would always appear if we go through with the "options/variables sometimes required" approach here - there is no way of implementing that without that result.

Luckily, as you can see there's an easy fix - but this might cause some minor churn.

return result ? children(result as any) : null;
}

Expand Down
112 changes: 112 additions & 0 deletions src/react/hooks/__tests__/useQuery.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8236,4 +8236,116 @@ describe.skip("Type Tests", () => {
// @ts-expect-error
variables?.nonExistingVariable;
});

describe("optional/required options/variables scenarios", () => {
test("untyped document node, variables are optional and can be anything", () => {
const query = {} as DocumentNode;
useQuery(query);
useQuery(query, {});
useQuery(query, { variables: {} });
useQuery(query, { variables: { opt: "opt" } });
useQuery(query, { variables: { req: "req" } });
});
test("typed document node with unspecified TVariables, variables are optional and can be anything", () => {
const query = {} as TypedDocumentNode<{ result: string }>;
useQuery(query);
useQuery(query, {});
useQuery(query, { variables: {} });
useQuery(query, { variables: { opt: "opt" } });
useQuery(query, { variables: { req: "req" } });
});
test("empty variables are optional", () => {
const query = {} as TypedDocumentNode<
{ result: string },
Record<string, never>
>;
useQuery(query);
useQuery(query, {});
useQuery(query, { variables: {} });
useQuery(query, {
variables: {
// @ts-expect-error on unknown variable
foo: "bar",
},
});
Comment on lines +8594 to +8599
Copy link
Member Author

Choose a reason for hiding this comment

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

Indenting these tests like this to highlight where exactly I am expecting the error to be propagated to the user.

});
test("all-optional variables are optional", () => {
const query = {} as TypedDocumentNode<
{ result: string },
{ opt?: string }
>;
useQuery(query);
useQuery(query, {});
useQuery(query, { variables: {} });
useQuery(query, { variables: { opt: "opt" } });
useQuery(query, {
variables: {
// @ts-expect-error on unknown variable
foo: "bar",
},
});
useQuery(query, {
variables: {
opt: "opt",
// @ts-expect-error on unknown variable
foo: "bar",
},
});
});
test("non-optional variables are required", () => {
const query = {} as TypedDocumentNode<
{ result: string },
{ req: string }
>;
// @ts-expect-error on missing options
useQuery(query);
useQuery(
query,
// @ts-expect-error on missing variables
{}
);
useQuery(query, {
// @ts-expect-error on empty variables
variables: {},
});
useQuery(query, {
variables: {
// @ts-expect-error on unknown variable
foo: "bar",
},
});
useQuery(query, { variables: { req: "req" } });
useQuery(query, {
variables: {
req: "req",
// @ts-expect-error on unknown variable
foo: "bar",
},
});
});
test("mixed variables are required", () => {
const query = {} as TypedDocumentNode<
{ result: string },
{ req: string; opt?: string }
>;
// @ts-expect-error on missing options
useQuery(query);
// @ts-expect-error on missing variables
useQuery(query, {});

useQuery(query, {
// @ts-expect-error on empty variables
variables: {},
});

useQuery(query, {
// @ts-expect-error on missing required variable
variables: {
opt: "opt",
},
});
useQuery(query, { variables: { req: "req" } });
useQuery(query, { variables: { req: "req", opt: "opt" } });
});
});
});
31 changes: 31 additions & 0 deletions src/react/hooks/useQuery.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,37 @@ const {
prototype: { hasOwnProperty },
} = Object;

interface QueryHookOptionsWithVariables<
TData,
TVariables extends OperationVariables,
> extends Omit<QueryHookOptions<TData, TVariables>, "variables"> {
variables: TVariables;
}

type OnlyRequiredProperties<T> = {
[K in keyof T as {} extends Pick<T, K> ? never : K]-?: T[K];
};

type HasRequiredVariables<T, TrueCase, FalseCase> =
{} extends OnlyRequiredProperties<T> ? FalseCase : TrueCase;

export function useQuery<
TData = any,
TVariables extends OperationVariables = OperationVariables,
>(
query: DocumentNode | TypedDocumentNode<TData, TVariables>,
...[options]: HasRequiredVariables<
TVariables,
[
optionsWithVariables: QueryHookOptionsWithVariables<
NoInfer<TData>,
NoInfer<TVariables>
>,
],
[options?: QueryHookOptions<NoInfer<TData>, NoInfer<TVariables>>]
>
Comment on lines +63 to +72
Copy link
Member Author

Choose a reason for hiding this comment

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

I have decided to use the "one signature, tuple behind conditional approach" here, as staying with only one public overload will make errors more readable and "localized".

This is how an error would look like if we had two overloads - the full call is marked red and the error is hard to read:

image
image

Instead, using this approach, the error looks like this - only variables are highlighted and the error is much more readable:
image
image

Copy link
Member Author

Choose a reason for hiding this comment

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

Also something to highlight in these screenshots: the interface name QueryHooksOptionsWithVariables is a lot clearer than something calculated inline, like QueryHookOptions<...> & { variables: TVariables }

): QueryResult<TData, TVariables>;

export function useQuery<
TData = any,
TVariables extends OperationVariables = OperationVariables,
Expand Down