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

Implement TypedDocumentNode support (with type-safe variables) #275

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

spion
Copy link

@spion spion commented Apr 22, 2022

I took at stab at fully implementing #235 and while complex this approach seems to work quite well.

I tried to cover everything in this PR (including docs) however do let me know if I've missed something.

@spion
Copy link
Author

spion commented Apr 22, 2022

Just noticed that this is based on an older version and the variable subsystem has seen significant work 😢

Would you be open to considering an approach like this which works with automatic inference, without explicitly specifying the variables and their types before hand?

@aexol
Copy link
Collaborator

aexol commented Apr 24, 2022

Don't worry new variable system is almost same as the old one I can help with rewriting your PR to the new variable system

@spion
Copy link
Author

spion commented Apr 24, 2022

The thing I'm not sure about is that the old system would traverse and discover variables and their types and the new one doesn't appear to do that anymore - unless I'm missing it somehow? The TypesPropsResolver and inspectVariables machinery appears to be gone now.

@aexol
Copy link
Collaborator

aexol commented Apr 27, 2022

Ok it is much easier than expected in 5.0.0. I will publish PR with TypedQueryDocumentNode today so you can review

@aexol
Copy link
Collaborator

aexol commented Apr 27, 2022

#279 Can you take a look on this? I think it solves the problem

@aexol
Copy link
Collaborator

aexol commented Apr 27, 2022

@spion

@aexol aexol closed this Apr 27, 2022
@spion
Copy link
Author

spion commented Apr 27, 2022

Hi @aexol unfortunately this doesn't solve the problem for me, for two reasons:

  1. I would like to use the query with different variable values every time, and the useZeusVariables API is a bit awkward way to do that as it assumes I can specify both the variables and their values at the same time.
  2. I don't want to manually specify the types for variables. It maybe easy in the simple cases but mutations will often have fairly complex input object possibilities and writing the types for those can be tedious and error-prone

The v4.0 solution I came up with infers all types fully automatically with no need to specify them upfront - while still remaining type-safe.

Would you be open to reconsider the useZeusVariables design choice? To be honest with you, I'm not sure I see how it fits the spirit of graphql-zeus thus far. Getting all the types automatically from Zeus has been super easy with zero boilerplate needed, which was what had me drawn to it as a user. The new approach to variables goes in a different direction.

@aexol
Copy link
Collaborator

aexol commented Apr 27, 2022

if it is just about inferring the type without specifying it I can do it

@spion
Copy link
Author

spion commented Apr 27, 2022

if it is just about inferring the type without specifying it I can do it

I'm not sure what you mean - would there still be useZeusVariables, or would it be possible to just use something like $("var")? I tried the approach with creating a variables proxy that inferred variables on usage but TS inference didn't quite cooperate in that case...

To be more specific, the full list of useZeusVariables issues I have is

  1. no type inference for arguments (with Hasura for example they can be actually quite complex)
  2. need to pass values for the variables (I don't have a value when I construct a TypeDocumentNode originally, the value is only available in the component when the URL specifies an ID or the user submits a form)
  3. the name can interfere with react's official eslint plugin for hooks ESlint: eslint-plugin-react-hooks is going too far  facebook/react#24252

Is it really necessary to have useZeusVariables? At least for TypedDocumentNode it was possible to make it work without a special call to create variables - were there other use cases where it was impossible?

@aexol
Copy link
Collaborator

aexol commented Apr 29, 2022

Yes we can go back to the old way, but with a more clever approach now.

@aexol aexol reopened this Jun 29, 2022
@aexol
Copy link
Collaborator

aexol commented Jun 30, 2022

I made it in 5.1.3 just opened this PR to show your contribution somehow.

@aexol
Copy link
Collaborator

aexol commented Jul 1, 2022

@spion

@ilijaNL
Copy link

ilijaNL commented Sep 1, 2022

I will bump this. I totally agree with @spion that specifying the input type is really error prone and should be automatically inferred (with help of the introduced scalars). If I have an query with $('user_id', 'uuid!'), someone could easily put in a String! instead. The error will be only catched runtime, thus making variables kind of useless in graphql-zeus context

@spion
Copy link
Author

spion commented Sep 1, 2022

Sorry @aexol looks like I forgot to reply to this thread.

I decided to go in a different direction. I quite liked the tql API with some small exceptions (method calls, lack of automatic inferrence for deeply nested variables) so I decided to write https://typed-graphql-builder.spion.dev/

I want to properly credit zeus though, I learned some cool clever tricks for the implementation to reduce compilation size compared to tql results, which is especially great when working with tools like Hasura.

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.

3 participants