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

Subscription execution #1

Merged

Conversation

remorses
Copy link

What i changed

  • split the Subscribe function in Subscribe (similar to graphql.Do) and ExecuteSubscription (similar to graphql.Execute)
  • Subscribe now does the parsing and validation, more similar to graphql.Do
  • removed the added context as there was already one in the params
  • rewritten tests as a grid
  • added testutils for subscriptions tests
  • added a SubscriptableSchema that can be used with https://github.com/graph-gophers/graphql-transport-ws

I still have to check if it works ok with the https://github.com/graph-gophers/graphql-transport-ws handler

subscription.go Outdated
resultChannel := make(chan *Result)
// SubscriptableSchema implements `graphql-transport-ws` `GraphQLService` interface: https://github.com/graph-gophers/graphql-transport-ws/blob/40c0484322990a129cac2f2d2763c3315230280c/graphqlws/internal/connection/connection.go#L53
// you can pass `SubscriptableSchema` to `graphql-transport-ws` `NewHandlerFunc`
type SubscriptableSchema struct {
Copy link
Owner

Choose a reason for hiding this comment

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

While I agree this is useful, there is no reason this needs to be part of the library. It can be implemented outside of the library as all the types it relies on are exported.

Copy link
Author

Choose a reason for hiding this comment

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

Where should this type live? handler package?

Copy link
Author

Choose a reason for hiding this comment

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

i removed it, this will probably live in the handler package as a NewSubscriptionHandler function that uses graphql-transport-ws under the hood

If you want to try some subscirption i made an example here

Copy link
Owner

Choose a reason for hiding this comment

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

i removed it, this will probably live in the handler package as a NewSubscriptionHandler function that uses graphql-transport-ws under the hood

If you want to try some subscirption i made an example here

yes, a different package. Feel free to PR it into the graphql-go-tools if you don't have another place for it. My request there is that it be its own package in a subfolder

subscription.go Outdated Show resolved Hide resolved
fmt.Println("strange program path")
return
}
resultChannel <- &Result{
Copy link
Owner

Choose a reason for hiding this comment

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

if the resultChannel is closed before this runs, this will panic which is why I had it in this deferred function at the end

Copy link
Author

Choose a reason for hiding this comment

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

the only way the channel can close is the defer close(resultChannel), this means the channel will be closed after ther error handling (defer functions are run from last to first)

I will add a test case for a panic

Copy link
Owner

@bhoriuchi bhoriuchi left a comment

Choose a reason for hiding this comment

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

looks good

@bhoriuchi bhoriuchi merged commit fd2a748 into bhoriuchi:subscription-execution May 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants