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

build_query Variables parameter should be optional

Open sachaarbonel opened this issue 6 years ago • 3 comments

Hi, thank's for this great library. First of all I'm new to rust and sorry if I missed something obvious here. I was working on a PR against this repo with a basic subscription example using the websocket crate. However, I came across a graphql use case where my subscription query doesn't have any variables. My query :

subscription getMessages {
  message(order_by: {timestamp: desc}) {
    text
    timestamp
    author {
      username
    }
  }
}

I thought as a workaround I could do this :

MyQuery::build_query(MyQuery::Variables::default())

mod my_query {
#[derive(Serialize,Default)]
    pub struct Variables(String);
}

But it doesn't seem to work.

Proposal

Graphql client should generate this impl trait :

impl graphql_client::GraphQLQuery for MyQuery {
    type Variables = my_query::Variables;
    type ResponseData = my_query::ResponseData;
    fn build_query(variables: Option(Self::Variables)) -> ::graphql_client::QueryBody<Self::Variables> {
        graphql_client::QueryBody {
            variables,
            query: get_messages::QUERY,
            operation_name: get_messages::OPERATION_NAME,
        }
    }
}

And it should be used like this :

MyQuery::build_query(None)

sachaarbonel avatar Aug 16 '19 13:08 sachaarbonel

Thanks for opening this issue, it's indeed an ergonomics problem with queries that don't have any variables.

I think we need a bit more design work, because I wouldn't feel comfortable with Option<Variables> since queries that do have variables would fail at runtime if passed None. Since Variables is an associated type on the GraphQLQuery trait, in case of empty variables, we could define it as (), so it would be called as MyQuery::build_query(()). We could even do this in a backwards-compatible way.

Also correct me if I am wrong, but at the moment I think MyQuery::build_query(my_query::Variables) should work.

tomhoule avatar Aug 17 '19 13:08 tomhoule

Thank's for the quick response @tomhoule ! I'm happy with your backward compatible solution, it is both smart and more ergonomic. I'll try you workaround and let you know (I didn't know it was possible to instantiate an empty struct in rust, nice !)

sachaarbonel avatar Aug 19 '19 13:08 sachaarbonel

@tomhoule sorry I just tried your workaround and it generates a null instead of an empty string "" when serializing with serde. My repo to see the behaviour

sachaarbonel avatar Aug 23 '19 20:08 sachaarbonel