-
Notifications
You must be signed in to change notification settings - Fork 43
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
Added support for parameterized queries and mutations. #20
Conversation
8e47902
to
984dd8b
Compare
984dd8b
to
73c7853
Compare
73c7853
to
fc2e84d
Compare
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.
Had one comment/suggestion about how to define variables in our DSL.
On a separate note originally I thought it would be really useful to support parametrized queries and mutations but the more I think about it, it may not be as useful in our case. Basically why would someone do:
client.query(ids: [42]) do
query(:$ids => :'[Int]') do
invoices(ids: :$ids) do
id
fee_in_cents
end
end
end
as opposed to
client.query do
query do
invoices(ids: [42]) do
id
fee_in_cents
end
end
end
As far as i understand, from Graphql server side, they both are processed the same way and there is no benefit in parametrizing the variables over plain query.
Maybe with parametrized approach its easier to share the DSL block?
Regardless its nice to support them, let me know what do you think about my DSL suggestion.
|
||
```ruby | ||
client.query(ids: [42]) do | ||
query(:$ids => :'[Int]') do |
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.
oo interesting approach, I wonder if we can make this slightly more DSLy by doing things like:
client.query(ids: [42]) do
query(:ids => [:int]) do
....
end
end
This means on our end we need to detect query variables and add $
to them when generating query. What do you think?
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.
Good idea, opened #23 for a future improvement.
fc2e84d
to
cccc062
Compare
Two reasons for doing a parameterized query ala version 1 above:
|
I rebased it, it's ready to merge again. |
On top of #19, added support for parameterized queries and mutations.
Closes #1.