Skip to content
This repository has been archived by the owner on Oct 22, 2020. It is now read-only.

Allow the server to push messages to the client #312

Closed
tamlyn opened this issue Jul 6, 2018 · 8 comments
Closed

Allow the server to push messages to the client #312

tamlyn opened this issue Jul 6, 2018 · 8 comments
Assignees
Labels
Server-Side Any issue regarding the server side components.

Comments

@tamlyn
Copy link
Contributor

tamlyn commented Jul 6, 2018

User Story:

As a user I want the application to update as soon new information is available.

Options

I would favour GraphQL subscriptions over pubsweet-sse in order to be consistent with our technology choices and also because the latter has a bunch of legacy behaviour which we don't need or want. I favour SSE over WebSockets because it is a simpler technology and requires less fiddling with firewall rules etc, however in this instance the corresponding package is much less popular and would need to be carefully audited before use. The two subscription transport packages are meant have the same API so it should be easy enough to switch if the need arose.

Product requirements/Acceptance criteria:

Functional:

  • Implement one of the above options to allow the pushing of messages from the server to the client
@hdrury1 hdrury1 added Server-Side Any issue regarding the server side components. Epic1 labels Jul 6, 2018
@diversemix
Copy link
Contributor

@tamlyn I would tend to agree to using GraphQL as its the technology of the API which is where the pub-sub model should be implemented. Do we need an ADR for this?

@mihaildu
Copy link
Contributor

Update:

I decided to skip pubsweet-sse, since it uses REST API and it would be a nice thing to have GraphQL subscription support in pubsweet as well. Looking at the options for GraphQL subscriptions:

  1. subscriptions-transport-sse is using some deprecated API from graphql-subscriptions so it doesn't really work. We could try to fix it but I think that would be too much work.
  2. subscriptions-transport-ws - I managed to get this running locally (not in pubsweet though). Right now there is a bug that needs fixing at !337. However, it seems to work just fine in GraphiQL.

@mihaildu
Copy link
Contributor

I managed to make it work for now with subscription-transport-ws. There is still some work to be done to make the code more maintainable and the changes in pubsweet are currently in review.

I also just realized that the PR I did here #368 has nothing to do with this issue, but rather with #313.

The MR that should solve this issue is actually the one on pubsweet here https://gitlab.coko.foundation/pubsweet/pubsweet/merge_requests/337

If the pubsweet MR doesn't get merged, we could always do something on our side I expect (like using our own apollo client with a websocket link and starting the websocket server somewhere on the server side).

@diversemix
Copy link
Contributor

@Dryhten - Can you actively start a conversation with Jure about this on mattermost please. See how far you get and drag me in if needs be. Thanks 👍

@mihaildu
Copy link
Contributor

@diversemix there is a conversion about this on mattermost with jure that's currently ongoing and has started last week.

@mihaildu
Copy link
Contributor

mihaildu commented Jul 31, 2018

The MR on pubsweet for graphql subscriptions was on hold because there was this discussion with pubsweet people if it should be part of a bigger feature or not (the notification system on the server). The outcome was that it shouldn't be part of it, so now it's back at waiting review.

Some things left to be done:

@mihaildu
Copy link
Contributor

Only thing left to be done is wait for review on this https://gitlab.coko.foundation/pubsweet/pubsweet/merge_requests/337/

And fix pipeline errors if there are any.

@mihaildu mihaildu removed the Blocked label Aug 16, 2018
mihaildu added a commit that referenced this issue Aug 17, 2018
mihaildu added a commit that referenced this issue Aug 17, 2018
this is to account for latest changes in pubsweet

re #312
mihaildu added a commit that referenced this issue Aug 17, 2018
new version has support for graphql subscriptions

re #312
@mihaildu
Copy link
Contributor

Done in https://gitlab.coko.foundation/pubsweet/pubsweet/merge_requests/370 using GraphQL subscriptions with WebSockets and subscriptions-transport-ws.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Server-Side Any issue regarding the server side components.
Projects
None yet
Development

No branches or pull requests

4 participants