Skip to content
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

ws Network Interface Integration #272

Closed
DxCx opened this issue Jan 19, 2017 · 14 comments
Closed

ws Network Interface Integration #272

DxCx opened this issue Jan 19, 2017 · 14 comments

Comments

@DxCx
Copy link
Contributor

DxCx commented Jan 19, 2017

Hi All,

I've been speaking with @helfer and we got into agreement that i should add a pure websocket integration.
before writing anything, i want to post here some mini-design to share my thoughts and hear some feedback.

package name: i was thinking about graphql-server-ws, then the function will be called graphqlWs.

how to use it:
So, i don't see any reason why not having the same interface as our other packages, therefore:

import { graphqlWs } from "graphql-server-ws";
import { Server as WsServer } from "ws";
import { Schema } from './schema';
import { graphiqlExpress } from "graphql-server-express";

let app = express();
app.use("/graphiql", graphiqlExpress({
    endpointURL: `ws://localhost:3000/graphql`,
}));
let server = app.listen(3000, () => {
    let wss = new WsServer({ server: server });
    wss.on("connection", graphqlWs((ws) => {
        // The user can basically use any information he pleases too from ws connection object.
        const location = url.parse(ws.upgradeReq.url, true);

        // Multiplex ws connections by path.
        switch ( location.pathname ) {
           case "/graphql": // Same path graphiql is pointed to
               return {
                   context: {},
                   schema: Schema,
               };
           default:
               ws.terminate();
               return undefined;
        }
    });
});

NOTE: One more options is to do the multiplexing internally, but it breaks abit the similarity to other interfaces, and it will look like:

    wss.on("connection", graphqlWs({
      "/graphql": (ws) => {
          // user can still use any information he pleases to make descisions.
          return {
               context: {},
               schema: Schema,
          };
        },
    });

underlaying protocol:
so, as i can see it, there is no much difference between using HTTP POST as a transport, or a websocket.

the main difference are:

  1. websocket remains opens therefore you can send more then one request.
  2. because of the above, batching is redundant and there is no reason to support it.
  3. websocket needs the ability to update older response.
  4. client of the websocket should have the ability to cancel/close open request.

to solve this, i suggest to add the following fields to request:

  1. id field - shall be used to respond.
  2. action field - one of the following values:
  • request - default value incase action is missing, will trigger a new request,
    when used, query, variables and operationName will be used.
  • cancel - cancel or stop updates for Query Id, only id field is being used.
interface WebsocketGraphqlRequest {
  id: number; // Per socket increasing number
  action?: "request" | "cancel";
  query?: string; // GraphQL Printed Query.
  variables?: any; // GraphQL variables.
  operationName?: string; // GraphQL operationName
}

also, the response should also add response type and id, so it will look like:

interface WebsocketGraphqlResponse {
  id: number;
  type: "init" | "update" | "error" | "complete"
  payload: any; // standard object (data/errors) incase of init, 
                         // Error for error type
                         // diff object incase of update
                         // empty incase of complete.
}

therefore, if we request a query with 1 result,
the server will send:

  1. init + payload
  2. complete

but if we request a query with more then 1 result, the server can send diff updates,
and the client can construct the full immutable object over and over.

how diffing will be done:
i really liked the deep-diff library,
so i'm thinking of using it and keep the diff as is at the moment.

This is more or less what i had on my mind,
feel free to comment and give feedback :)

@NeoPhi
Copy link
Contributor

NeoPhi commented Jan 25, 2017

This is something that I'm interested in as well. A few thoughts below, not as fleshed out as yours at the moment.

I'd propose graphql-server-module-ws as I can see this being easily added to any one of the graphql-sever-* implementations, like GraphiQL.

I can't say that I'm a fan of the location semantics for configuration. I'd use graphqlOptions: GraphQLOptions passed into the constructor. The optional async functional version would be passed information about the socket/connection to allow inspection for dynamic options.

I think the request and response messages should be structured the same. Each would have id, type, and payload, where the message type defines the expected payload. Most common payload being query, variables, operationName. This provides a path for future message types and using subscriptions-transport-ws with a little adapter. Some initial types: QUERY_REQUEST, QUERY_RESPONSE, QUERY_CANCEL, and QUERY_CANCELED.

My hope is that code in graphql-server-core such as runQuery could be used to handle most of the low-level handling.

I'm unclear about the use-case for the diffing that you mentioned.

@DxCx
Copy link
Contributor Author

DxCx commented Jan 25, 2017

@NeoPhi Thanks!

i was already thinking about diffing because in the future i want to subscribe / @LiVe / @defer / @stream with it.
for that, you need diff capability ;)

@wmertens
Copy link
Contributor

wmertens commented Feb 6, 2017

Very nice!

I wanted to point out one extra advantage of using ws for graphql: Since the connection remain live for a long time, you only need to verify the user session once, so that saves on a bunch of database lookups.

@NeoPhi
Copy link
Contributor

NeoPhi commented Feb 6, 2017

Keep in mind you have some other mechanism to invalidate the WebSocket connection should the user's session become invalid for some reason.

@DxCx
Copy link
Contributor Author

DxCx commented Feb 8, 2017

Hi,

I've tried to review how can i support the current subscription manager for the new approach.
the best thing i could think about is to add another key named engine to the options.
so, when the options returned it will have it

               return {
                   context: {},
                   schema: Schema,
                   engine: new SubscriptionManagerEngine({ pubsub, setupFunctions }),
               };

the idea is to do the new in a static location and not inside a callback which calls every time ofcourse.

