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

prevent accidental widening of inferred TData and TVariables generics for query hook option arguments #10506

Merged
merged 4 commits into from
Feb 15, 2023

Conversation

phryneas
Copy link
Member

@phryneas phryneas commented Feb 3, 2023

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

This issue came up in a discussion with @benjamn about type behavior.
The PR is best explained by the comment I added on types.ts:

/**
Helper type that allows using a type in a way that cannot be "widened" by inference on the value it is used on.
This type was first suggested [in this Github discussion](https://github.com/microsoft/TypeScript/issues/14829#issuecomment-504042546).
Example usage:
```ts
export function useQuery<
TData = any,
TVariables extends OperationVariables = OperationVariables,
>(
query: DocumentNode | TypedDocumentNode<TData, TVariables>,
options: QueryHookOptions<NoInfer<TData>, NoInfer<TVariables>> = Object.create(null),
)
```
In this case, `TData` and `TVariables` should be inferred from `query`, but never widened from something in `options`.
So, in this code example:
```ts
declare const typedNode: TypedDocumentNode<{ foo: string}, { bar: number }>
const { variables } = useQuery(typedNode, { variables: { bar: 4, nonExistingVariable: "string" } });
```
Without the use of `NoInfer`, `variables` would now be of the type `{ bar: number, nonExistingVariable: "string" }`.
With `NoInfer`, it will instead give an error on `nonExistingVariable`.
*/

for `useQuery`, `useLazyQuery` and `useMutation`
to prevent accidental "widening" of inferred generics
@changeset-bot
Copy link

changeset-bot bot commented Feb 3, 2023

🦋 Changeset detected

Latest commit: 9315a76

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 Minor

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

@phryneas
Copy link
Member Author

phryneas commented Feb 3, 2023

PS: this also fixes the incorrect type behaviour I demonstrated in this playground - although that should not be a problem going forward, since we want to remove the query option anyways.

@phryneas phryneas self-assigned this Feb 7, 2023
Copy link
Member

@benjamn benjamn left a comment

Choose a reason for hiding this comment

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

We've talked this over on Zoom/Slack and I agree it's an improvement.

If there are any reports of unexpected type errors on the release-3.8 branch, we can fix them before release.

@phryneas phryneas merged commit 2dc2e1d into release-3.8 Feb 15, 2023
This was referenced Feb 15, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants