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

Required Query Variables are optional #2870

Closed
tbrannam opened this issue Nov 2, 2019 · 9 comments
Closed

Required Query Variables are optional #2870

tbrannam opened this issue Nov 2, 2019 · 9 comments
Labels
waiting-for-answer Waiting for answer from author

Comments

@tbrannam
Copy link

tbrannam commented Nov 2, 2019

Is your feature request related to a problem? Please describe.

I'm frustrated that my GraphQL variables are not enforced in generated hooks

type Query {
    hello(foo: String!): String
}
query Bar ($foo: String!) {
  hello(foo: $foo)
}

Describe the solution you'd like

Hooks (Apollo 3) generated typically have optional baseOptions. So callers of the generated hooks sometimes forget that a query requires a variable. If a query document includes a variable - it would be ideal if the type signature reflected this requirement.

See: https://codesandbox.io/s/apollo-server-de1p6

Describe alternatives you've considered
Create module level override hooks that add required fields from their signature and call the generated ones.

@dotansimha
Copy link
Owner

@tbrannam the query and fragment you mentioned are not valid. Can you please share a reproduction of the issue? in a repo or in a codesandbox.

Also, which version of codegen do you use?

@dotansimha dotansimha added the waiting-for-answer Waiting for answer from author label Nov 3, 2019
@tbrannam
Copy link
Author

tbrannam commented Nov 4, 2019

Using 1.8.3. I've created a mock server here: https://codesandbox.io/s/apollo-server-de1p6

It has client.ts which could call then generated hook variable contract isn't being typechecked. It would seem that if a Query required variables, the baseOptions would no longer be marked as optional.

@tbrannam
Copy link
Author

tbrannam commented Nov 8, 2019

closing pending further research

@tbrannam tbrannam closed this as completed Nov 8, 2019
@Stuart4
Copy link

Stuart4 commented Jan 4, 2020

Why did you close this? baseOptions and baseOptions.variables should not be required if the variable is required in the query.

@dotansimha
Copy link
Owner

@Stuart4 if you are still having issue with this, can you please open a new issue with a reproduction?

@eyalw
Copy link

eyalw commented Jan 30, 2020

Having the same issue here, the generated typing does not enforce user to provide mandatory variables.

@dotansimha
Copy link
Owner

@eyalw can you please share a reproduction?

@Mathieuu
Copy link

Mathieuu commented Aug 20, 2020

@dotansimha Thanks for maintaining that awesome library.

Here's a sandbox inspired by the one above to reproduce the issue:
https://codesandbox.io/s/apollo-server-forked-fn0q5?file=/client.ts

In that sandbox you can see that the mutation UpdateUser requires an id as an input.

However in the component:

  // INCORRECT - Missing id
  // This should throws an error because id is a required variable
  // BaseOption should not be optional
  update();

  // CORRECT - Throw an error
  update({ variables: { input: { } } });

  // CORRECT
  update({ variables: { input: { id: "hello" } } });

@filip-lipinski-tsh
Copy link

filip-lipinski-tsh commented Aug 23, 2022

@dotansimha
Hello! Will the changes be completed? It would be great if this were merged as this optional variables type can cause a lot of trouble.
Problem is with BaseQueryOptions type, variables are always optional (it shouldn't, if some variable is required), see screenshot: Zrzut ekranu 2022-08-23 o 14 45 35

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting-for-answer Waiting for answer from author
Projects
None yet
Development

No branches or pull requests

6 participants