DmitryTsepelev / graphql-ruby-persisted_queries

Persisted queries for graphql-ruby
MIT License
170 stars 20 forks source link

Keep extensions in the query context #25

Closed DmitryTsepelev closed 4 years ago

bmorton commented 4 years ago

Not sure exactly what you mean here, but one thing my team noticed is that this plugin deletes the extensions so it would not be compatible with any other plugins that require extensions to be passed. I investigated fixing this, but if we don't delete it, then graphql-ruby throws an exception because the keyword argument isn't expected. I was thinking about opening an issue in graphql-ruby for this.

Is this related?

DmitryTsepelev commented 4 years ago

There was an idea by @JanStevens to put extensions to the context instead of passing them along with it (which makes sense). graphql-ruby does not implement extensions keyword at all, because it does not implements none of features that require this parameter. It means that other gems that use it will have to do the same thing and delete extensions from the params. However, it looks like if we put extensions to the context in the controller, then they will be available for other gems to use (but they'll have to follow this convention). What do you think? By the way, do you know any examples of other plugins that use extensions?

JanStevens commented 4 years ago

@rmosolgo sorry to bring you into this but I think this needs to be settled by the graphql-ruby maintainer

Whats the preferred way for extensions? This gem adds some additional functionality but needs now to patch it self in between. Would it be an idea to extend graphql-ruby to support this behaviour (and maybe is there support for it from graphql spec?)

Related: https://github.com/rmosolgo/graphql-ruby/issues/2751

DmitryTsepelev commented 4 years ago

and maybe is there support for it from graphql spec?

GraphQL spec does not describe extensions, all these tricks like APQ are build on top of it πŸ™‚It's just a convention between client a server like "hey, if I send you hash instead of the query could you please take a look at your storage and substitute the query if you can, and then give it to the execution engine as usual (or otherwise β€” let me know so I send you a full version)"

rmosolgo commented 4 years ago

In my own work, I've taken values from params[:extensions] and "manually" added them to context. For example, OperationStore:

# app/controllers/graphql_controller.rb
context = {
  # ...
  # Support Apollo Link:
  operation_id: params[:extensions][:operationId]
}

Any reason not to do it that way? Why should extensions: be a separate input?

Or, we could have a convention of adding all extensions to context, eg

context = {
  extensions: params[:extensions], 
  # ...
}
DmitryTsepelev commented 4 years ago

I think I'd go with the second option (extensions: params[:extensions]), that should perfectly fit our needs, thank you πŸ™Œ