-
-
Notifications
You must be signed in to change notification settings - Fork 250
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
Add param to GraphQLRequest
to identify GET requests
#2329
Conversation
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 |
Yeah I was worried about that part as well. I'll make the change and mark the PR as breaking |
GraphQLRequest.extensions
to identify GET requestsGraphQLRequest
to identify GET requests
extensions: Option[Map[String, InputValue]] = None | ||
) { | ||
extensions: Option[Map[String, InputValue]] = None, | ||
@transient isHttpGetRequest: Boolean = false |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
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 theexecuteRequest
method, but that would require breaking the API, and that might be too soon after the last major version releaseEDIT: 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