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

Add web socket server #407

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Add web socket server #407

wants to merge 3 commits into from

Conversation

jquense
Copy link
Member

@jquense jquense commented May 18, 2021

Adds an option for a pure websocket server.

@jquense jquense requested review from itajaja and r1b May 18, 2021 19:21
}

class GraphQLSocket extends EventEmitter {
protocol: 'graphql-transport-ws' | 'socket-io';
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The plan is to support: GraphQL over websockets protocol next

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i am a little confused, this GraphQLSocket only seems to be used by the websocket, so why have the socket-io protocol as an allowed value?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah sorry. So in this case "socket-io" is really "4C's bespoke protocol of connect and authenticate calls" I don't remember why i named it "socket-io" maybe just because it depends on the socket-io handshake events

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and by "I named it" what do you mean by that? where is this name specified? do you think it's too late to change? maybe we can add a comment to avoid confusion

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's made up by me. this theoretically supposed to match the web socket sub protocol. 'socket-io' is a subprotocol used by socket-io which is why i just named it that for the pure web socket but it's not actually that protocol so i should disambiguate and name it 4c-subscription-server

@jquense jquense mentioned this pull request May 20, 2021
this.io.on('connection', (socket: io.Socket) => {
const request = Object.create((express as any).request);
Object.assign(request, socket.request);
this.opened(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

onOpenConnection or initConnection

src/types.ts Show resolved Hide resolved
}

class GraphQLSocket extends EventEmitter {
protocol: 'graphql-transport-ws' | 'socket-io';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i am a little confused, this GraphQLSocket only seems to be used by the websocket, so why have the socket-io protocol as an allowed value?

src/types.ts Show resolved Hide resolved
src/WebSocketSubscriptionServer.ts Outdated Show resolved Hide resolved
BREAKING CHANGE: the subscription server export is now an abstract class
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants