Skip to content
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

Don't assume server is mounted at /graphql #146

Merged
merged 1 commit into from
Jul 5, 2016

Conversation

joshprice
Copy link

The GraphQL server happens to be mounted at graphql in this case (https://github.com/graphql/graphiql/blob/master/example/server.js#L28), but we shouldn't assume that.

This fix should allow the GraphQL server to be mounted anywhere.

The reason this is important is that we use this template in GraphQL Elixir (other implementations would likely do something similar). Our template can be found here https://github.com/graphql-elixir/plug_graphql/blob/master/templates/graphiql.eex

We found some issues when we took this template verbatim before line 111.

@ghost
Copy link

ghost commented Jun 15, 2016

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at [email protected]. Thanks!

aweiker added a commit to graphql-elixir/plug_graphql that referenced this pull request Jun 15, 2016
This includes the fix made in the PR
graphql/graphiql#146

This fix is required so that only a single query and operation are sent
to the server as GraphiQL wil also include all parameters in the web
browsers URL.
@joshprice
Copy link
Author

CLA is signed, also I have investigated those test failures and could not figure out why the linter is broken. This is also happening on master locally for me so suspect this might be a dependency change causing this.

@facebook-github-bot
Copy link

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@asiandrummer
Copy link
Contributor

@joshprice - Sorry for keeping you hanging! I'm taking a look at the linter issue, and although I have no idea why this is happening yet, I'll see if I can unblock you from this as soon as possible.

The GraphQL server happens to be mounted at `graphql` in this case (https://github.com/graphql/graphiql/blob/master/example/server.js#L28), but we shouldn't assume that.

This fix should allow the GraphQL server to be mounted anywhere.

The reason this is important is that we use this template in [GraphQL Elixir](https://github.com/graphql-elixir/graphql) (other implementations would likely do something similar). Our template can be found here https://github.com/graphql-elixir/plug_graphql/blob/master/templates/graphiql.eex

We found some issues when we took this template verbatim before line 111.
@joshprice
Copy link
Author

Rebased and pushed to retrigger the build, looks like it was transient. I did see an explanation somewhere but can't seem to track it down right now.

@asiandrummer
Copy link
Contributor

asiandrummer commented Jun 23, 2016

Me neither, but kudos for staying on top of it - thanks for the contribution :D
I was caught up solving this linter issue, but what do you think about optionally parameterizing the custom endpoint pathname? Something like:

function graphQLFetcher(graphQLParams, endpointURL = '/graphql') {
  return fetch(window.location.origin + endpointURL, {});
}

This way we can customize the URL as we want, but it'll default to /graphql when not supplied. Aside form the customization, I was also thinking this change may break other applications if it relies on /graphql URL to use fetcher somehow.

@joshprice
Copy link
Author

As mentioned in the issue description we have the situation where we have many GraphQL endpoints mounted at various locations and your code would involve us configuring GraphiQL differently for each endpoint whereas using the current path makes it work transparently.

@asiandrummer
Copy link
Contributor

Sure - again, thanks for the contribution!

@asiandrummer asiandrummer merged commit 6f382b2 into graphql:master Jul 5, 2016
@joshprice joshprice deleted the patch-1 branch September 8, 2016 12:00
asiandrummer added a commit to asiandrummer/plug_graphql that referenced this pull request Nov 1, 2016
[This PR](graphql/graphiql#146) changed the fetcher URL to be relative - due to my carelessness this broke GraphiQL example code. I'd like to propose the change that will let you keep the relativeness of the fetcher URL and still have a specific endpoint URL for graphql execution-related requests.
acao pushed a commit to acao/graphiql that referenced this pull request Jun 1, 2019
acao pushed a commit to acao/graphiql that referenced this pull request Jun 5, 2019
Fix typo in resources/README.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants