Support subscriptions #203

matiasanaya · 2018-05-05T23:46:13Z

This addresses the subscription part of #15.

Please look through subscription_test.go which has an example subscription resolver implemented.

I've left // TODO: ... markers where I feel the implementation could be improved, though not sure how.

Missing from this PR is tracing for the execution of each query.

I've put together a graphQL server that supports subscriptions as a proof of concept based on these changes (though it might be out of sync since this PR is changing)

pavelnikolov · 2018-05-06T10:58:40Z

internal/exec/subscribe.go

+					// Send response within timeout
+					// TODO: maybe block until sent?
+					select {
+					case <-subCtx.Done():


Should you send a timeout error here?

Don't believe it would make sense to try and send a timeout error since we've time'd out waiting to send an actual response. But I do think maybe this should be checking against ctx.Done() instead. Thoughts?

pavelnikolov · 2018-05-06T11:02:26Z

subscription_test.go

+import (
+	"context"
+	"encoding/json"
+	stdErrors "errors"


I'd personally add an alias to the other "errors" package and leave the standard lib one as is.

pavelnikolov · 2018-05-06T11:06:34Z

subscription_test.go

+					helloSaid {
+            msg
+          }
+				}


Please, fix indentation above.

pavelnikolov · 2018-05-06T11:07:31Z

subscription_test.go

+	return r.msg, r.err
+}
+
+func closedUpstream(rr ...*helloSaidEventResolver) <-chan *helloSaidEventResolver {


Should this be named closedChannel instead? For a moment, I got confused about the meaning of upstream here.

pavelnikolov · 2018-05-06T11:12:13Z

subscription_test.go

+				subscription onHelloSaid {
+					helloSaid {
+		        msg
+		      }


Fix indentation.

pavelnikolov · 2018-05-06T11:12:29Z

subscription_test.go

+				subscription onHelloSaid {
+					helloSaid {
+		        msg
+		      }


Fix indentation.

pavelnikolov · 2018-05-06T11:13:00Z

subscription_test.go

+
+	type Query {
+		hello: String!
+	}


Fix indentation.

pavelnikolov · 2018-05-06T11:13:10Z

subscription_test.go

+const schema = `
+  schema {
+    subscription: Subscription,
+		query: Query


Fix indentation.

subscription_test.go

pavelnikolov · 2018-05-06T11:31:12Z

This is a good start! Thank you @matiasanaya!
I know it is outside of the scope of this PR, but it would be really nice to add a subscription to the StarWars example!

pavelnikolov · 2018-05-07T11:23:48Z

lgtm

ghost · 2018-05-22T12:35:30Z

Hello everyone who are pursuing the endeavor of implementing subscriptions. How it is going recently? Do you need assistance?

tsauvajon · 2018-05-23T09:52:55Z

We strongly need subscriptions aswell, we would be glad to help if any work remains before merging this PR!

matiasanaya · 2018-06-03T10:53:23Z

@Xeizzen @tsauvajon - I've written a new transport library that aligns with this library and updated the subscription example server with a more robust implementation. If you are keen, please have a look at those projects, try them out and contribute :)

tsauvajon · 2018-06-13T09:36:51Z

LGTM, I tried it in our project and it works as planned.
I've used you "say hello" exemple only at the moment (poudre-aux-yeux/rapiquette@271f9c7), but I will share when I go further.

EDIT: we implemented some features with this and it's perfect for our needs. Subscriptions work as planned. We love the use of channels for this.

tsauvajon · 2018-06-14T09:50:15Z

The thing I don't really understand is

select {
  case r.pointScoredEvents <- e:
  case <-time.After(1 * time.Second):
}

Why use a select, and why this time.After ?

pavelnikolov · 2018-06-14T09:55:29Z

The above select statement will block until one of these happen:

either a message is received from the r.pointScoredEvents channel (if there is no buffer or buffer capacity); or
the operation times out after 1 second

In other words - wait for someone to receive your message (i.e. e), but don't wait for more than 1 second.

ande8133 · 2018-06-18T02:52:17Z

We also really need subscriptions. I was wondering how close this is to being checked in. =)

tsauvajon · 2018-06-18T06:53:19Z

@ande8133 check @matiasanaya 's link. Subscriptions work for me following his example. I just had to add CORS and return 200 on OPTIONS (for preflight requests).

kivutar

I've got it working using this PR too. We should definitely merge this. It's a very good starting point.

stephenafamo · 2018-09-17T12:29:16Z

Is there a reason why this is not yet merged?
Is any help needed?
Everything seems to work fine.

pavelnikolov · 2018-09-17T12:36:51Z

I was wondering the same...

bsr203 · 2018-09-22T12:38:45Z

@tonyghita @neelance please consider spending little of your time as it been the most requested feature and many claim works well. thank you.

smithaitufe · 2018-09-22T12:56:44Z

IMHO, It seems PRs are not appreciated.

bsr203 · 2018-09-22T13:03:20Z

@smithaitufe @neelance commented here as he don't have the bandwidth for some time. I wish more capable maintainers are identified :-( .

stephenafamo · 2018-09-22T13:33:53Z

Is there no one else maintaining this project? Is it just @neelance ?

neelance · 2018-09-22T14:17:59Z

I am not maintaining this project any more, since I'm currently (unfortunately) not working on any project that uses GraphQL. Therefore I'm a bit detached from the GraphQL ecosystem atm and someone else would probably do a better job at maintaining this project. @tonyghita already volunteered, but we can add another helping hand if someone wants to take the job. ;-)

pavelnikolov · 2018-09-22T14:33:04Z

I would like to volunteer. I have been following the project very carefully and my team has been using it in production for more than 1.5 years. We are interested in subscriptions but also extensions and other GraphQL features. @matiasanaya (the author of this PR) is also from my team and I peer-tested his work long time ago. I'm a little disappointed that there hasn't been much activity lately.
If you accept contributors/maintainers I'd be happy to participate/help.

stephenafamo · 2018-09-22T14:33:17Z

I'll be happy to, although since I've not contributed to this project before, I don't know if I am the best person to do it.

However, I am working heavily with something involving GraphQL, and will be for a while.

pavelnikolov · 2018-09-22T14:36:04Z

Adding this project to the #gfrs list in the gophers Slack might be a good idea. This way it can be trully community managed!
IMHO, This project is awesome! The only problem is that it is not maintained regularly. And there isn't much interaction with potential contributors/volunteers.

stephenafamo · 2018-09-22T14:39:00Z

I agree @pavelnikolov

neelance · 2018-09-22T15:45:53Z

I'd like to have a single primary maintainer for the project. The primary maintainer can add secondary maintainers if desired.

@pavelnikolov Using it for 1.5 years sounds like a good qualification. :)

@tonyghita Would it be okay with you to make @pavelnikolov the primary maintainer?

stephenafamo · 2018-09-23T23:01:09Z

I think this would be great.

adriancuadros · 2018-09-26T17:21:51Z

Awesome Library kudos and thanks to the new maintainers @pavelnikolov and @tonyghita

pavelnikolov · 2018-09-26T21:10:11Z

Thank you for trusting in me! It is an awesome privilege and I will take this responsibility seriously.
I’m going on a quick holiday and when I’m back next Tuesday subscriptions will be my first priority.
@matiasanaya let’s catch up early next week re subscriptions, ws transport and example(s).

romshark · 2018-09-26T23:06:57Z

As I read about websocket transport I'd like to ask this question: will the library remain transport-layer agnostic? Will we be able to use graphql-go with subscriptions support on top of any custom transport layer?

I'm worried because we're using a home-grown websocket abstraction library called webwire together with graph-gophers/graphql-go and we currently implemented subscriptions without GraphQL support just by using webwire server-side signals and regular mutations

pavelnikolov · 2018-09-27T00:28:41Z

@romshark I think that the library has to remain transport agnostic and as minimalistic as possible! The WS transport will be added as a separate repository under graph-gophers.

lindroth · 2018-10-15T14:55:53Z

@matiasanaya does this use one websocket for all the subscriptions that the client has, or is it multiple connections?

I know that with apollo it's possbile to send queries and mutations via websocket as well.

pavelnikolov · 2018-10-15T15:15:50Z

@lindroth the library itself is transport agnostic. The subscriptions updates are sent over Go channel. It's up to the user of the library to decide how to handle it. There is a reference websocket implementation that @matiasanaya worked on and send a PR graph-gophers/graphql-transport-ws#1
I'm reviewing both PRs now.

matiasanaya · 2018-10-15T21:54:44Z

@lindroth if you end up using graph-gophers/graphql-transport-ws (once it's merged), one websocket can be used to handle all subscriptions, queries and mutations. It's based on the apollo implementation. In the end though it's going to be up to the client you use.

tsauvajon · 2018-10-22T15:26:38Z

🎉

hampusohlsson · 2018-11-06T12:07:49Z

Awesome work on this!

one websocket can be used to handle all subscriptions, queries and mutations

@matiasanaya Trying to do exactly what you're suggesting here - use websockets for not just subscriptions but querying as well (to lower the number of http requests from the browser), but the implementation seems to explicitly prevent that.

graphql-go/subscriptions.go

Lines 48 to 50 in 940d2b0

    
           if op.Type != query.Subscription { 
        
           	return sendAndReturnClosed(&Response{Errors: []*qerrors.QueryError{qerrors.Errorf("%s: %s", "subscription unavailable for operation of type", op.Type)}}) 
        
           }

I'm using apollo-link-ws on the browser side. Am I missing something?

pavelnikolov · 2018-11-08T02:32:16Z

@hampusohlsson Thanks for your comment. I haven't used apollo-link-ws but maybe it's worth raising a separate issue.

matiasanaya · 2018-11-08T03:28:59Z

@hampusohlsson I believe you are right and that I've, at some point, sneaked in this limitation. Care to raise a separate issue so we can track the work to support queries and mutations through subscriptions?

hampusohlsson · 2018-11-08T12:22:21Z

No problem! Opened an issue here #286

…ptions Support subscriptions

matiasanaya mentioned this pull request May 5, 2018

Make subscriptions work #132

Closed

pavelnikolov reviewed May 6, 2018

View reviewed changes

subscription_test.go Show resolved Hide resolved

matiasanaya added 3 commits May 12, 2018 07:09

Support subscriptions

177b9a1

Remove stdErrors alias

3add01d

Fix graphql indentation

a2b77e9

matiasanaya force-pushed the feature/subscriptions branch from 08079d8 to a2b77e9 Compare May 11, 2018 21:09

Fix formatJSON() call after upstream change

cb1a8ec

pavelnikolov approved these changes Jun 3, 2018

View reviewed changes

Niennienzz mentioned this pull request Jul 9, 2018

DT4-137 Subscription DealTap/graphql-go#7

Closed

kivutar approved these changes Aug 11, 2018

View reviewed changes

sachaarbonel mentioned this pull request Sep 10, 2018

Support for external PubSub Engine 99designs/gqlgen#337

Closed

matiasanaya mentioned this pull request Oct 15, 2018

Problem running example matiasanaya/go-graphql-subscription-example#6

Closed

pavelnikolov merged commit b174f9e into graph-gophers:master Oct 22, 2018

tinnywang pushed a commit to tinnywang/graphql-go that referenced this pull request Dec 13, 2018

Merge pull request graph-gophers#203 from matiasanaya/feature/subscri…

b2470f2

…ptions Support subscriptions

Support subscriptions #203

Support subscriptions #203

Conversation

matiasanaya commented May 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavelnikolov May 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavelnikolov commented May 6, 2018

pavelnikolov commented May 7, 2018

ghost commented May 22, 2018

tsauvajon commented May 23, 2018

matiasanaya commented Jun 3, 2018

tsauvajon commented Jun 13, 2018 • edited Loading

tsauvajon commented Jun 14, 2018

pavelnikolov commented Jun 14, 2018 • edited Loading

ande8133 commented Jun 18, 2018

tsauvajon commented Jun 18, 2018 • edited Loading

kivutar left a comment

Choose a reason for hiding this comment

stephenafamo commented Sep 17, 2018

pavelnikolov commented Sep 17, 2018

bsr203 commented Sep 22, 2018 • edited Loading

smithaitufe commented Sep 22, 2018

bsr203 commented Sep 22, 2018

stephenafamo commented Sep 22, 2018

neelance commented Sep 22, 2018

pavelnikolov commented Sep 22, 2018

stephenafamo commented Sep 22, 2018

pavelnikolov commented Sep 22, 2018

stephenafamo commented Sep 22, 2018

neelance commented Sep 22, 2018

stephenafamo commented Sep 23, 2018

adriancuadros commented Sep 26, 2018

pavelnikolov commented Sep 26, 2018 • edited Loading

romshark commented Sep 26, 2018

pavelnikolov commented Sep 27, 2018

lindroth commented Oct 15, 2018 • edited Loading

pavelnikolov commented Oct 15, 2018

matiasanaya commented Oct 15, 2018

tsauvajon commented Oct 22, 2018

hampusohlsson commented Nov 6, 2018

pavelnikolov commented Nov 8, 2018

matiasanaya commented Nov 8, 2018

hampusohlsson commented Nov 8, 2018

matiasanaya commented May 5, 2018 •

edited

Loading

pavelnikolov May 6, 2018 •

edited

Loading

tsauvajon commented Jun 13, 2018 •

edited

Loading

pavelnikolov commented Jun 14, 2018 •

edited

Loading

tsauvajon commented Jun 18, 2018 •

edited

Loading

bsr203 commented Sep 22, 2018 •

edited

Loading

pavelnikolov commented Sep 26, 2018 •

edited

Loading

lindroth commented Oct 15, 2018 •

edited

Loading