search

github-community-projects / graphql-client

A Ruby library for declaring, composing and executing GraphQL queries
MIT License
45 stars 218 forks source link

TODO for graphql-client 1.0 #15

Open rmosolgo opened 9 months ago

rmosolgo commented 9 months ago

This library has been around a long time, and it's a hard dependency for a lot of projects. I'd like to release a 1.0.0 version, and before that, I have at least a few things in mind:

If anyone has other suggestions for 1.0, please share them in a comment below.

DawidJanczak commented 9 months ago

First off: thanks for taking the project over and reviving it!

There's one thing that would help us a bit: it would be great to have access to response headers from GraphQL HTTP call. I believe those were either discarded or just not returned by the client. Do you think that's doable?

rmosolgo commented 9 months ago

Hey, yes, I think it is. Here's where the request details are dropped:

https://github.com/github-community-projects/graphql-client/blob/925e3c7deab395f417d49a6c156d25d8e2d5c48e/lib/graphql/client/http.rb#L73-L80

I bet we can modify the returned object there to make it include the HTTP response and retain compatibility. I'll take a look!

billybonks commented 7 months ago

@rmosolgo Given that the response is either the body or the errors returned object, there is to make it include the HTTP response and retain compatibility.

There are a few ways we could achieve this

  1. Extend the hash to include a meta key. I'm curious to know how many people are integrating unthinkingly over the root keys. It also makes it seem the caller returned that meta.

  2. would be to create an object that proxies to the hash but has extra functions to return metadata about the results.

  3. one option would be to have a method on the client called "last_request_metadata".

One is a breaking change, and I would instead create a breaking change for 1.0 that is clearer. 2 I am not sure exactly how to code this in ruby, so I would not be able to help immediately. 3. This could cause issues in a multi-threaded environment.

If we do create a breaking change we can include an option pre 1.0 that gives 1.0 behaviour to allow early usage and migration. This feature is quite important to me to handle rate limits so happy to make contributions.

let me know your thoughts.

Samsinite commented 2 months ago

@rmosolgo Given that the response is either the body or the errors returned object, there is to make it include the HTTP response and retain compatibility.

There are a few ways we could achieve this

  1. Extend the hash to include a meta key. I'm curious to know how many people are integrating unthinkingly over the root keys. It also makes it seem the caller returned that meta.
  2. would be to create an object that proxies to the hash but has extra functions to return metadata about the results.
  3. one option would be to have a method on the client called "last_request_metadata".

One is a breaking change, and I would instead create a breaking change for 1.0 that is clearer. 2 I am not sure exactly how to code this in ruby, so I would not be able to help immediately. 3. This could cause issues in a multi-threaded environment.

If we do create a breaking change we can include an option pre 1.0 that gives 1.0 behaviour to allow early usage and migration. This feature is quite important to me to handle rate limits so happy to make contributions.

let me know your thoughts.

Number 3 seems quite hacky, out of these 3 options I'm partial to number 2.

Another potential solution would be to be able to opt into the old API that returns JSON.parse, otherwise the new API can return a new object that can contain the original response and provide easy access to data. Using the old API could issue a deprecation warning that support for this usage would be removed in v2.0. This would allow the API to evole into something that might be preferable, but allow applications to continue to upgrade to 1.0 as long as they set this configuration appropriately, and provide a means to iterate to the new API at their convience. Maybe even retain compatibility that it quacks like a hash (I'm not sure how doable this is Hash has a decently sized API, maybe just make commonly used hash methods available or use method_missing/respond_to? to proxy it), but issues a deprecation warning when used like this.

billybonks commented 1 week ago

@Samsinite i created this pr eventually https://github.com/github-community-projects/graphql-client/pull/43, the client API followed number 2, but used number 3 to move the data around in the lib in order to produce the output.

The reason is described in the pr.

@rmosolgo we can probably tick Return response metadata in HTTP client results since now consumers can access the HTTP response directly

rmosolgo commented 1 week ago

Thanks for the reminder on that, @billybonks. I also did the perf audit a long time ago and found that we'd already got all the major hotspots. I checked those both off.