Skip to content

Allow custom endpoint URL.#33

Open
kivutar wants to merge 7 commits into
graphql-go:masterfrom
kivutar:master
Open

Allow custom endpoint URL.#33
kivutar wants to merge 7 commits into
graphql-go:masterfrom
kivutar:master

Conversation

@kivutar

@kivutar kivutar commented Jan 3, 2018

Copy link
Copy Markdown

Attempt to address #31
Websocket subscriptions are now working.

@coveralls

Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.2%) to 87.5% when pulling 593f69e on Kivutar:master into cc93f95 on graphql-go:master.

@coveralls

Copy link
Copy Markdown

Coverage Status

Coverage decreased (-1.4%) to 85.876% when pulling 69f77bc on Kivutar:master into cc93f95 on graphql-go:master.

1 similar comment
@coveralls

Copy link
Copy Markdown

Coverage Status

Coverage decreased (-1.4%) to 85.876% when pulling 69f77bc on Kivutar:master into cc93f95 on graphql-go:master.

@Jannis Jannis left a comment

Copy link
Copy Markdown

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!

Comment thread handler.go
Pretty bool
GraphiQL bool
EndpointURL string
SubscriptionsEndpoint string

Copy link
Copy Markdown

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: ...,
    }
})

Copy link
Copy Markdown

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).

Comment thread graphiql.go Outdated
OperationName string
EndpointURL template.URL
EndpointURLWS template.URL
SubscriptionsEndpoint template.URL

Copy link
Copy Markdown

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.

@coveralls

Copy link
Copy Markdown

Coverage Status

Coverage decreased (-1.5%) to 85.795% when pulling aace1d0 on Kivutar:master into cc93f95 on graphql-go:master.

1 similar comment
@coveralls

Copy link
Copy Markdown

Coverage Status

Coverage decreased (-1.5%) to 85.795% when pulling aace1d0 on Kivutar:master into cc93f95 on graphql-go:master.

@kivutar

kivutar commented Jan 5, 2018

Copy link
Copy Markdown
Author

Thanks for your reviews.

If I use a pointer for GraphiQLConfig I have to handle too many pointer dereferences in order to access the values. (I'm a golang beginner)

And if I use a plain struct, my changes are not consistent anymore with the current code, as handler.Config is a pointer.

What should I do?

@coveralls

coveralls commented Jan 5, 2018

Copy link
Copy Markdown

Coverage Status

Coverage decreased (-1.5%) to 85.795% when pulling d34f105 on Kivutar:master into cc93f95 on graphql-go:master.

@coveralls

coveralls commented Jan 5, 2018

Copy link
Copy Markdown

Coverage Status

Coverage decreased (-1.5%) to 85.795% when pulling 9041423 on Kivutar:master into cc93f95 on graphql-go:master.

@ccamel

ccamel commented May 11, 2018

Copy link
Copy Markdown

@kivutar What's the status on this one ? I'd be really interested in having this feature available. 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants