-
Notifications
You must be signed in to change notification settings - Fork 2k
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
[RFC] Initial pass at adding subscription
to executor
#189
Conversation
Here's an issue in Relay for adding an API for user-land mutations facebook/relay#411 |
CC @laneyk Yep, this all matches my thoughts almost exactly; graphql-js probably doesn't have much to say about subscriptions, it just knows how to execute them, and a lot of the complexity lives in the userland.
Yep, we do the submission over MQTT; we have the client submit a
Yep, it's quite similar. Overall, this looks like the right approach to me. CC @leebyron, @schrockn for their thoughts. |
The proposed solution and a discussion in a slack channel inspired me to make a write-up about my thought on a GraphQL subscription mechanism. It turned out to be a bit bigger than I expected, so I put it in a gist: https://gist.github.com/OlegIlyenko/a5a9ab1b000ba0b5b1ad I think it's a bit different from the solution described in this issue, but I hope it would be helpful for a final decision. |
@OlegIlyenko That write-up is fantastic! I think "subscription field results as well as their meaning are completely user-defined" is key there; it means we can probably introduce the core functionality needed to build subscriptions as a pretty minimal set of changes to It seems like this PR is basically the minimal set of changes needed to allow graphql-js to do anything with subscriptions. The open question seems to be how execution works; does the executor for a subscription expect |
@dschafer I don't think they're quite symmetric, though. One advantage of the " |
@dschafer thanks for looking into it and describing the approach you are using at FB! I agree, it's very important to keep semantics a small as possible. Putting too much constraints, or explicitly supporting particular patterns in subscriptions can drastically reduce it's usefulness for many people. I guess what I described in this gist was more an example of subscription use-case in server-to-server communication. I really excited to explore this area, I think it has a lot of potential. The approach, that you have described (where observable sits atop of the executor), is definitely a viable solution. One thing concerns me though. It has an assumption of having only one type/kind of observable. One way to look at GraphQL is to treat it as an integration point for different backend services. This means that As far as I understand, in case of observable sitting atop of the executor, every field of
one can write subscription query like this:
Now I actually get a feeling that what we are discussing here is more about the execution mechanics/implementation. I wonder how this will affect the actual specification and whether it can be possible to describe subscription semantics in a way that leaves room for both of these implementation approaches. Maybe it would be a good idea to explore this in graphql/graphql-spec#86? :) |
Consider how we might want to define a subscription in var GraphQLRenameTodoMutation = mutationWithClientMutationId({
name: 'RenameTodo',
inputFields: {
id: { type: new GraphQLNonNull(GraphQLID) },
text: { type: new GraphQLNonNull(GraphQLString) },
},
outputFields: {
todo: {
type: GraphQLTodo,
resolve: ({localTodoId}) => getTodo(localTodoId),
}
},
mutateAndGetPayload: ({id, text}) => {
var localTodoId = fromGlobalId(id).id;
renameTodo(localTodoId, text);
return {localTodoId};
},
}); What I would like to have would be something like var GraphQLRenameTodoSubscription = subscriptionWithClientId({
name: 'RenameTodo',
outputFields: {
todo: {
type: GraphQLTodo,
resolve: ({localTodoId}) => getTodo(localTodoId),
}
},
subscribeAndGetPayload: (callback, {id}) => {
var localTodoId = fromGlobalId(id).id;
emitter.on(`renameTodo:${localTodoId}`, () => callback({localTodoId});
// Or return an RxJS observable.
},
}); The symmetry here is that |
} | ||
|
||
var schema = new GraphQLSchema(schemaBody); |
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.
you could combine this line with below, returning directly.
This is looking great - I agree that this is the right step forward for enabling experimentation around subscriptions. Could you add some test cases, perhaps within some of the validation rules, to ensure these additions you made are correct? |
Awesome. Yah I'll put that together tonight or tomorrow. |
This is looking great @skevy |
Guess I better get my butt in gear on this given this blog post: http://graphql.org/blog/subscriptions-in-graphql-and-relay/ :) |
Random aside...the hardest thing about writing tests for this is keeping the line lengths for the test descriptions under 80 characters :-p |
Updated tests @leebyron. One note - since subscriptions, from an executor perspective, execute the same as queries, I didn't create a separate |
[RFC] Initial pass at adding `subscription` to executor
This is really solid. Thanks for your hard work and for your patience for my review! |
Awesome stuff |
One question. How do you unsubscribe? or close the subscription. |
Would it be wise to adopt/model the MQTT spec for this? https://www.npmjs.com/package/mqtt#mqttclientsubscribetopictopic-arraytopic-object-options-callback |
Or maybe http/2? http://timkellogg.me/blog/2015/02/20/can-http2-replace-mqtt/ |
Hi, Some input and feedback from my side: I totally agree that GraphQL should neither prescribe
The event based solution which is described in @OlegIlyenko's further examples (and also similary in this post: http://graphql.org/blog/subscriptions-in-graphql-and-relay/ ) most makes a lot of sense if you already have organized your backend in an event processing form. If not (you only store the latest) it might not be the best way for you:
This means if you want to get reliable updates in that fashion you also need store some history of events and provide that on demand to the client (like @OlegIlyenko showed with Live queries make that easier, but as the FB blog post discusses, they might be hard to implement. I think what is needed for all scenarios is a way to get some subscription context information inside the resolve function:
In the beginning I thought that probably every field could return an observable for subscriptions, but I don't really know how this would work out with subscriptions. The interesting thing about GraphQL is that we have nested queries, and therefore even while a client is subscribed to the same query internal subscriptions might need to be cancelled and recreated. Another options that I thought about was that the resolve function could pass some kind of On how to get subscription data from the server to the client: Although not necessarily needed it might make sense to standardize some of those mechanisms so that GraphQL server implementations are compatible - and compatible with tooling like GraphiQL). |
In the meantime I came to the conclusion that internal subscriptions (the things that need to be monitored in order to provide updates to a client) can not be valid for longer than 1 update in the general case with GraphQL. As an example lets look at the following subscription query:
In order to provide live updates for this subscription we would internally need to have a subscription on the list of players on the server. Additionally we need to to have a subscription on the list of friends for the top player. As soon as one of those internal dependencies changes we need to rerun the query in order to deliver an update to the client. During this update our internal subscriptions would be partly invalidated and must be updated -> We still need to track the list of players, but we might night to track another players friends list. Depending on how you model your interface and data updating internal subscriptions is not necessary, but for the general case it seems to be. This means for me that returning observables from the first query execution and monitoring them for delivering subscriptions would probably not work out too good. It also means that @arunoda & Co might need to check how their invalidation server approach for reactive GraphQL covers this scenarios. I guess it won't if the dependencies are only sent during the initial subscription and not updated along with the individual subscription results? |
So, in talking to @taion and @KyleAMathews about subscriptions in the GraphQL Slack chatroom, I felt like I should pass my work on this along.
TLDR; - there's a lot of work to be done on supporting subscriptions in the Relay/GraphQL ecosystem, and lots to figure out. This RFC doesn't do much, except allow "subscription" operations to be executed through the existing executor. Also, I haven't fully tested this. Tests PASS (or they did when I wrote it), but if we go forward with something like this, I will of course update code as needed.
Longer version:
In thinking about subscriptions in GraphQL land, and looking at what GraphQL (specifically graphql-js) does now...it seems like graphql-js really doesn't have to prescribe much about subscriptions. It seems like all that really needs to happen is that the server UNDERSTANDS subscription operations, and is able to execute them just like normal queries. The implementor of the GraphQL resolver would take the extra step to actually cause the side effect of the subscription query being stored, so that it can be executed later. It wouldn't make sense to have this functionality be part of GraphQL, because GraphQL is server agnostic.
My perceived flow of how subscriptions would work in GraphQL/Relay land would be this:
subscription
operation to the GraphQL backend.subscriptionWithClientId
method, such as themutationWithClientMutationId
in graphql-relay-js (this would be out of scope of graphql-js).subscriptionId
that the client could use to listen for updates. You could either store the whole query (as is discussed here: [Feature request] Returning fieldASTs to be able to cache them for performance imporvements #158 (comment)) or store just the ASTs. Again this would be up to the implementor to build in user-land.In this whole process, I feel like the only thing graphql-js needs to care about is actually executing the subscription queries, which is what this PR addresses.
I'm curious as to everyones thoughts on all of this. It seems like, in talking to @taion and @KyleAMathews, and of things that I've heard from @dschafer, that I'm on the right track. But regardless of whether I am going down the right implementation path or not, I wanted to open this PR so that there could be a discussion that didn't occur over Slack, since it loses history constantly. :)