introduce GraphQLExecutorWithSubscriptions #47

DxCx · 2017-02-18T08:26:39Z

Hey,

I've refactored graphql-subscriptions so the execution logic will be isolated then the SubscriptionManager.
With this executor we will support subscriptions for the new graphql-server-ws package.

This PR also adds support for query/mutation.

TODO:

Update README.md add example.

DxCx · 2017-02-18T08:27:41Z

package.json

@@ -8,7 +8,8 @@
    "url": "/~https://github.com/apollostack/graphql-subscriptions.git"
  },
  "dependencies": {
-    "es6-promise": "^3.2.1"
+    "es6-promise": "^3.2.1",
+    "graphql-server-observable": "file://../graphql-server/packages/graphql-server-observable"


pending merging of apollographql/apollo-server#308

stubailo · 2017-02-22T03:31:09Z

Why does this code need to be inside this package? Can't it just go into your GraphQL-server-ws package?

DxCx · 2017-02-22T08:08:59Z

I can wrap it the other way around and give it another package inside graphql-server,
Howerver:
A. I spoke with @helfer before and we both agreed we dont need to introduce another package but just patch the existing package
B. Performance-wise, you will have more logic running that is redundant

stubailo · 2017-02-22T18:29:24Z

Is the main goal here to make the subscription return an observable instead of the callback style? Or does it also do something else?

stubailo · 2017-02-22T18:33:21Z

After looking at the code I think it could be a really great change, but it would be a lot easier to look at it if there was a description about what it is trying to achieve. I agree it is a good idea to have an execution function that decides whether to run a subscription or a static query, but it wasn't clear at first that this is what the PR is doing.

DxCx · 2017-02-22T19:01:26Z

Sorry @stubailo
Well, yeah it has 2 goals:

Allow engine code execute staticlly or reactive.
Export functionality of the engine alone, so graphql-server-ws will be able to support the exisiting pubsub code.

This way people can migrate easily from running on both http and websocket to run all operations on a single websocket connection.

stubailo · 2017-02-22T23:39:16Z

Is there any reason it's split into two steps like that? Why not:

graphqlExecutorWithSubscriptions({
  pubsub,
  setupFunctions,
  schema,
  ...
}

Right now you have to initialize the executor and then call execute but it could just be one function I think?

DxCx · 2017-02-22T23:55:01Z

But the idea is to be comeptiable with graphql execute engine step.
So the GraphQLWithSubscription should be constructed with what makes it a special engine.
(Which is the pubsub related stuff)
Then it will export a function that accepts the exact same arguments as execute gets (and schema being one of them)

DxCx · 2017-02-22T23:57:24Z

If the main goal was to just introduce obeservable that was probably right, but as ive said i wanted the code to be more reusable in general..

stubailo · 2017-02-23T00:00:13Z

But it still won't be a simple replacement right, since graphql returns a promise and this returns an observable. However, if that's your goal I think it would be best for GraphQLExecutorWithSubscriptions to return a function rather than an object, like so:

const graphql = createGraphQLExecutorWithSubscriptions(...);

const resultObservable = graphql(...);

Then it will be even more interchangeable?

DxCx · 2017-02-23T11:36:59Z

Well, the same interface, but instead of a promise, an observable, so more then one value can be emitted.

Why would returning a factory will be better?
Essentially typescript compiler will convert class to functions anyway, so why is that better?

stubailo · 2017-02-23T18:27:10Z

Because it's more similar to the original graphql API? graphql-js exports a function, so this would be more consistent with how people are used to consuming that API. (In your case I think you're creating a class with one method on it, which is a pretty different API)

DxCx · 2017-02-23T21:36:45Z

But it also exports execute which is just the execute step. Original apollo runQuery runs the execute step, thats the reason i made it symmetrical with execute..

…

On Thu, 23 Feb 2017 at 20:27 Sashko Stubailo ***@***.***> wrote: Because it's more similar to the original graphql API? graphql-js exports a function, so this would be more consistent with how people are used to consuming that API. (In your case I think you're creating a class with one method on it, which is a pretty different API) — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#47 (comment)>, or mute the thread </~https://github.com/notifications/unsubscribe-auth/ABB8WwGvKwP7SpwYBQ6u-8zFAbxXDDIrks5rfc9_gaJpZM4MFC-g> .

stubailo · 2017-02-23T21:39:10Z

OK I think that's fine - I think @Urigo should review.

My last opinion here would be that GraphQLExecutorWithSubscriptions should ideally take an entire SubscriptionManager object as one argument, so that we don't have to document those arguments twice (since it takes basically the same things).

DxCx · 2017-02-23T22:34:32Z

Well, as i said, i can do the exact opposite, the truth is that i started this way - so i can easily revert changes.. But, then i realized that there is alot of logic running for nothing and the right thing is to do just the opposite (extract the minimum logic so both packages can use just it) So eventually, the users still can use their existing pubsub/setupFunctions/schema but constructing different objects..

…

On Thu, 23 Feb 2017 at 22:39 Sashko Stubailo ***@***.***> wrote: OK I think that's fine - I think @Urigo </~https://github.com/Urigo> should review. My last opinion here would be that GraphQLExecutorWithSubscriptions should ideally take an entire SubscriptionManager object as one argument, so that we don't have to document those arguments twice (since it takes basically the same things). — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#47 (comment)>, or mute the thread </~https://github.com/notifications/unsubscribe-auth/ABB8W-pfCZPcV7I_X1uupA9H3b20K0nkks5rffyFgaJpZM4MFC-g> .

…ak legacy BREAKING CHANGE: - removed publish from SubscriptionManager. - tests once assumed publish is async and once sync, looks like there was no default behavior. This commit sets publish to run onNextTick, this way, once unsubscribe is called, you are guaranteed not to get callback called again.

DxCx · 2017-03-29T08:06:39Z

this PR is ready to be merged. 👍

NeoPhi · 2017-04-03T14:29:18Z

src/GraphQLExecutorWithSubscriptions.ts

+    this.pubsub = options.pubsub;
+  }
+
+  public executeReactive(


Feels like this should be using an object to pass in arguments. Likewise for execute below.

The idea is to have the same signature as original's graphql execute..

NeoPhi · 2017-04-03T14:32:34Z

src/pubsub.ts

@@ -36,13 +21,15 @@ export class PubSub implements PubSubEngine {
    }

    public publish(triggerName: string, payload: any): boolean {
-        this.ee.emit(triggerName, payload);
+        process.nextTick(() => this.ee.emit(triggerName, payload));


Comment about why the nextTick is needed.

I dont remember the exact issue, but it is related to how the tests assume data is handled.. i can dig in again

NeoPhi · 2017-04-03T14:36:13Z

src/GraphQLExecutorWithSubscriptions.ts

+        .subscribe({
+          next: (v) => {
+            resolve(v)
+            process.nextTick(() => sub.unsubscribe());


Comment about why nextTick is needed.

Because you want to make sure sub is resolved with a value.

NeoPhi · 2017-04-03T14:39:40Z

src/GraphQLExecutorWithSubscriptions.ts

+            resolve(v)
+            process.nextTick(() => sub.unsubscribe());
+          },
+          error: (e) => reject(e),


Shouldn't we also have to unsubscribe in the error case?

Thats on the observer's duty, it might just unsubscribe or handle the error if it wants too, but we shouldn't unsubscribe for the user

DxCx · 2017-05-15T14:09:00Z

@Urigo you forgot about this PR that addes GraphQLExecutorWithSubscriptions.

Urigo · 2017-05-16T14:15:23Z

@DxCx I have not :) please check out my comment here - apollographql/subscriptions-transport-ws#108 (comment)

DxCx · 2017-05-18T07:35:06Z

well, looks like executor pattern was removed in (apollographql/subscriptions-transport-ws#133) which means there is not reason to keep that PR..

DxCx commented Feb 18, 2017

View reviewed changes

DxCx and others added 9 commits March 27, 2017 17:49

chore(executor): refactor repository into 3 parts, introducing executor

4adba81

fix(executor): operation was not selected properly

ee2e152

chore(executor): fixes and unit-testing

5ab87c1

chore(executor): import only observable

1d53b85

doc(executor): update README.md

5b22a09

chore(executor): fix encapsulation of handleSubscription

08f382b

relative path

2074502

chore(executor): remove dependancy on graphql-server-observable

284a7d6

DxCx force-pushed the executor branch from 7439a9a to 284a7d6 Compare March 29, 2017 06:55

NeoPhi reviewed Apr 3, 2017

View reviewed changes

DxCx closed this May 18, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

introduce GraphQLExecutorWithSubscriptions #47

introduce GraphQLExecutorWithSubscriptions #47

DxCx commented Feb 18, 2017 •

edited

Loading

DxCx Feb 18, 2017

stubailo commented Feb 22, 2017

DxCx commented Feb 22, 2017

stubailo commented Feb 22, 2017

stubailo commented Feb 22, 2017

DxCx commented Feb 22, 2017 •

edited

Loading

stubailo commented Feb 22, 2017

DxCx commented Feb 22, 2017

DxCx commented Feb 22, 2017

stubailo commented Feb 23, 2017

DxCx commented Feb 23, 2017

stubailo commented Feb 23, 2017

DxCx commented Feb 23, 2017 via email

stubailo commented Feb 23, 2017

DxCx commented Feb 23, 2017 via email

DxCx commented Mar 29, 2017

NeoPhi Apr 3, 2017 •

edited

Loading

DxCx Apr 4, 2017

NeoPhi Apr 3, 2017

DxCx Apr 4, 2017

NeoPhi Apr 3, 2017

DxCx Apr 4, 2017

NeoPhi Apr 3, 2017

DxCx Apr 4, 2017

DxCx commented May 15, 2017

Urigo commented May 16, 2017

DxCx commented May 18, 2017

introduce GraphQLExecutorWithSubscriptions #47

introduce GraphQLExecutorWithSubscriptions #47

Conversation

DxCx commented Feb 18, 2017 • edited Loading

Choose a reason for hiding this comment

stubailo commented Feb 22, 2017

DxCx commented Feb 22, 2017

stubailo commented Feb 22, 2017

stubailo commented Feb 22, 2017

DxCx commented Feb 22, 2017 • edited Loading

stubailo commented Feb 22, 2017

DxCx commented Feb 22, 2017

DxCx commented Feb 22, 2017

stubailo commented Feb 23, 2017

DxCx commented Feb 23, 2017

stubailo commented Feb 23, 2017

DxCx commented Feb 23, 2017 via email

stubailo commented Feb 23, 2017

DxCx commented Feb 23, 2017 via email

DxCx commented Mar 29, 2017

NeoPhi Apr 3, 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

DxCx commented May 15, 2017

Urigo commented May 16, 2017

DxCx commented May 18, 2017

DxCx commented Feb 18, 2017 •

edited

Loading

DxCx commented Feb 22, 2017 •

edited

Loading

NeoPhi Apr 3, 2017 •

edited

Loading