-
Notifications
You must be signed in to change notification settings - Fork 135
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
Allow custom endpoint URL. #33
base: master
Are you sure you want to change the base?
Conversation
1 similar comment
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.
I'd recommend separating the GraphiQL configuration parameters from the handler configuration a bit. Apart from that this looks great!
handler.go
Outdated
Pretty bool | ||
GraphiQL bool | ||
EndpointURL string | ||
SubscriptionsEndpoint string |
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.
Instead of making EndpointURL
and SubscriptionsEndpoint
top-level configuration fields, it would probably be cleaner to group them under a GraphiQLConfig
field that can optionally be passed in together with GraphiQL: true
. Example:
handler.New(&handler.Config{
Schema: ...,
GraphiQL: true,
GraphiQLConfig: &handler.GraphiQLConfig{
Endpoint: ...,
SubscriptionsEndpoint: ...,
}
})
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.
I noticed that the Apollo server code for GraphiQL mentioned in #31 has a few more configuration variables that might make sense to add here (like additional HTTP headers to be passed to the endpoint for e.g. authentication).
graphiql.go
Outdated
OperationName string | ||
EndpointURL template.URL | ||
EndpointURLWS template.URL | ||
SubscriptionsEndpoint template.URL |
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.
Only EndpointURLWS
is being used in the template, SubscriptionsEndpoint
isn't. I personally favor the name SubscriptionsEndpoint
and would pass Endpoint
and SubscriptionsEndpoint
(falling back to Endpoint
if not set) in to the template.
1 similar comment
Thanks for your reviews. If I use a pointer for And if I use a plain What should I do? |
@kivutar What's the status on this one ? I'd be really interested in having this feature available. 😄 |
Attempt to address #31
Websocket subscriptions are now working.