-
Notifications
You must be signed in to change notification settings - Fork 34
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
Port github.com/matiasanaya/graphql-transport-ws #1
Conversation
README.md
Outdated
@@ -1,2 +1,2 @@ | |||
# graphql-transport-ws | |||
WebSocket transport for GraphQL subscriptions | |||
WebSocket transport for GraphQL |
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.
Please, add a short description in the README.md
.
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.
Added some info.
README.md
Outdated
|
||
A Go package that leverages WebSockets to transport GraphQL subscriptions, queries and mutations implementing the [[email protected] protocol](https://github.com/apollographql/subscriptions-transport-ws/blob/v0.9.4/PROTOCOL.md) | ||
|
||
See [this repo](https://github.com/matiasanaya/go-graphql-subscription-example) for an example usage. |
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'm happy this to be a link, but ideally this project should be moved here under example
folder. WDYT?
Or maybe even better under the example folder of the graphql-go
.
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.
Do you want to send a PR, once I merge the subscriptions work?
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.
Yes, that would be ideal. I've copy pasted some of the code into the README so that the link is a last resort at this point.
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.
Please, remove the first select and you'll end up with the same behaviour. Then, I'd be happy to merge this.
case <-stop: | ||
return | ||
default: | ||
} |
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.
Actually, on a second look, this select statement is not needed at all. If the <-stop
operation unblocks then you return and exist the func. But you have the same thing in the second select as well. So this is unnecessary duplication.
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.
Last thing - remove the first select and it's done.
A big woop woop! to everyone that was involved in making this pr part of master! |
No description provided.