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

TODO for graphql-client 1.0 #15

Open
2 of 3 tasks
rmosolgo opened this issue Feb 12, 2024 · 6 comments
Open
2 of 3 tasks

TODO for graphql-client 1.0 #15

rmosolgo opened this issue Feb 12, 2024 · 6 comments
Milestone

Comments

@rmosolgo
Copy link
Collaborator

rmosolgo commented Feb 12, 2024

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:

  • Performance audit, especially for view helpers and runtime code
  • Find a way to support subscriptions
  • Return response metadata in HTTP client results

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

@rmosolgo rmosolgo added this to the 1.0.0 milestone Feb 12, 2024
@DawidJanczak
Copy link

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
Copy link
Collaborator Author

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

response = connection.request(request)
case response
when Net::HTTPOK, Net::HTTPBadRequest
JSON.parse(response.body)
else
{ "errors" => [{ "message" => "#{response.code} #{response.message}" }] }
end
end

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
Copy link
Contributor

@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
Copy link
Contributor

Samsinite commented Aug 26, 2024

@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
Copy link
Contributor

billybonks commented Nov 14, 2024

@Samsinite i created this pr eventually #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
Copy link
Collaborator Author

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.

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

No branches or pull requests

4 participants