Add a more complex example by kivutar · Pull Request #24 · functionalfoundry/graphqlws

kivutar · 2018-01-02T16:32:19Z

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 · 2018-01-03T14:56:07Z

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

Jannis

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.

Jannis · 2018-01-04T20:59:23Z

examples/server/main.go

+	log "github.com/sirupsen/logrus"
+)
+
+type Document struct {


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

Jannis · 2018-01-04T21:23:07Z

examples/server/main.go

+				"document": &graphql.Field{
+					Type: documentType,
+					Args: graphql.FieldConfigArgument{
+						"docId": &graphql.ArgumentConfig{


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

Jannis · 2018-01-04T22:11:55Z

examples/server/main.go

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


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

Jannis · 2018-01-04T22:12:08Z

examples/server/main.go

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


Also remove this one.

Jannis · 2018-01-04T22:13:44Z

examples/server/main.go

+							Type: graphql.String,
+						},
+						"content": &graphql.ArgumentConfig{
+							Type: graphql.String,


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.

Jannis · 2018-01-04T22:15:29Z

examples/server/main.go

+					},
+					Resolve: func(p graphql.ResolveParams) (interface{}, error) {
+
+						docID := p.Args["docId"].(int)


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

Jannis · 2018-01-04T22:18:09Z

examples/server/main.go

+	}
+
+	var err error
+	schema, err = graphql.NewSchema(schemaConfig)


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.

schema is also used in the mutation resolver

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

Jannis · 2018-01-04T22:19:32Z

examples/server/main.go

+							for _, subscription := range subscriptions {
+
+								// JSON interface is float64
+								var subdocID int = int(subscription.Variables["docId"].(float64))


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?

Jannis · 2018-01-04T22:20:32Z

examples/server/main.go

+
+									data := graphqlws.DataMessagePayload{
+										Data:   result.Data,
+										Errors: graphqlws.ErrorsFromGraphQLErrors(result.Errors),


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

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

Jannis · 2018-01-05T13:34:02Z

examples/server/main.go

+										),
+									}
+
+									sub.SendData(sub, &data)


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

sub.SendData(&data)

david4shure · 2018-06-06T21:52:04Z

examples/server/main.go

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


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

See https://github.com/graphql-go/handler/blob/master/handler.go#L162

You will also need this PR https://github.com/graphql-go/handler/pull/33/files

Jannis self-requested a review January 4, 2018 20:34

Jannis added enhancement pending review labels Jan 4, 2018

Jannis requested changes Jan 4, 2018

View reviewed changes

Jannis mentioned this pull request Jan 5, 2018

subscriptions: Convert GraphQL errors to nil if they are empty #25

Merged

Jannis reviewed Jan 5, 2018

View reviewed changes

kivutar added 11 commits January 5, 2018 20:35

Add a more complex example

eaed1bd

Resolve at the function level

ecafd07

Add subscription filter on docId

a7b92c7

Set graphiql subscriptions endpoint

7710a46

Don't export document

a342c2a

Use id in place of docId

d4bebe8

wrap mutation arguments with graphql.NewNonNull

a614736

Remove useless new lines

cdf65d8

Use short variable names

fd92641

EndpointURL was renamed to Endpoint in graphql-go/handler

c881694

Update SendData signature

fde93d7

kivutar force-pushed the master branch from 5e9b53c to fde93d7 Compare January 5, 2018 14:05

Jannis approved these changes Jan 9, 2018

View reviewed changes

david4shure reviewed Jun 6, 2018

View reviewed changes


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

Conversation

kivutar commented Jan 2, 2018

Uh oh!

kivutar commented Jan 3, 2018

Uh oh!

Jannis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants