graphql-zeus icon indicating copy to clipboard operation
graphql-zeus copied to clipboard

Implement TypedDocumentNode support (with type-safe variables)

Open spion opened this issue 2 years ago • 14 comments

I took at stab at fully implementing https://github.com/graphql-editor/graphql-zeus/issues/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 avatar Apr 22 '22 17:04 spion

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

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?

spion avatar Apr 22 '22 18:04 spion

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

aexol avatar Apr 24 '22 13:04 aexol

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.

spion avatar Apr 24 '22 20:04 spion

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

aexol avatar Apr 27 '22 08:04 aexol

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

aexol avatar Apr 27 '22 09:04 aexol

@spion

aexol avatar Apr 27 '22 09:04 aexol

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.

spion avatar Apr 27 '22 12:04 spion

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

aexol avatar Apr 27 '22 17:04 aexol

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 https://github.com/facebook/react/issues/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?

spion avatar Apr 27 '22 17:04 spion

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

aexol avatar Apr 29 '22 09:04 aexol

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

aexol avatar Jun 30 '22 14:06 aexol

@spion

aexol avatar Jul 01 '22 21:07 aexol

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

ilijaNL avatar Sep 01 '22 05:09 ilijaNL

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.

spion avatar Sep 01 '22 09:09 spion