venia icon indicating copy to clipboard operation
venia copied to clipboard

Add support for variable-types apart from `keyword`

Open itadventurer opened this issue 6 years ago • 0 comments

I cross-posted this issue at the seemingly most-active fork https://github.com/district0x/graphql-query as this repository seems to be dead. Issue: https://github.com/district0x/graphql-query/issues/4

Currently the spec describes allows to be a :variable/type to be only a keyword:

(s/def :variable/type keyword?)

This works fine in many cases but is for our use case a serious limitation.

I would propose to add functionality to suport all valid GraphQL input types as described in the GraphQL Spec.

Here are some examples:

I will use following query as an example (and modify it accordingly):

query Foo($Foo:Int){employee{name}}

The corresponding Venia query (as it works now without problems) is:

{:venia/operation {:operation/name "Foo" :operation/type :query}
 :venia/variables
 [{:variable/name "Foo"
   :variable/type :Int}]
 :venia/queries [[:employee [:name]]]}

I will focus now only on the :venia/variables part:

Simple variable

GraphQL:

query Foo($Foo:Int){employee{name}}

Variables:

[{:variable/name "Foo"
   :variable/type :Int}]

Required variable

GraphQL:

query Foo($Foo:Int!){employee{name}}

Variables:

[{:variable/name "Foo"
  :variable/type {:type/type :Int
                  :type/required? true}}]

Another option would be to add a :variable/required? key to the variable map but I personally dislike this option as the required-property is part of the type and not the variable definition.

List

GraphQL:

query Foo($Foo:[Int!]!){employee{name}}

Variables:

[{:variable/name "Foo"
  :variable/type {:type/kind :list ;; :type/kind instead of :type/type!
                  :type.list/items {:type/type :Int
                                    :type/required? true}
                  :type/required? true}}]

I do not particulary like the :type/kind declaration as there is only one valid value (:list) for it but I do not see another nice way to do so. I mislike the idea to add a special :type/type called :List as it is not reserved by the GraphQL specification (as far as I can see) and someone may add a custom :List type to their schema and then this will break.

Final remarks

According to the GraphQL specification this are the only valid options for the variable types.

Implementation

@r0man already implemented a way to add Lists as types here but it misses following patterns (when I read the code correctly): [Int]!, [[Int]] and so on.

I will provide an implemenation (and create a PR) as soon as possible but would be happy for any feedback for this solution approach.

itadventurer avatar Aug 21 '18 16:08 itadventurer