Subscriptions support #846

Urigo · 2017-05-09T18:50:29Z

This is a PR to add subscriptions support to the official Javascript reference implementation.

This is based on the new PR to the official GraphQL spec by @robzhu - graphql/graphql-spec#305

This is a joint work of the GraphQL team on Facebook, Apollo and many people from the community, so thanks to everyone who used Subscriptions till today and gave feedback and PRs for the early libraries implementations we've created.

All the existing Apollo libraries for GraphQL Subscriptions that you use today are ready for the new upgrade and would be released once this PR will be merged.
To track those changes in advance check out those PRs:

…bscribe.js is calling existing functions.

leebyron

So excited about this addition!

leebyron · 2017-05-09T19:04:03Z

src/subscription/subscribe.js

+    addPath(undefined, responseName)
+  );
+
+  // TODO: handle the error


This is the biggest open question in my mind. What should happen if an error is encountered not while producing the result for a published payload, but when originally setting up the subscription?

My thinking is that this should probably have a very similar kind of error as if you submitted a subscription request that violated a validation rule.

Hm, do we consider the initial response to be privileged in some way?

I'm not sure if privileged is how I would characterize it, but it's certainly different. There's a difference between failing to create a subscription and an error during the first published payload. Clients attempting to subscribe need to be given the appropriate info to distinguish between the two.

In graphql-subscriptions, in case of an error during the creation of the subscription we returned the error as a subscription error immediately (here, and a test for it).

We didn't see people complaining about this approach, and it looks like that the timing of the error was enough until now for people to understand which type of error occurred.

I've changed it, and now errors thrown by resolveFieldValueOrError will be thrown (and the developer need to fix it, just like the Subscription must return Async Iterable error).

Do you think it makes sense?

leebyron · 2017-05-09T19:12:53Z

src/subscription/__tests__/subscribe-test.js

