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

Add a more complex example #24

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

kivutar
Copy link

@kivutar kivutar commented Jan 2, 2018

This example is closer to a real life use of the library. It shows how to send data to subscribers as result of a mutation, as it is often the case in graphql. It also shows how to subscribe to a topic and broadcast only to the subscribers of this topic.

@kivutar
Copy link
Author

kivutar commented Jan 3, 2018

With this last commit 7465fba, we need this PR merged: graphql-go/handler#33

Copy link
Contributor

@Jannis Jannis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a couple of things that I'd like to see improved but overall, everything looks very good. Once the "problems" (most of them are of cosmetic nature) are addressed, I see nothing that blocks the PR from getting merged.

log "github.com/sirupsen/logrus"
)

type Document struct {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is a local type (to the file), it should be document, otherwise golint complains:

➜  graphqlws git:(kivutar/master) ✗ golint examples/server/
examples/server/main.go:13:6: exported type Document should have comment or be unexported

"document": &graphql.Field{
Type: documentType,
Args: graphql.FieldConfigArgument{
"docId": &graphql.ArgumentConfig{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is very much a matter of taste but I'd use id instead of docId everywhere.

},
},
Resolve: func(p graphql.ResolveParams) (interface{}, error) {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd remove empty lines at the beginning of code blocks. Like this one.


for _, subscriptions := range subscriptionManager.Subscriptions() {
for _, subscription := range subscriptions {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also remove this one.

Type: graphql.String,
},
"content": &graphql.ArgumentConfig{
Type: graphql.String,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd wrap this and the docId and title types with graphql.NewNonNull() so that they are required to always be present in the mutation. That way you don't have to check if the type assertions in lines 83-85 are ok.

},
Resolve: func(p graphql.ResolveParams) (interface{}, error) {

docID := p.Args["docId"].(int)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you don't wrap the argument types in the definitions, you'd have to check whether the type assertions are successful like so

docID, docIDOk := p.Args["docId"].(int)
if !docIDOK {
    ... return an error or something ...
}

}

var err error
schema, err = graphql.NewSchema(schemaConfig)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

schema is only needed in main(), so I'd remove the var schema graphql.Schema further towards the top and just do the following here:

schema, err := graphql.NewSchema(schemaConfig)

That way you also don't have to declare err as a var before using it.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

schema is also used in the mutation resolver

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, you're right, sorry. Ignore this comment then. :)

for _, subscription := range subscriptions {

// JSON interface is float64
var subdocID int = int(subscription.Variables["docId"].(float64))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is >90 characters. That's the (undocumented) limit we've been sticking to across this repo. Could you break this line into two?


data := graphqlws.DataMessagePayload{
Data: result.Data,
Errors: graphqlws.ErrorsFromGraphQLErrors(result.Errors),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is also >90 characters but this could easily be solved:

Errors: graphqlws.ErrorsFromGraphQLErrors(
	result.Errors,
),

),
}

sub.SendData(sub, &data)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made a breaking change in master, removing the unnecesary *Subscription parameter from the SendData function. So now it's just

sub.SendData(&data)

Pretty: true,
GraphiQL: true,
Endpoint: "http://localhost:8085",
SubscriptionsEndpoint: "ws://localhost:8085/subscriptions",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In github.com/graphql-go/handler, where handler.Config is defined, I don't think Endpoint or SubscriptionsEndpoint are properties.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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