yuki24 / artemis

Ruby GraphQL client on Rails that actually makes you more productive
MIT License
207 stars 14 forks source link

Allow to dynamically call the operation #62

Closed JanStevens closed 5 years ago

JanStevens commented 5 years ago

Hello,

I have a usage where I have some queries who can basically triggered by different UI actions, for this reason I would like to have an execute or call_operation that would accept a first param the name of the operation and then all the other args. For example

client.query_event(id: 1)

vs

client.execute(:query_event, id: 1)

This would allow to to make the :query_event param configurable. Internally it could also clean up some code so that the method_missing would delegate to the execute method instead.

 # Executes a given query, raises if we didn't define the operation
  #
  # @param [String] operation
  # @param [Hash] context
  # @param [Hash] arguments
  # @return [GraphQL::Client::Response]
  def execute(operation, context: {}, **arguments)
    raise Artemis::GraphQLFileNotFound unless self.class.resolve_graphql_file_path(operation)
    const_name = operation.to_s.camelize
    self.class.load_constant(const_name) unless self.class.const_defined?(const_name)
    client.query(self.class.const_get(const_name), variables: arguments, context: context)
  end
yuki24 commented 5 years ago

At Artsy we have a controller action that does what you described, which could be summarized to:

CLASS_MAP = {
  'accept' => AcceptOfferMutation,
  'confirm' => ApproveOrderMutation,
  'counter' => CounterOfferMutation,
  'decline' => DeclineOfferMutation,
  'pickup' => ConfirmPickupOrderMutation,
  'ship' => FulfillOrderMutation
}

# In controller:
def action
  type  = params[:mutation_type]
  klass = CLASS_MAP[type]

  # call the GraphQL API
  klass.do_stuff
end

@jonallured do you think we could clean up some of the code here if there was the execute method suggested above?

jonallured commented 5 years ago

Hmmm, maybe! Being able to dynamically call an operation seems pretty neat, but then I would still be left with a response that has the name of the operation in it - for example:

operation_arguments = {}
response = client.approve_order(operation_arguments)
order = response.data.ecommerce_approve_order.order_or_error

Here there are two hard-coded pieces of information - the name of the operation (approve_order) and the name of the key in the data where my object is (ecommerce_approve_order).

So if I could do this:

operation_arguments = {}
operation_name = :approve_order
response = client.execute(operation_name, operation_arguments)

Then I'd still need to be able to do this:

operation_response_name = :ecommerce_approve_order
order = response.data[operation_response_name].order_or_error

I guess what I'm saying is that unless I can be dynamic on both the execution AND parsing of the response, this dynamism isn't really that valuable to me - does that make sense?

yuki24 commented 5 years ago

Thanks for the explanation @jonallured! So this wouldn't remove the need of an extra conditional unless the dynamically specified queries all have a consistent shape of response.

So in my imagination this:

case identifier
when 'my_query'
  Client.my_query(...)
when 'other_query'
  Client.other_query(...)
when ...

Could be re-written to:

data = Client.execute(my_query, ...) # Done!

When the reality is:

data = Client.execute(my_query, ...)

case identifier
when 'my_query'
  extract_data_from_my_query(data) # or render "my_query", ...
when 'other_query'
  extract_data_from_other_query(data) # or render "other_query", ...
when ...

Which does not look like a significant improvement.

@JanStevens Is your source code available online? Or is it possible to share with us a summarized snippet of code that you think could be simplified with the proposed method?

JanStevens commented 5 years ago

We rename the requested fields so they line up

mutation($input: [IdentifierTypeInput!]!) {
  action: identifierTypeUpsertMulti(input: $input) {
    response: identifierTypes {
      id
      name
    }
  }
}

This way our client can be generic

data = Client.execute(my_query, ...)
data.action.response

I must admit that the usage might be very slim but I like the concept overall since it then extract a lot of logic from method missing. I personally don't like to much magic in method missing and always let it delegate to another method