-
-    const pubsub = new EventEmitter();
-
+  const createSubscription = pubsub => {


minor style detail: functions defined at the top level we use function createSubscription(pubsub) {

leebyron · 2017-05-09T19:14:06Z

src/subscription/__tests__/subscribe-test.js

+        null, // context
+        { priority: 1 }
+      ),
+      sendImportantEmail,


small thing, but moving this to the first key in this return object would make it more obvious that more than one thing are being returned, otherwise it's easy to mistake this as another argument to subscribe

leebyron · 2017-05-09T19:16:22Z

src/subscription/__tests__/subscribe-test.js

+        fields: {
+          importantEmail: {
+            type: GraphQLString,
+            resolve: () => 'test',


I think you mean to test subscribe: () => 'test' here?

Another test to add (which will not have correct behavior just yet) is if a subscription resolver returns an error or throws one: subscribe: () => new Error('test') or subscribe: () => { throw new Error('test'); }

Fixed the first one, and added a test case for the error

leebyron · 2017-05-09T19:19:15Z

src/subscription/__tests__/subscribe-test.js

+        ast,
+        null,
+        null, // context
+        { priority: 1 });


note: all the {priority: 1} aren't being used by the test AST (those are variables), so these tests could all be simplified to subscribe(schema, ast)

leebyron · 2017-05-09T19:19:46Z

src/subscription/__tests__/subscribe-test.js

+    }).not.to.throw();
+  });
+
+  it('throws when subscribe does not return a valid iterator', () => {


Is this the same test as the one on line 404?

OlegIlyenko · 2017-05-09T22:25:18Z

I noticed that all test subscription types have only a single field (importantEmail in this case). I think it would be quite helpful to have at least 2 subscription fields in the tests and ensure that asserts accurately verify null and undefined values.

stubailo · 2017-05-10T02:30:39Z

@OlegIlyenko that's reasonable given that the spec currently only allows one field right?

DxCx · 2017-05-10T05:53:14Z

is there a reason to introduce new subscribe callback?
i mean, you can support AsyncIterator from resolve function, and then you could use subscription resolve to return the right iterator.
also, you can use mapAsyncIterator on this resolve function to modify the original emitted values
and introcude filterAsyncIterator instead of the setupFunctions thingy.
and then subscription will become just like query and mutation, and you can use the same infrastructure in the future for @LiVe.

stubailo · 2017-05-10T06:32:22Z

I think a core part of the design here is that subscription resolvers are not special, and in fact could be called from an entirely separate service. In one production ready design for subscriptions, in particular the one used at Facebook, the GraphQL API service is completely stateless and called once for every subscription event. If resolvers returned an iterator, that architecture wouldn't really be possible.

DxCx · 2017-05-10T06:54:02Z

@stubailo not sure how current approach fixes it,
you still have AsyncIterator, but you will resolve it in one place and then map it's value on "resolver"

OlegIlyenko · 2017-05-10T08:29:19Z

@stubailo I don't mean 2 fields in the query but rather in the schema. E.g.

const SubscriptionType = new GraphQLObjectType({
      name: 'Subscription',
      fields: {
        importantEmail: { type: EmailEventType },
        newContact: { type: Person },
      }
    });

query still can use only one of these fields.

given that the spec currently only allows one field right?

From what I can tell, proposed changes to the spec indeed allow to query only a single field. Though this feature is still in PR stage, so it's subject for a discussion, if I understand it correctly.

taion · 2017-05-10T20:25:17Z

src/subscription/subscribe.js

+      schema,
+      document,
+      payload,
+      contextValue,


Can there be some way to customize this context, possibly by adding some way to preprocess the arguments to execute here? (This could go well with converting these function signatures to take objects, per #356.)

In our current implementation, we use time-limited auth tokens. For our subscription implementation, we allow the client to update the auth token used.

This auth token is bound to the client session (it comes from somewhere separate). Implementation-wise, we currently swap out the context used for resolving subscriptions, which allows us to continue to use an up-to-date auth token without e.g. tearing down and recreating all the subscriptions.

@taion could you offer a suggestion as to how this might impact the actual reference implementation? One of the goals here is to illustrate how subscriptions are a thin layer atop the existing execution operation.

By the way, the pattern you're talking about is often implemented in pure-functional systems using an "atom", which is essentially a mutable reference point. I could imagine that rather than passing the auth token directly as context, you could pass an object which contains a reference to an auth token, that way when you need to update the auth token, you can just mutate the context object without requiring tearing down and recreating subscriptions.

I could hold onto a reference to the context with which I called subscribe, and mutate it, but that feels ugly.

This subscribe function is what I call to set up a subscription, right? The most light-touch way to do this would be to just allow a custom execute callback. That gives a fairly generic way for me to modify the context passed down to execute.

leebyron · 2017-05-11T22:38:45Z

@DxCx - there's an important distinction to be made between the creation of a subscription and the execution of a response per each published event from that subscription. Critically, as @stubailo pointed out, these two steps may occur on different services. In that case, it's important for the execution portion of the subscription to remain separately invokable and not special from the GraphQL executor's point of view.

taion · 2017-05-11T23:00:13Z

How does unsubscribing work if subscribe only returns an async iterable of execution results? It seems like I'd have to build a separate side channel where e.g. I inject some reference when setting up the subscription that I can then use separately to tear down that subscription later on? That feels awkward.

taion · 2017-05-11T23:09:21Z

Thinking about this a little more, I'm not sure this is the best or simplest API possible. I do see and appreciate the elegance of field.subscribe returning an async iterator, and then subscribe() dealing with the execution.

However, I think the API could be significantly simpler and more flexible if control were un-inverted. Instead of field.subscribe returning an async iterator, with subscribe() calling execute(), it would be simpler if field.subscribe returned an opaque value. It would not be much more work for the user code to use this value to set up a subscription (or this value could even be an async iterator), and then for the user code to own calling execute when a subscription event happens.

This sort of API would also allow patterns like centralizing subscription-level auth checks. Additionally, it would make it easier to handle unsubscribing, since the opaque field.subscribe return value could include sufficient information to actually handle unsubscribing, without resorting to side channels.

taion · 2017-05-11T23:10:58Z

Concretely, I'm suggesting that subscribe just return the value of resolveSubscription directly, and that resolveSubscription not be constrained to return an async iterable.

Alternatively, it would also make sense to both expose the lower-level API that I propose, and then build the current API on top of that lower-level API.

taion · 2017-05-11T23:18:04Z

Oops, my bad, didn't notice the .return() handling.

leebyron · 2017-05-12T20:23:41Z

However, I think the API could be significantly simpler and more flexible if control were un-inverted. Instead of field.subscribe returning an async iterator, with subscribe() calling execute(), it would be simpler if field.subscribe returned an opaque value. It would not be much more work for the user code to use this value to set up a subscription (or this value could even be an async iterator), and then for the user code to own calling execute when a subscription event happens.

Alternatively, it would also make sense to both expose the lower-level API that I propose, and then build the current API on top of that lower-level API.

This idea is reasonable and easy to add - we simply need to move the first two steps of subscribe() (creating an execution context and resolving the subscription event source) into it's own function, and then also export that function. I think that actually better mirrors the algorithms the spec is attempting to define. The "Subscribe" algo is extremely simple: First produce an event stream, Second execute GraphQL for each event. In practice for "real" systems, this algorithm would live across services - one service responsible for stateful connection maintenance, one service responsible for stateless GraphQL execution. The hope is that this reference implementation can be used both for single-process small installations, but also for these multi-process "real" systems. Your suggestion helps get us closer to that goal.

leebyron · 2017-05-12T20:24:48Z

I would suggest createSubscriptionEventSource as a name for that new function. Thoughts?

taion · 2017-05-12T20:35:01Z

I would suggest calling it something like getX or resolveX. The current implementation I use for subscriptions essentially follows this pattern. The subscribe methods on my subscriptions only returns the name of a Redis topic. They don't "create" anything, beyond formatting strings.

I'd argue that in this pattern, it's possible and perhaps even desirable for the subscribe methods to be side-effect-free, so naming like get or resolve might better communicate what's happening.

leebyron · 2017-05-12T21:19:12Z

Good feedback. "resolve" already has a meaning here as a more specific function that is part of what we're talking about - resolving a subscription event source refers to getting the event stream (asynciterable) for a specific subscription field - it's input is a Field. This function does that while also handling some other minor details like creating an execution context and checking some invariants and finding the relevant Field - it's input is a Document.

Right now, there isn't a substantial difference between the two since we're starting with limiting subscriptions to include only one top level field - so there is a 1:1 relationship at play. However if we decide in the future to allow multiple top level fields, then this top level function would be responsible for merging event streams.

I suggested create from a mental model that an event stream is created upon request (typically true for AsyncIterators, but perhaps not for AsyncIterables) - I'm okay with get prefix.

…ion fields and multiple schema subscriptions

Urigo · 2017-05-14T13:59:14Z

@OlegIlyenko I added test cases for multiple subscriptions defined in the schema (and query for one of them), and multiple subscriptions defined in schema and query for two of them (throws error).

In case of null and undefined returned from the subscriptions resolver - an error will be thrown (source) because the only allowed return value is AsyncIterable.

Do you still think we need to add tests for specific types? we have tests that checks the return value of subscribe (here)

Urigo · 2017-05-14T14:18:57Z

@taion @leebyron
I understand and see the use case.
As suggested, the subscribe method is now splitted into two methods (called getSubscriptionEventSource) and exported from the package.

taion · 2017-05-14T20:37:00Z

I was imagining that getSubscriptionEventSource would return some sort of opaque subscription handle, instead of an async iterable – so I could potentially set up and tear down the subscriptions at the server level. That does mess up the signature of subscribe for static typing, though, so I'm not sure it's better.

leebyron · 2017-05-14T22:15:20Z

@taion can you clarify what you mean by:

would return some sort of opaque subscription handle, instead of an async iterable – so I could potentially set up and tear down the subscriptions at the server level.

Wouldn't you want to use an opaque wrapper to remove the ability to operate on the response rather than adding new capabilities? Perhaps I don't fully understand the use case you have in mind.

leebyron

So close!!

leebyron · 2017-05-14T22:46:39Z

src/subscription/mapAsyncIterator.js

+): AsyncIterator<U> {
+  // Fixes a temporary issue with Regenerator/Babel
+  // https://github.com/facebook/regenerator/pull/290
+  const iterator = iterable.next ? (iterable: any) : getAsyncIterator(iterable);


This issue was fixed! Pretty sure this can now be safely replaced with const iterator = getAsyncIterator(iterable);

facebook/regenerator#290 was merged but not included in a release yet. maybe we make release happen? (for regenerator-transform and babel-plugin-transform-regenerator).

cc @benjamn

I believe regenerator-transform and babel-plugin-transform-regenerator automatically include the latest point releases of regenerator, but explicitly bumping their minimum required version would be a good idea to help close the gap on this bug

leebyron · 2017-05-14T22:52:22Z

src/subscription/subscribe.js

+  );
+
+  // TODO: make GraphQLSubscription flow type special to support defining these?
+  const resolveFn = (fieldDef: any).subscribe || defaultFieldResolver;


To fix this (and remove the any cast) I think you can just add a subscribe entry that mirrors resolve to GraphQLField and GraphQLFieldConfig in type/definition.js. Should probably also add a similar use of isValidResolver (L527).

I've added subscribe to the Flow types.
Are you sure that isValidResolver can help us here? because isValidResolver allows null or function as resolvers, but in this case we want to enforce a resolver (and disallow null value).
In our case we want to make sure that subscribe resolver exists and returns a valid AsyncIterable, otherwise we can't resolve the subscription.

taion · 2017-05-15T00:26:06Z

I take back what I said – my current API design is a bit silly, in that I have my subscribe resolvers return Redis topics. It would be cleaner for them to return async iterables.

That said, is there some valid use case where the service processing subscription requests is not the service dispatching subscription updates? Maybe some sort of persistent subscriptions, like for push notifications? Or is that best handled through a different mechanism than this?

leebyron · 2017-05-15T17:52:38Z

That said, is there some valid use case where the service processing subscription requests is not the service dispatching subscription updates? Maybe some sort of persistent subscriptions, like for push notifications? Or is that best handled through a different mechanism than this?

This is absolutely a valid use case. However if you're spreading work across services like this, then you're probably not using AsyncIterators to get the job done. That's totally fine.

taion · 2017-05-15T17:54:17Z

@leebyron In which case, resolveSubscription should not check isAsyncIterable when called via getSubscriptionEventSource?

leebyron · 2017-05-15T18:36:04Z

Actually, in which case resolveSubscription() wouldn't be used either - in a scenario where you're spreading the subscribe, publish, and execute steps across multiple services then you're definitely not using graphql.js's built in subscriptions implementation.

taion · 2017-05-15T18:59:06Z

It'd be nice to have some way to run the subscription's subscribe method purely for e.g. the side effects or whatever, though, no?

Urigo · 2017-05-16T12:43:09Z

@taion
If you need to map Redis topic subscription to GraphQL subscription, you can map it using a PubSub implementation that subscribe to Redis topics.

There is an implementation for Redis with GraphQL subscription, but it need some changes after merging this PR to support AsyncIterable API.

Note that you can always wrap every AsyncIterator and add your own custom logic (you can find a filter implementation here), it wraps specific event from PubSub instance and filter the published payload. It also allows you to map multiple PubSub events into a single AsyncIterator.

robzhu

I just went over this PR with latest spec PR side-by-side. See inline comments.

robzhu · 2017-05-16T15:51:05Z

src/subscription/__tests__/subscribe-test.js

+        ast,
+        null,
+        null);
+    }).to.throw('A subscription must contain exactly one field.');


Maybe word this as "A subscription operation must contain exactly one root field"?

robzhu · 2017-05-16T16:07:54Z

src/subscription/__tests__/subscribe-test.js

+        null);
+    }).to.throw('test error');
+  });
+});


Can we add another test case with two subscriptions on importantEmail and verify that they both receive the payload?

robzhu · 2017-05-16T16:15:30Z

src/subscription/subscribe.js

+  OperationDefinitionNode,
+} from '../language/ast';
+
+export function getSubscriptionEventSource(


Nit: can we rename this to "createSourceEventStream" to be consistent with the spec language?

Agree except it should probably clarify that it is part of the subscription API in the name, yeah? How about createSubscriptionSourceEventStream? Long but more accurate. We could change the name in the spec too if matching is desirable

I slightly prefer "CreateSourceEventStream". In the spec, "subscription" is fairly obvious based on context.

Changed to createSubscriptionSourceEventStream, is that ok?

Let's go with "CreateSourceEventStream"

robzhu · 2017-05-16T16:17:20Z

src/subscription/subscribe.js

+    addPath(undefined, responseName)
+  );
+
+  const subscription = resolveFieldValueOrError(


Might be useful to add a comment that this mirrors the "ResolveFieldEventStream" step in the spec algo: https://github.com/robzhu/graphql/blob/master/spec/Section%206%20--%20Execution.md

Do you want me to add this comment along with other comments to mirror the spec? or only this specific one?

Yeah, I think comments are useful wherever the function names diverge from the spec or where the implementation doesn't match the spec algo step-by-step.

leebyron · 2017-05-17T16:37:03Z

src/execution/values.js

@@ -191,7 +191,7 @@ function coerceValue(type: GraphQLInputType, value: mixed): mixed {
    const itemType = type.ofType;
    if (isCollection(_value)) {
      const coercedValues = [];
-      const valueIter = createIterator(_value);
+      const valueIter = createIterator((_value: any));


Could you remove this line and rebase this PR from master branch again?

Some of the less-related parts of this PR have been already merged into master

# Conflicts: # package.json

… subscription

leebyron · 2017-05-17T22:15:30Z

I agree that we should follow this up by adding in some additional comments. It's always nice to reference the spec from the doc blocks above functions.

Similarly, createSubscriptionSourceEventStream is exported but doesn't have a doc block. It should include a description of what its for and how to use it. And specifically call out the use case of building a cross-service subscriptions installation.

DxCx · 2017-05-17T22:16:08Z

woohoo! 🎉

Urigo · 2017-05-18T08:05:45Z

@leebyron followed up your comment on this PR

DxCx · 2017-05-18T08:21:13Z

@leebyron can you give an example of how cross-service subscriptions should work?
i can see couple of ways and not sure which one is the intended one..

@stubailo

This adds an argument to `execute()`, `createSourceEventStream()` and `subscribe()` which allows for providing a custom field resolver. For subscriptions, this allows for externalizing the resolving of event streams to a separate function (mentioned in #846) or to provide a custom function for externalizing the resolving of values (mentioned in #733) cc @stubailo @helfer

@stubailo

This adds an argument to `execute()`, `createSourceEventStream()` and `subscribe()` which allows for providing a custom field resolver. For subscriptions, this allows for externalizing the resolving of event streams to a separate function (mentioned in #846) or to provide a custom function for externalizing the resolving of values (mentioned in #733) cc @stubailo @helfer

@stubailo

This adds an argument to `execute()`, `createSourceEventStream()` and `subscribe()` which allows for providing a custom field resolver. For subscriptions, this allows for externalizing the resolving of event streams to a separate function (mentioned in #846) or to provide a custom function for externalizing the resolving of values (mentioned in #733) cc @stubailo @helfer

leebyron and others added 4 commits May 9, 2017 20:57

Rough take on subscriptions

18826e1

Minor refactoring of execute.js to make it more clear that most of su…

b8a3c74

…bscribe.js is calling existing functions.

feat(subscriptions): expose subscribe in the bundle

74a7dc6

feat(subscriptions): added unit tests

1242665

facebook-github-bot added the CLA Signed label May 9, 2017

This was referenced May 9, 2017

WIP: Support for new graphql-js subscriptions support apollographql/graphql-subscriptions#60

Merged

WIP: Support for new graphql-js subscriptions support apollographql/subscriptions-transport-ws#122

Closed

leebyron reviewed May 9, 2017

View reviewed changes

taion reviewed May 10, 2017

View reviewed changes

leebyron mentioned this pull request May 13, 2017

Spec: Formal Subscriptions Definition graphql/graphql-spec#305

Merged

Urigo added 2 commits May 14, 2017 16:18

feat(subscriptions): added error handling and fixed tests

0dd3ce3

feat(subscriptions): added test cases for querying multiple subscript…

ba0f60e

…ion fields and multiple schema subscriptions

feat(subscriptions): split subscribe function

e989ae5

feat(subscriptions): export getSubscriptionEventSource

0d8380f

leebyron reviewed May 14, 2017

View reviewed changes

feat(subscriptions): added subscribe to field flow type

e3f695c

robzhu reviewed May 16, 2017

View reviewed changes

leebyron reviewed May 17, 2017

View reviewed changes

Urigo added 3 commits May 17, 2017 22:32

Merge branch 'master' into subscriptions

a76e7a3

# Conflicts: # package.json

feat(subscriptions): minor fixes

54ca988

feat(subscriptions): added test for multiple subscribers for the same…

5f25f53

… subscription

leebyron merged commit a13abf7 into graphql:master May 17, 2017

Urigo mentioned this pull request May 18, 2017

feat(subscriptions): added code comments #858

Merged

leebyron mentioned this pull request May 18, 2017

Allow providing custom default field resolver. #865

Merged

IvanGoncharov mentioned this pull request May 23, 2018

[RFC] Executor Semantics for Subscriptions #224

Closed

rrosenzweig mentioned this pull request Aug 3, 2018

Subscription support andreas/ocaml-graphql-server#101

Merged

5 tasks


		const pubsub = new EventEmitter();

		const createSubscription = pubsub => {

Subscriptions support #846

Subscriptions support #846

Conversation

Urigo commented May 9, 2017

leebyron left a 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

Urigo May 14, 2017 • 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

OlegIlyenko commented May 9, 2017 • edited Loading

stubailo commented May 10, 2017

DxCx commented May 10, 2017

stubailo commented May 10, 2017

DxCx commented May 10, 2017

OlegIlyenko commented May 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leebyron commented May 11, 2017

taion commented May 11, 2017

taion commented May 11, 2017

taion commented May 11, 2017

taion commented May 11, 2017

leebyron commented May 12, 2017

leebyron commented May 12, 2017

taion commented May 12, 2017

leebyron commented May 12, 2017

Urigo commented May 14, 2017

Urigo commented May 14, 2017 • edited Loading

taion commented May 14, 2017

leebyron commented May 14, 2017

leebyron left a 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

taion commented May 15, 2017

leebyron commented May 15, 2017

taion commented May 15, 2017

leebyron commented May 15, 2017

taion commented May 15, 2017

Urigo commented May 16, 2017

robzhu left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leebyron commented May 17, 2017

DxCx commented May 17, 2017

Urigo commented May 18, 2017

DxCx commented May 18, 2017 • edited Loading

Urigo May 14, 2017 •

edited

Loading

OlegIlyenko commented May 9, 2017 •

edited

Loading

OlegIlyenko commented May 10, 2017 •

edited

Loading

Urigo commented May 14, 2017 •

edited

Loading

DxCx commented May 18, 2017 •

edited

Loading