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

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

Conversation

phryneas
Copy link
Member

@phryneas phryneas commented Sep 22, 2023

This would address #10857

I'm opening this PR now to get feedback on it - if we agree that this is the way forward, it would need to be done for all other hooks as well - maybe we can find a nice type helper for it, although the implicitly created interface with a name helps for autocomplete readability.

I would target 3.9 with this - I don't expect this to negatively impact anyone, but it should probably be in a patch release.

Checklist:

  • If this PR contains changes to the library itself (not necessary for e.g. docs updates), please include a changeset (see CONTRIBUTING.md)
  • If this PR is a new feature, please reference an issue where a consensus about the design was reached (not necessary for small changes)
  • Make sure all of the significant new logic is covered by tests

@changeset-bot
Copy link

changeset-bot bot commented Sep 22, 2023

🦋 Changeset detected

Latest commit: 08793f5

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@apollo/client Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Comment on lines +63 to +72
...[options]: HasRequiredVariables<
TVariables,
[
optionsWithVariables: QueryHookOptionsWithVariables<
NoInfer<TData>,
NoInfer<TVariables>
>,
],
[options?: QueryHookOptions<NoInfer<TData>, NoInfer<TVariables>>]
>
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 }

Comment on lines +8265 to +8270
useQuery(query, {
variables: {
// @ts-expect-error on unknown variable
foo: "bar",
},
});
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.

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

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.

@github-actions
Copy link
Contributor

github-actions bot commented Sep 22, 2023

size-limit report 📦

Path Size
dist/apollo-client.min.cjs 37.11 KB (0%)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/main.cjs" 43.47 KB (0%)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/main.cjs" (production) 42.01 KB (0%)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/index.js" 32.54 KB (0%)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/index.js" (production) 31.28 KB (0%)
import { ApolloProvider } from "dist/react/index.js" 1.22 KB (0%)
import { ApolloProvider } from "dist/react/index.js" (production) 1.21 KB (0%)
import { useQuery } from "dist/react/index.js" 4.28 KB (0%)
import { useQuery } from "dist/react/index.js" (production) 4.09 KB (0%)
import { useLazyQuery } from "dist/react/index.js" 4.59 KB (0%)
import { useLazyQuery } from "dist/react/index.js" (production) 4.41 KB (0%)
import { useMutation } from "dist/react/index.js" 2.53 KB (0%)
import { useMutation } from "dist/react/index.js" (production) 2.51 KB (0%)
import { useSubscription } from "dist/react/index.js" 2.24 KB (0%)
import { useSubscription } from "dist/react/index.js" (production) 2.2 KB (0%)
import { useSuspenseQuery } from "dist/react/index.js" 4.6 KB (0%)
import { useSuspenseQuery } from "dist/react/index.js" (production) 4.03 KB (0%)
import { useBackgroundQuery } from "dist/react/index.js" 4.11 KB (0%)
import { useBackgroundQuery } from "dist/react/index.js" (production) 3.53 KB (0%)
import { useReadQuery } from "dist/react/index.js" 2.97 KB (0%)
import { useReadQuery } from "dist/react/index.js" (production) 2.92 KB (0%)
import { useFragment } from "dist/react/index.js" 2.08 KB (0%)
import { useFragment } from "dist/react/index.js" (production) 2.03 KB (0%)

@phryneas phryneas changed the title variables/options should only be required if TVariables is empty or default variables/options should be required if TVariables is not empty/default Sep 22, 2023
@jerelmiller jerelmiller force-pushed the release-3.9 branch 2 times, most recently from c80ba36 to 3f7eecb Compare December 6, 2023 20:55
@phryneas phryneas changed the base branch from release-3.9 to release-3.10 January 8, 2024 15:10
@phryneas phryneas added this to the Release 3.10 milestone Jan 8, 2024
Base automatically changed from release-3.10 to main April 24, 2024 15:49
@jerelmiller jerelmiller removed this from the Release 3.10 milestone Apr 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants