WIP: Graphql Transport WS Protocol #108

mistic · 2017-04-13T19:43:31Z

This PR is still a work in progress (with the remain tasks needed to be done described below), that has been done close to @DxCx supervision, but it has already implemented the majority of needed changes to the new package graphql-trasnport-ws according our point 2 on the shared design document ( GraphQL WebSocket transport plan ). So in this PR we have implemented the new protocol in order to support queries, mutations and subscriptions both on server and on the client side (all the code tasks from point 2 of the document are done).

Please note this is an intermediate pull request to allow you guys start reviewing the most important changes. However a final pull request will be opened from a branch called graphql-transport-ws containing the current code of this branch (graphql-transport-ws-protocol) plus the changes you request in this review, unit tests and those following changes:

internal code library split into 3 new packages: common, client and server.
unit tests split into client and server with a common config.

PS: @DxCx thank you so much for your help, support and continuous reviews during the all process!

TODO:

Fix all the old tests.
Code tests to cover all the new logic and mapping functions for legacy features.
Add env var to change between hybrid/full ws transport, persistedQueries support, max query size limitation, improve code, and write documentation update for full-transport-ws GitHunt repos.
Update documentation and also including examples for full and hybrid ws transport on it.
Update CHANGELOG.md with changes made.

…s both on client and server side using websockets as transport layer to be used on a new package called graphql-transport-ws. fix(NA): fixed path of new files.

apollo-cla · 2017-04-13T19:43:32Z

@mistic: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/

Urigo · 2017-04-18T12:13:53Z

@mistic great job! I love your PR and it helps us move a great step forward.

The direction is exactly as discussed on the design doc and you can move forward.

Here are some comments from reviewing the current state:

I think we should change GraphQLTransportWSServer to expose static method for creation without constructor because the instance is useless
We still want to continue supporting sending queries and mutation over HTTP and subscriptions over Websocket, but now you removed the addGraphQLSubscriptions function. we should keep that functionality, but not here. it is related to Apollo Client network interface and this package is not tied to Apollo. so please create it as a separate repo and npm package for that and I will add it under the Apollo organization when ready.
I've tried to run Githunt with your fork but failed with the following error:

Please also include an example of Githunt running with the package both with complete websockets and hybrid with HTTP and Websocket side by side.
backward compatibility Currently the code is not backward compatible because the name changed from SubscriptionServer to GraphQLTransportWSServer and also GraphQLTransportWSClient but, because this would be released as a new npm package, maybe instead of keeping the code back-compack, we can just remove the code and keep the current version on a branch we can support and keep releasing patches from

Great job!

DxCx · 2017-04-19T07:19:34Z

regarding backwards compatibility, we should expose the new names by the old names with deprecation warning.

mistic · 2017-04-20T21:16:41Z

@DxCx I've changed the names both on server and on the client back for the old ones as we discussed.

@Urigo Can u review my updates to see if they matches your expectations?

This is an example of how to do hybrid mode (https + ws) or full ws mode with that implementation:

import {SubscriptionClient, addGraphQLSubscriptions} from 'subscriptions-transport-ws';
import ApolloClient, {createNetworkInterface} from 'apollo-client';

// Create WebSocket client
const wsClientOptions = {
	reconnect: true,
	connectionParams: {
    // Pass any arguments you want for initialization
	}
};
const wsNetworkInterface = new SubscriptionClient('ws://localhost:3000/graphql', wsClientOptions);

// Create regular NetworkInterface by using apollo-client's API:
const httpNetworkInterface = createNetworkInterface({
 uri: '/graphql' // Your GraphQL endpoint
});

// Create Hybrid interface
const hybridInterface = addGraphQLSubbscriptions(httpNetworkInterface, wsNetworkInterface)

// Finally, create your ApolloClient instance with the modified network interface
// Full websocket: wsNetworkInterface
// Mixed Mode: hybridInterface
const apolloClient = new ApolloClient({
    networkInterface: hybridInterface
});

PS: I pointed both Githunt-API and Githunt-react for this code and all the old behaviour with subscriptions over ws and queries and mutations over http is working :D At the moment I'm bot able to make a test with GitHunt with full ws transport because it requires some more work.

DxCx · 2017-04-21T06:04:12Z

src/client.ts

@@ -159,11 +161,11 @@ export class GraphQLTransportWSClient {
    }

    if (
-      !isString(query) ||
+      ( !isString(query) && !getOperationAST(query, operationName)) ||


query instanceof DocumentNode is more cleaner.. 👍

It cannot be done that way as we talked :(

DxCx · 2017-04-21T06:06:05Z

src/helpers.ts

    },
    unsubscribe(id: number): void {
      wsClient.unsubscribe(id);
    },
  });
 }
+
+export function addGraphQLWS(networkInterface: any, wsClientOptions: ClientOptions): any {


hmm, i might be confused, but why do we need that?

DxCx · 2017-04-21T06:06:44Z

src/helpers.ts

    },
    unsubscribe(id: number): void {
      wsClient.unsubscribe(id);
    },
  });
 }
+
+export function addGraphQLWS(networkInterface: any, wsClientOptions: ClientOptions): any {
+  if (typeof networkInterface._uri !== 'string') {


you are checking if network interface has implementation related uri.. :\

mistic · 2017-04-22T14:55:58Z

@DxCx @Urigo I've updated my last comment to reflect how we should use this PR to implement the hybrid mode and the full ws mode. Is this according your expectations?

feat(NA): applied changes according pull request review.. feat(NA): added static method for instance creation. refact(NA): change the class names back to subscriptionserver and subscriptionclient. feat(NA): applied changes according pull request review. feat(NA): added static method for instance creation. fix(NA): fixed error thrown when we try to print and query its already a string. refact(NA): change the class names back to subscriptionserver and subscriptionclient. feat(NA): added new helper function to fully extend network interface with ws transport. fix(NA): built legacy messages on server side and other bad object accesses. feat(NA): enclose wsclient creation inside addGraphQLWS function. fix(NA): removed useless addGraphQLWS function. feat(NA): added legacy support to the old server protocol.

DxCx · 2017-04-29T14:30:09Z

src/server.ts

-    });
+  private sendError(connectionContext: ConnectionContext, reqId: string, errorPayload: any,
+                           overrideDefaultErrorType?: string): void {
+    if ([


BUG: overrideDefaultErrorType might be undefined.

DxCx · 2017-04-29T15:11:09Z

src/client.ts

+        const requestPayloadErrors = parsedMessage.payload.errors ?
+          this.formatErrors(parsedMessage.payload.errors) : null;
+
+        this.requests[reqId].handler(requestPayloadErrors, requestPayloadData);


bug: Should be this.requests[reqId].handler(null, parsedMessage.payload);

DxCx · 2017-04-29T15:12:58Z

src/client.ts

-      }
-    }, this.subscriptionTimeout);
-    return subId;
+  public subscribe(options: RequestOptions, handler: (error: Error[], result?: any) => void) {


here we need to add a wrapper handler for converting payload to:

const requestPayloadData = parsedMessage.payload.data || null; const requestPayloadErrors = parsedMessage.payload.errors ? this.formatErrors(parsedMessage.payload.errors) : null;

to keep backward comptiable with subscription manager

mistic · 2017-05-02T13:33:02Z

@DxCx @Urigo just merged the changes to support executor pattern. I believe at this point I can start writing the unit tests. What do u think guys? There are anything more to do on the features side?

feat(NA): added subscription function request. fix(NA): fixed unsubscribe function. feat(NA): added constructor verification for executor and schema. chore(NA): added correct comment inside on message function. feat(NA): added initial filtering for requests. feat(NA): added initial filtering for requests. fix(NA): changed way to store unsubscribe function. fix(NA): removed contextValue from serverOptions. fix(NA): correct warn comment when using legacy subscription manager, chore(NA): Implement Adapter pattern for executor refact(NA): refactor on ExecuteAdapters. feat(NA): added code to support formatError and formatResponse. fix(NA): fix way we invoke format functions. fix(NA): small changes on error creation and executor/subscriptionManager check on loadExecutor. fix(NA): fix bad usage of base params on execute. feat(NA): updated ws dependency to the last version. fix(NA): overrideDefaultErrorType might be undefined. fix(NA): support for legacy handler. fix(NA): fixed const and payload errors.

Urigo · 2017-05-02T18:28:33Z

@mistic let's go ahead and finish the tests.
also if you could link to all the relevant Githunt examples we talked about (and add them to your checklist on the opening comment).
Also, please rebase to master since we merged this PR - #90

mistic · 2017-05-02T19:00:36Z

@Urigo I'll already merge and solve the conflicts between this branch and last changes on master :D
Regarding to GitHunt can u check my personal messages that I sent you?

I already finished the GitHunt example integration with the full ws transport. You can check it here:

https://github.com/mistic/GitHunt-API/tree/full-transport-ws
https://github.com/mistic/GitHunt-React/tree/full-ws-transport

However in order to be able to make a PR to GitHunt I need to complete some minor details (like a toggle between hybrid and full ws transport on GitHunt). Let's discuss this.

helfer

@Urigo asked me to review 🙂

Great job so far @mistic! I had a lot of comments, but they are almost all minor (but still important to me, haha).

The biggest thing is probably that since this will go into a different package, and not subscriptions-transport-ws, I don't think we should keep any backwards compatibility. The backwards compatibility can be kept entirely in subscriptions-transport-ws, which will be a deprecated layer on graphql-transport-ws.

Once we merge this PR, we should keep a branch separate from master for subscriptions-transport-ws and rename this project to graphql-transport-ws (to keep the stars, links etc.).

I only had time to review the client, but I'll try to review the server before the weekend as well.

Really great work @mistic and @DxCx !!! 💪

helfer · 2017-05-04T02:37:11Z

src/client.ts

 import isString = require('lodash.isstring');
 import isObject = require('lodash.isobject');
+import { ExecutionResult, print, getOperationAST } from 'graphql';


I think importing like this will create a huge bundle. We should check the bundle size with a script like we do for react-apollo and apollo-client.

helfer · 2017-05-04T02:40:37Z

src/client.ts

-    if (!query) {
-      throw new Error('Must provide `query` to subscribe.');
-    }
+  public query(options: RequestOptions): Promise<ExecutionResult> {


Is this function purely for convenience? What if someone submits a live query to it? Or if the document is a subscription? I think the transport should be absolutely minimal and just have an "executeRequest" on it. Everything else can be handled by the caller, which may be a network interface.

well @helfer at the moment we will be compatible with current apollo,
however, it is built in a way that when you will go forward to use 1 method that returns an Observable, the changes here are minimal.

Ok, I think if @Urigo prefers it this way, it's fine 🙂 We can rewrite it later.

So lets keep this one as is

helfer · 2017-05-04T02:41:54Z

src/client.ts

-      throw new Error('Incorrect option types to subscribe. `subscription` must be a string,' +
-      '`operationName` must be a string, and `variables` must be an object.');
-    }
+  public subscribe(options: RequestOptions, handler: (error: Error[], result?: any) => void) {


Same comment here as for query. I don't think we need legacy compatibility if we're building a completely new transport. We can provide that legacy compat by making the next update of the old packages wrap the new transport. Or am I missing something?

No need to change it right now, it's ok to keep it for backcompat.

As discussed let's keep things related with backwards compatibility for now.

helfer · 2017-05-04T02:43:27Z

src/client.ts

@@ -165,95 +149,151 @@ export class SubscriptionClient {
  }

  public unsubscribe(id: number) {


If we're supporting all kinds of operations, including live queries, then all the operations can be stopped. In that case unsubscribe isn't the right word, so we should use stop or something (for the new transport, not in this package, of course).

No need to change it right now, it's ok to keep it for backcompat.

As discussed let's keep things related with backwards compatibility for now.

helfer · 2017-05-04T02:43:54Z

src/client.ts

+    if (this.requests[id]) {
+      delete this.requests[id];
+    }
+    this.sendMessage(id, MessageTypes.GQL_END, undefined);
  }

  public unsubscribeAll() {


Maybe stopAll instead?

No need to change it right now, it's ok to keep it for backcompat.

As discussed let's keep things related with backwards compatibility for now.

helfer · 2017-05-04T03:16:50Z

src/client.ts

+
+    // Complete every request that are not registered on the client but are being sent by the server and are not
+    // in the allowed list
+    if (


This is a bit hard to read. I think if the code was easier to understand, the comment wouldn't be needed.

helfer · 2017-05-04T03:18:30Z

src/helpers.ts

 export function addGraphQLSubscriptions(networkInterface: any, wsClient: SubscriptionClient): any {
+  console.warn('This method becomes deprecated in the new package graphql-transport-ws. Start using the ' +


Let's make sure this warning is not printed if NODE_ENV is production.

helfer · 2017-05-04T03:19:21Z

src/message-types.ts

+  public static GQL_DATA = 'data';
+  public static GQL_ERROR = 'error';
+  public static GQL_COMPLETE = 'complete';
+  public static GQL_END = 'end';


What is the difference between COMPLETE and END? If end is to stop an operation, we should call it STOP.

COMPLETE => Means no data from server
END => Means client doesn't want anymore data.

Ok, let's call that STOP. end is more commonly used as a noun. Stop is almost always a verb.

helfer · 2017-05-04T03:19:39Z

src/message-types.ts

+  public static GQL_ERROR = 'error';
+  public static GQL_COMPLETE = 'complete';
+  public static GQL_END = 'end';
+  // NOTE: Those message types are deprecated and will be removed soon.


In the new package we shouldn't have these at all.

that's exactly what this comment means.
but let's finish merging the compatible version first before moving forward

As discussed let's keep things related with backwards compatibility for now.

helfer · 2017-05-04T03:20:08Z

src/helpers.ts


 // Quick way to add the subscribe and unsubscribe functions to the network interface
+// We will move this into a new package in the future


In the new package we shouldn't have this, but we can keep it in the old one and just print the deprecation warning.

As discussed let's keep things related with backwards compatibility for now.

DxCx · 2017-05-05T06:22:44Z

src/server.ts

+    if ([
+        MessageTypes.GQL_CONNECTION_ERROR,
+        MessageTypes.GQL_ERROR,
+      ].indexOf(overrideDefaultErrorType) === -1) {


you forgot to use sanitizedOverrideDefaultErrorType in here.

Yeah thank you :D

dotansimha · 2017-05-06T15:03:30Z

@mistic @DxCx @Urigo
Noticed an issue earlier: In some cases, the client disconnects, but the onClose of the client isn't called, and the Network tab of the browser still thinks the client is connected.
The server side tried to publish data to this socket and getting "Not opened" error (the connection state is 3 = CLOSED).

mistic · 2017-05-08T16:26:28Z

@dotansimha Have u tested with last ws version? Ins't the problem related with ws library itself?
websockets/ws#686

mistic · 2017-05-09T21:00:04Z

Guys ( @DxCx @helfer @NeoPhi @Urigo @dotansimha or anyone else interested in give some help ) will be really appreciated some help with those 2 tasks:

Update documentation and also including examples for full and hybrid ws transport on it.
Update CHANGELOG.md with changes made.

…ng on old tests to keep working on new code. fix(NA): error handling between client and server. refact(NA): applied some changes purposed on pull request. refact(NA): change executeRequest to executeOperation on client. fix(NA): apply console warn only outside production env. fix(NA): import one by one to reduce bundler size. fix(NA): return original errors format. test(NA): refact some tests to work with new version. refact(NA): removed console logs from tests. test(NA): update old tests. test(NA): update old tests. refact(NA): changed requests to operations. fix(NA): add return to discard invalid messages. test(NA): fixed bad timeout on test. test(NA): commented non sense unit test. test(NA): adapt legacy test to new server code logic with unsubscribe. test(NA): fixed some legacy tests. fix(errors): error handling between client & server chore(NA): added todo about validation error in the server.

…s-protocol-review

helfer · 2017-05-12T02:50:04Z

@mistic wow, great work! I've been really busy myself, so I couldn't help or even review until now. If Uri can't review, I hope I can do it over the weekend.

mxstbr · 2017-05-12T15:41:19Z

Do I understand correctly that this PR makes it possible to run the GraphQL and the subscriptions server off of the same server and thus the same port? (which is a requirement for deploying on now.sh)

I've tried taking the code sample above and using the branch this PR lives in, but I can't seem to make it work. Are there any docs yet so I can try this out and give some feedback?

mistic · 2017-05-12T16:20:39Z

@mxstbr You'll be able to understand how to use this PR looking for those already changed GitHunt example to support this PR:

https://github.com/mistic/GitHunt-API/tree/full-transport-ws
https://github.com/mistic/GitHunt-React/tree/full-ws-transport

mistic · 2017-05-12T16:27:22Z

Guys ( @DxCx @helfer @NeoPhi @Urigo @dotansimha ) I think we're reaching the end of the work on this PR. Today I merged into this PR the Uri changes to correctly support subscribe method from graphql-js into our server graphql executor. Also I already wrote the new CHANGELOG.md.

Work needed to be done to merge this PR:

Write some tests to cover the new logic and the mapping features. All the old ones are already updated and being pass.
Update documentation to reflect the new API on server and client and also provide examples.

docs(NA): changelog update. docs(NA): changelog update. docs(NA): changelog update. feat(NA): add subscribe from graphql. fix(NA): check for connection state inside sendMessage. fix(NA): merge executeFromSubscribe with executeFromExecute. feat(NA): changelog update. fix(NA): clear immediate action on unsubscribe. fix(NA): removed typo from condition. fix(NA): added esnext to tslint config.

Urigo · 2017-05-14T14:56:34Z

@mistic amazing amazing work.
I can't thank you enough for all your effort.
There are still more tasks to be done, but I'm going to merge this now because I want @thebigredgeek and @srtucker22 to be able to work and push their changes on top of the new API.

During the next couple of days:

Add tests for the new API - @mistic
Add documentation for the new API - @mistic
@thebigredgeek and @srtucker22 - Rebase your PR against the latest master and the new API.
I will make sure to release a version 0.7.0 that supports the current API without breaking changes, but also include the new API based on the upcoming graphql-js with subscriptions
I will remove all the old API code and will release a new package called graphql-ws-transport

Urigo · 2017-05-16T14:14:44Z

@mistic , thank you again for your great PR.
During your work, a new graphql.js subscriptions support came to be.

This changes a bit our goals so I want to reflect on that for a sec, to create an updated path with slight adjustments.

V0.7 goals:

Full backward compatibility
New API
- Support query and mutations over WebSocket, with option for hybrid.
- Support new graphql.js subscriptions with AsyncIterable

Changes by @mistic :

Added support for full websocket network interface.
Added support for hybrid network interface (using addGraphQLSubscriptions method)
Implemented new transport protocol
Added ExecuteAdapters to support both old API (SubscriptionsManager) and new API (graphql subscribe)
Added executeReactive as default payload source (no need to run ExecuteAdapter), and implemented the transport with Observable base.

On top of those changes, I’ve added the support for the new graphql.js subscriptions implementation:

Transport work natively with AsyncIterable for payload source (but requires ExecuteAdapters)
Added ability for the transport to expect execute and subscribe directly from graphql-js new subscription implementation.
- Tiago changed the API from new SubscriptionsClient({ execute, subscribe }) to new SubscriptionsClient({ executer: { execute, subscribe } }) in order to make executeReactive as named priority
Transport works with the recent graphql-subscriptions master (with both new and old API).

The problems I have with the current state are those:

We understood from the graphql.js subscriptions work that in most cases, we should separate initiating the subscriptions and the firing events Lee’s comment here
That means we want to make that common API as simple as possible and the default one instead of the special executeReactive API
Once we do that, we should come up with a better design on how to support the executeReactive approach without complexing the common API and implementation

So the changes I'm proposing to do before releasing 0.7 are:

Transport work natively with AsyncIterable for payload source to make the most common use case the simplest and the default one supported (instead of executeReactive)
Transport should expect execute and subscribe directly from graphql-js new subscription implementation
bring back the simple new SubscriptionsClient({ execute, subscribe }) API
In order to support executeReactive while keeping this library simple, I believe the right solution should be to add a different executor library for that approach. That library should use this transport like so: expose subscribe and execute directly and include an adapter for Observable -> AsyncIterable internally, instead of placing that code inside this library
Merge the two open PRs to help with authentication:
- lazy mode and deferred connection params #124
- onSubscribe and use middleware to modify runtime SubscriptionOptions #78
Solve those issues:
Tests for all the new changes and APIs
Docs for all the new changes and APIs

V0.8 goals:

Clean and remove all backward compatible code

V0.9 goals:

Ability to replace ws library with uws or any other WS implementation library

@DxCx @mistic @helfer @stubailo @NeoPhi I want to hear your opinions on that plan

DxCx · 2017-05-16T15:02:05Z

@Urigo the work on Websocket is done for ~0.5year now,
I've already written it 3 times, while Tiago now did the 4th time with me helping him,

we used observables back then and that's the reason it has observables, AsyncIterator was not in the picture until last month. (more then that, supporting Observables in GraphiQL was already in progress, which made sense why using it).

if you want to change executeReactive signature to return AsyncIterator, I'm totally fine with that because graphql internally started to use, but we shouldn't remove executeReactive, if you have a better proposed solution, let me know.

so, unless you know that plans for how facebook is going to support reactive directives such as @defer or @live we need a reactive execute function to be supported,
and the executor pattern is great because it gives you great flexibility.

about the versioning, generally i agree about the content but i think that V0.8 Should be V1.0 or something like that because it will have a lot of breaking changes.

Urigo · 2017-05-16T18:44:59Z

@DxCx

I totally understand you pain, but let's try to solve this together. that's why I wrote this comment.
Let's separate WebSocket work and @live. WebSocket is now merged and it can work without executeReactive. You now will be able to send queries and mutations over WS as well which is something a lot of people as asked for. So it doesn't matter how long WebSocket work has been done, now it's in master and it can work even without executeReactive.
AsyncIterator wasn't a thing, that's why I'm writing this comment, because now it is a thing. If we really want to build the tools that help GraphQL developers use the main reference implementation and help them do that in production environment with the minimum changes, we should change that API as little as possible. On other libraries we can create completely separate APIs, but those are different libraries with different goals. We should be really careful about what is the goal for each library.
I want to work together in order for you to be able to use as much from this library as possible in your solution, without enforcing everyone to add size and complexity if they are not going to use your solution. I would love to hear more design ideas from you about how to do that. Do you think that there is a solution where you wrap the API that I described in another library and exposing the API you want without hurting the API that 90% of the users will use?

I agree with you about versioning. let's do a v1.0 after v0.7.

DxCx · 2017-05-16T20:20:40Z

alright, @Urigo so as i suggested we can move implementation to async iterator instead of observable, and that will be the smallest change possible, i don't understand why do we need to completely remove executeReactive support if it won't really hurt anyone...

also, it will let people with legacy subscription server upgrade to run mutation + queries with one or two lines of change, not needing to rebuild their whole schemas.

one last thing is that if you can discuess lee about making executeReactive (or any other name you will choose) that accepts executes parameters and return AsyncIterator we can also make sure that the name will less likely to change.
the implementation for such is very easy:

check operation if subscription => return subscribe
else return AsyncIterator wrapped promise of execute

mistic · 2017-05-17T01:07:59Z

@Urigo I agree with @DxCx in almost everything!

1 - the v0.8 should be the v1.0.0. It will have a lot of breaking changes and it's almost a completely rewrite. In my opinion is a major version.

2 - Regarding to the SubscriptionsServer, what's the problem with the executor pattern? In my opinion this isn't a deal breaker with anything coming from graphql-js. We just accept an executor in our Server that can be composed by execute & subscribe from graphql-js, by executeReactive from graphql-subscriptions or simply execute from graphql-subscriptions and subscribe from graphql-js. What's the real problem with this thing? Is simply a design choice from our server. In the limit we could provide some predefined executor interfaces to provide more context to the user when instantiating the server.

3 - If you prefer we can move observables to AsyncIterator.

dotansimha · 2017-05-17T10:53:13Z

@mistic @DxCx @Urigo
What do you think about accepting either Promise or AsyncIterator for execute function?

This means that Hagai can implement @live logic inside a separate package, and expose two functions: execute and subscribe (just like graphql-js does).

The transport can check if the return value is AsyncIterator and wait for reactive changes and publish it over the socket for each value, or use Promise and publish it only once.

DxCx · 2017-05-17T12:58:00Z

@dotansimha that works as well!
my only problem with that is we did not get Lee's confirmation.
which means that we are going to modify a signature of an existing function.
so, if future plans of facebook is to modify execute signature, that's fine.
but if not, it's better to just introduce a new one IMO.
which is essentially the executeReactive introduced here.
as both me & @mistic stated above, there is no problem to eliminate observable reference and support only AsyncIterator

dotansimha · 2017-05-17T13:04:07Z

@DxCx
We don't need to modify the execute signature in GraphQL. We just need to accept a return value of both options, and handle it correctly inside this transport.
It does not require any modifications in graphql-js, the changes could be in here:
https://github.com/apollographql/subscriptions-transport-ws/blob/master/src/server.ts#L53-L60

And change it to:

export type ExecuteFunction = (
  schema: GraphQLSchema,
  document: DocumentNode,
  rootValue?: any,
  contextValue?: any,
  variableValues?: {[key: string]: any},
  operationName?: string,
) => Promise<ExecutionResult> | AsyncIterator<ExecutionResult>;

This way, graphql-js remains the same, but the transport can support both Promise and AsyncIterable.

What do you think?

DxCx · 2017-05-17T13:07:09Z

@dotansimha i totally understood your meaning, as i said it could work.
but then, don't matter which package will be able to return AsyncIterator as execute, it won't be compatible with other graphql tools except for this package.
the only reason to do it is if reference implementation are going towards this direction as well instead of introducing a new call, but i really don't believe this will happen as it is a breaking change.

dotansimha · 2017-05-17T13:41:04Z

@DxCx
What about exposing AsyncIterator along with IObservable from packages?

For example, Hagai's ReacticeExecutor can expose execute that returns AsyncIterator along with executeObservable that returns IObservable.

DxCx · 2017-05-17T13:54:22Z

@dotansimha as i mentioned above, i don't mind terminating the observable.
the only intention with observables is to be able to return reactive result. AsyncIterator gives exactly that as well.
the idea is to have an execute signature that also returns an AsyncIterator, if you suggested executeObservable then i really don't see a reason why not keep the existing name of executeReactive which returns AsyncIterator instead of Observable?

dotansimha · 2017-05-17T13:55:56Z

I meant to expose executeObservable from your GraphQL alternative executer package (the implementation that in your graphlq-subscription PR).

DxCx · 2017-05-17T14:20:59Z

@dotansimha i think there is a confusion in here, this is what i'm trying to achieve:

change exiting executeReactive signature to return AsyncIterator instead of Observable
change all inner Observable implementation to AsyncIterator, removing Observable code completely
change GraphQLExecutorWithSubscription (graphql-subscriptions) to return AsyncIterator and not observable

Do you agree?

dotansimha · 2017-05-17T14:26:45Z

@DxCx Yeah that's sound great.

Also, I think that since we can remove SubscriptionsManager in a future version, and the dependency for graphql-js in graphql-subscritpions - maybe it's better to implement this in a new package?

I think that the future version of graphql-subscriptions will only contain utils like channel mapping, filters and such.

What do you think?

DxCx · 2017-05-17T14:31:21Z

the idea behaind it was to allow people people that are using graphql-subscriptions today to upgrade easily to full graphql over websocket.
there is no reason for that patch as well if there is no SubscriptionManager.
so, when we clean subscription manager we can clean this too (as long as last supported version of subscription manager has it)

* chore(package): update graphql to version 0.7.0 https://greenkeeper.io/ * fix failing tests

feat(NA): new protocol supporting queries, mutations and subscription…

98a7f52

…s both on client and server side using websockets as transport layer to be used on a new package called graphql-transport-ws. fix(NA): fixed path of new files.

DxCx mentioned this pull request Apr 15, 2017

Question: Why two different graphql endpoints? #109

Closed

Urigo mentioned this pull request Apr 18, 2017

onSubscribe and use middleware to modify runtime SubscriptionOptions #78

Merged

4 tasks

DxCx mentioned this pull request Apr 19, 2017

ws Network Interface Integration apollographql/apollo-server#272

Closed

mistic force-pushed the graphql-transport-ws-protocol branch 2 times, most recently from 10dafaf to 0fedbd3 Compare April 20, 2017 22:51

DxCx reviewed Apr 21, 2017

View reviewed changes

mistic force-pushed the graphql-transport-ws-protocol branch from 24202de to 4952d35 Compare April 22, 2017 14:42

mistic force-pushed the graphql-transport-ws-protocol branch from 6ae8112 to 7b19459 Compare April 22, 2017 16:28

DxCx reviewed Apr 29, 2017

View reviewed changes

mistic force-pushed the graphql-transport-ws-protocol branch from ff666f7 to 062d1d9 Compare May 2, 2017 14:21

refact(NA): merge with last changes on master.

c2a1988

helfer suggested changes May 4, 2017

View reviewed changes

DxCx reviewed May 5, 2017

View reviewed changes

Urigo mentioned this pull request May 9, 2017

WIP: Support for new graphql-js subscriptions support #122

Closed

Merge branch 'graphql-transport-ws-protocol' into graphql-transport-w…

6b33b99

…s-protocol-review

mistic force-pushed the graphql-transport-ws-protocol branch from 2c48656 to 1be31a1 Compare May 11, 2017 16:59

mistic force-pushed the graphql-transport-ws-protocol branch from 407969e to 23cc224 Compare May 12, 2017 16:17

mistic force-pushed the graphql-transport-ws-protocol branch from e5db8a9 to a854e33 Compare May 12, 2017 16:54

Urigo merged commit 0e81e9e into apollographql:master May 14, 2017

Urigo mentioned this pull request May 16, 2017

introduce GraphQLExecutorWithSubscriptions apollographql/graphql-subscriptions#47

Closed

1 task

dotansimha mentioned this pull request May 17, 2017

WIP: Continue work to support new graphql-js #133

Merged

4 tasks

DxCx mentioned this pull request May 18, 2017

GraphQL-server websocket support apollographql/apollo-server#308

Closed

4 tasks

baconz pushed a commit to PhiloInc/subscriptions-transport-ws that referenced this pull request Apr 24, 2018

Update graphql to version 0.7.0 🚀 (apollographql#108)

e60f279

* chore(package): update graphql to version 0.7.0 https://greenkeeper.io/ * fix failing tests

		@@ -165,95 +149,151 @@ export class SubscriptionClient {
		}

		public unsubscribe(id: number) {

		export function addGraphQLSubscriptions(networkInterface: any, wsClient: SubscriptionClient): any {
		console.warn('This method becomes deprecated in the new package graphql-transport-ws. Start using the ' +


		// Quick way to add the subscribe and unsubscribe functions to the network interface
		// We will move this into a new package in the future

WIP: Graphql Transport WS Protocol #108

WIP: Graphql Transport WS Protocol #108

Conversation

mistic commented Apr 13, 2017 • edited Loading

apollo-cla commented Apr 13, 2017

Urigo commented Apr 18, 2017

DxCx commented Apr 19, 2017

mistic commented Apr 20, 2017 • edited Loading

Choose a reason for hiding this comment

mistic Apr 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mistic commented Apr 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mistic commented May 2, 2017

Urigo commented May 2, 2017

mistic commented May 2, 2017 • edited Loading

helfer 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

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

DxCx May 4, 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

dotansimha commented May 6, 2017

mistic commented May 8, 2017

mistic commented May 9, 2017 • edited Loading

helfer commented May 12, 2017

mxstbr commented May 12, 2017

mistic commented May 12, 2017

mistic commented May 12, 2017

Urigo commented May 14, 2017

Urigo commented May 16, 2017 • edited Loading

DxCx commented May 16, 2017

Urigo commented May 16, 2017 • edited by DxCx Loading

DxCx commented May 16, 2017 • edited Loading

mistic commented May 17, 2017

dotansimha commented May 17, 2017

DxCx commented May 17, 2017

dotansimha commented May 17, 2017

DxCx commented May 17, 2017

dotansimha commented May 17, 2017

DxCx commented May 17, 2017

dotansimha commented May 17, 2017 • edited Loading

DxCx commented May 17, 2017 • edited Loading

dotansimha commented May 17, 2017

DxCx commented May 17, 2017 • edited Loading

mistic commented Apr 13, 2017 •

edited

Loading

mistic commented Apr 20, 2017 •

edited

Loading

mistic Apr 22, 2017 •

edited

Loading

mistic commented May 2, 2017 •

edited

Loading

DxCx May 4, 2017 •

edited

Loading

mistic commented May 9, 2017 •

edited

Loading

Urigo commented May 16, 2017 •

edited

Loading

Urigo commented May 16, 2017 •

edited by DxCx

Loading

DxCx commented May 16, 2017 •

edited

Loading

dotansimha commented May 17, 2017 •

edited

Loading

DxCx commented May 17, 2017 •

edited

Loading

DxCx commented May 17, 2017 •

edited

Loading