this way we can support this approach for legacy users who wants to switch,
and in the future add more engines,
what do you think about this approach?

CC:
@helfer / @stubailo / @NeoPhi / @Urigo / @davidyaha

@NeoPhi
Copy link
Contributor

NeoPhi commented Feb 9, 2017

@DxCx I'm having a little trouble fitting the pieces together. I presume the need for this would be to support @live like directives. How would the engine be accessed or used?

@DxCx
Copy link
Contributor Author

DxCx commented Feb 9, 2017

Well, the engine should export a reactive execution function. Which gets the same parameters as execute but returns an observable interface.

Usig this aproach, in the future we can write an executaion engine that supports the reactive directives, for now only subscription will be supported by the same interface as subscription manager as i exampled above

@helfer
Copy link
Contributor

helfer commented Feb 9, 2017

I like most of your proposal here @DxCx. 🙂

I think it's fine to keep the same interface for the websocket server as we did for the other servers until we see a need to change it.

For the websocket protocol, I agree with @NeoPhi that we should just extend (and maybe harmonize) what we already have for subscriptions. So messages both to and from the server should have { id: <operation id>, type: <START/DATA/COMPLETE/ERROR/STOP>}, payload

START is sent to start an operation (query, mutation or subscription) and contains payload of { query, variables, operationName }.

DATA contains the usual graphql response or an update to previous data in case of multiple results. I don't think this payload should have any fancy diff format at first, it should just contain all the data from the root upwards and overwrite whatever's in the store. I know this isn't necessarily efficient, but I don't believe we should lock ourselves into a diff format this early on.

COMPLETE is sent when no more results are to be expected from the server. This could also be STOP, but I kind of like to keep them separate because there's the small distinction that STOP could also be sent when not all data has been received. We could also roll this type up into DATA if we add some meta field to that message, but keeping it separate leaves us the option of saying that there is no more data even when no data was sent at all. You could imagine this use-case for a subscription, which only sends data on certain events. When it knows for sure no more of those events will happen, it can send COMPLETE without having to send any data. This also aligns more closely with observables.

ERROR is sent when there's an error that didn't happen inside graphql validation or execution. An error also means this particular operation is stopped and no further results will be received.

STOP is sent by the client whenever it isn't interested in more data. I don't think we need to differentiate between stop and cancel, so just one type will do.

Maybe we also need some sort of keepalive / heartbeat message to keep the socket connection open.

As for actually executing the GraphQL query on the server, I assume that this will eventually be taken care of by graphql-js entirely, but for now we should have our own engine (I prefer to call it executor, because that's what we're naming it on the client). That function should have the same function signature as the graphql-js graphql function, but it should return an observable instead of a promise. At the moment we still need to special-case subscriptions though, so we could call it ExecutorWithSubscriptions. I don't think we should call it SubscriptionManagerEngine, because that suggests that it contains only subscriptions, when it fact it also runs queries and mutations.

DxCx added a commit to DxCx/graphql-server that referenced this issue Feb 10, 2017
@NeoPhi
Copy link
Contributor

NeoPhi commented Feb 15, 2017

@DxCx Thinking more about sending diffs, I would suggest using an implementation that adheres to RFC6902 aka JSON Patch. There are many conforming implementations which would give the server and client a choice of the implementation to use.

@DxCx
Copy link
Contributor Author

DxCx commented Feb 15, 2017

nice! thanks @NeoPhi i didn't know this format,
well if there is a diffing format for JSON then i totally agree with you this is the one we should use.
i want initially to release the first version without any diff, and then later on i'll add it 👍

DxCx added a commit to DxCx/graphql-server that referenced this issue Mar 18, 2017
@DxCx
Copy link
Contributor Author

DxCx commented Mar 20, 2017

hey @helfer
i wanted to test compatibility between the existing websocket implementation and this one here,
they are not exactly the same:

  1. Message names are not exactly the same:
'subscription_fail' => `error`
'subscription_end' => `stop`
'subscription_data' => `data`
'subscription_start' => `start`
  1. subscription_start doesn't hold query & variables inside payload, but as a key in the root.
  2. there is no subscription_success for graphql-server-ws
  3. there is no complete (No more data from server) for subscription-transport-ws

how do you want to tackle those? align subscription-transport-ws to the protocol you described here? or make sure graphql-server-ws does exactly as subscription-transport-ws?
we can upgrade the server (of subscription-transport-ws) to support both formats, while newer client will use the cleaner alternative..

  1. subscription_start => will be handled as start (moving query & variables into payload), and marking oldProtocol flag.
  2. subscription_success will be sent only if oldProtocol is set.
  3. subscription_end => will be handled as stop
  4. subscription_data => will be handled as data

DxCx added a commit to DxCx/graphql-server that referenced this issue Mar 22, 2017
DxCx added a commit to DxCx/graphql-server that referenced this issue Mar 22, 2017
DxCx added a commit to DxCx/graphql-server that referenced this issue Mar 29, 2017
DxCx added a commit to DxCx/graphql-server that referenced this issue Apr 9, 2017
@vladshcherbin
Copy link
Contributor

@DxCx hey, any news on graphql queries over ws support?

@DxCx
Copy link
Contributor Author

DxCx commented Apr 19, 2017

@vladshcherbin @MrGekko
there is already an open PR for the initial protocol changes, you can track it here:
apollographql/subscriptions-transport-ws#108

@helfer
Copy link
Contributor

helfer commented Jun 28, 2017

Closing this for now as we've gone into a different direction for now 🤷‍♂️

@helfer helfer closed this as completed Jun 28, 2017
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants