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

[discussion]: Custom GraphQL over WebSocket protocol when enabling subscriptions #1341

Closed
asmeikal opened this issue Jan 24, 2021 · 3 comments · Fixed by #1348
Closed

[discussion]: Custom GraphQL over WebSocket protocol when enabling subscriptions #1341

asmeikal opened this issue Jan 24, 2021 · 3 comments · Fixed by #1348

Comments

@asmeikal
Copy link
Contributor

asmeikal commented Jan 24, 2021

When enabling subscriptions, the GraphQL module relies on the default behaviour of Apollo Server, which is to use subscriptions-transport-ws. The library is not actively maintained anymore, and their README points to graphql-ws as an alternative. Apollo Server has still subscription-transport-ws in their docs (see here), but according to their roadmap they want to remove it somewhere in the future.

subscriptions-transport-ws has several protocol bugs that haven't been addressed in the last year. One of them is this one, which allows connections to be established while skipping the recommended way to perform authentication. Other protocol bugs are on the client side, and they keep coming even after the last batch of bug fixes, and old bugs have still not been addressed.

subscriptions-transport-ws is clearly not production ready. I've seen the impact of all of these bugs on the project I'm currently working on, and fixed some of them by forking the library and using the fork in our NestJS subscription servers, but we are desperate for a long term solution. Switching from subscriptions-transport-ws to graphql-ws (as suggested on the first library repo) cannot be done transparently: the two protocols have differences in the structure of messages that are exchanged between client and server that make them incompatible, so which protocol to use should be left as a choice to the user.

What I'd like is for the GraphQL module to allow the user to choose which GraphQL over WebSocket protocol to use, while still defaulting to subscriptions-transport-ws. It should be possible, since subscriptions-transport-ws is not directly referenced anywhere in this repo, but I still haven't made an attempt. I will try as soon as I can and update this issue.

EDIT: I've put together a simple and rough integration of graphql-ws. It's available here and it's in no way good, but it works. The new protocol is enabled by passing protocol: 'graphql-ws' to the subscriptions option of the GraphQL module. The subscriptions option is a union to allow this (see here). Using the graphql-ws protocol is relatively straight forward (see here), but that code is relying on this function provided by graphql-ws, and the authors say that it's just an example implementation.

I've added a test to make sure that the subscriptions-transport-ws protocol still works, and one to test the graphql-ws protocol.

@kamilmysliwiec
Copy link
Member

Thanks for reporting & leaving a good explanation. If it's something that Apollo has on their roadmap, we should start tracking this too. Would you like to submit a PR?

@asmeikal
Copy link
Contributor Author

Hi @kamilmysliwiec, yes, I can try to clean up the example to get a PR for this.

I've been expanding the example implementation, and everything provided by NestJS seems to work out of the box. There are some differences on how authentication should be performed when using connectionParams, it should probably be addressed in this documentation issue once this is done.

@kamilmysliwiec
Copy link
Member

Let's track this here #1348

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 a pull request may close this issue.

2 participants