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

Add param to GraphQLRequest to identify GET requests #2329

Merged
merged 5 commits into from
Sep 20, 2024

Conversation

kyri-petrou
Copy link
Collaborator

@kyri-petrou kyri-petrou commented Jul 6, 2024

In short, I would like to reduce our reliance on FiberRef to propagate information across different parts in Caliban for various reasons but primarily because updating FiberRefs continuously will result in degraded performance. With this PR, we use the GraphQLRequest extensions to propagate that information as it's much cheaper to do so.

@ghostdogpr @paulpdaniels Do you see an issue with this approach? Alternatively, we could add a parameter to the executeRequest method, but that would require breaking the API, and that might be too soon after the last major version release

EDIT: Based on @paulpdaniels recommendation, and since we're updating the minor version, I opted for the bincompat breaking change by adding the param to GraphQLRequest

@paulpdaniels
Copy link
Collaborator

I'm not really in favor of this approach. I'd move toward pushing the value into the graphql request directly instead of mixing it with user-land code. Even if that means waiting for 2.8.0

@kyri-petrou
Copy link
Collaborator Author

Yeah I was worried about that part as well. I'll make the change and mark the PR as breaking

@kyri-petrou kyri-petrou added the breaking change The PR contains binary incompatible changes label Jul 9, 2024
@kyri-petrou kyri-petrou changed the title Use GraphQLRequest.extensions to identify GET requests Add param to GraphQLRequest to identify GET requests Jul 9, 2024
extensions: Option[Map[String, InputValue]] = None
) {
extensions: Option[Map[String, InputValue]] = None,
@transient isHttpGetRequest: Boolean = false
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this do in the context of a case class member?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one comes from jsoniter (it's not the Scala one). It excludes the case class member from serialization and always uses the default value for deserialization

@kyri-petrou kyri-petrou merged commit f45e0d3 into series/2.x Sep 20, 2024
11 checks passed
@kyri-petrou kyri-petrou deleted the propagate-http-request-method-via-extension branch September 20, 2024 10:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change The PR contains binary incompatible changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants