rmosolgo / graphiql-rails

Mount the GraphiQL query editor in a Rails app
MIT License
447 stars 135 forks source link

Add :self as option for graphql_path #9

Closed jamesreggio closed 7 years ago

jamesreggio commented 8 years ago

I would like to serve GraphiQL from the same path as my GraphQL endpoint. (GraphiQL is returned for GETs, and query results are returned for POSTs.)

I need this feature, because I'd like to package both GraphQL and GraphiQL into a mountable engine (in a gem), and I won't actually know the full graphql_path at the time that my engine's routes are drawn.(It's determined by where my gem's engine is mounted by the Rails app.)

This PR allows use of :self to indicate the the GraphQL endpoint is the same URL that GraphiQL loaded from. I've added tests and documentation for this minor change.

jamesreggio commented 8 years ago

@rmosolgo: this is a fantastically convenient project, by the way. Thanks for all your GraphQL contributions to the Ruby community.

rmosolgo commented 8 years ago

Sounds like a cool project!

How does this compare to passing an empty string as the graphql path? If that also sends queries to the mount path, would it be a suitable implementation for this behavior?

jamesreggio commented 8 years ago

Thanks. If you don't mind, I may reach out for a quick review before I publish in a couple days.

Regarding the use of an empty string, it certainly could work here, but I feel like it's a bit more error prone. Imagine we're loading the path from the app config, like such:

Rails.application.routes.draw do
  mount GraphiQL::Rails::Engine, at: '/graphiql', graphql_path: Rails.application.config.graphql_path
end

If we end up with an empty value in the config, I think it'd be better to raise an error here versus silently defaulting to sending queries to /graphql.

Definitely your call, though.

jamesreggio commented 8 years ago

@rmosolgo, could you let me know what you think about the comment above?

rmosolgo commented 8 years ago

Thanks for the bump! Sorry I didn't get back to you before.

an empty value in the config

What kind of value are you worried about here?

So, unless sending "" to fetch is not good behavior (I mean, if I've misunderstood and it's undefined behavior, or subject to bugs), I'd prefer to keep the String-only type for graphql_path!

jamesreggio commented 8 years ago

an empty value in the config

I tried to illustrate this concern in this comment, where an empty key/value pair in a YAML file could result in unexpected, post-to-same-URL behavior.

Another possibility is that I use ENV['GRAPHQL_PATH'] to specify the path, and if I fail to set that value in my environment, I'll get unexpected behavior instead of an explicit error.

However, this isn't something worth quibbling over :)

Regarding an empty url, the standard shows that its required, and the most common window.fetch polyfill doesn't gracefully handle an undefined value, so I think this PR (with the || window.location) is still necessary. I've removed the :self symbol; let me know if it needs any further revisions.