Skip to content
This repository was archived by the owner on Aug 18, 2020. It is now read-only.

Added realtime updates #78

Merged
merged 3 commits into from
Jul 13, 2019
Merged

Added realtime updates #78

merged 3 commits into from
Jul 13, 2019

Conversation

cesmec
Copy link

@cesmec cesmec commented Jul 8, 2019

Sends live events to the gui using server sent events.
Currently contains start/stop and log of instances.
Based on Issue #74.

@J0B10 J0B10 requested a review from sebinside July 8, 2019 19:52
@sebinside sebinside self-assigned this Jul 9, 2019
@sebinside
Copy link
Member

Thank you for this pull request. The usage of server side events is an interesting approach which might solve a lot of (future) problems with the gui connection.

To start, I want to integrate the events endpoint in the chatoverflow-api npm package, based on your swagger documentation. A callable method might also solve your todo, but I am not into events that much. https://github.com/codeoverflow-org/chatoverflow-gui/pull/2/files#diff-401c1d73fa16a958f7072b5491740c14R176

But first, we have to discuss the API interface. We used to send the auth key as header field. In your definition, your sendind it as parameter. Is this a limitation of the EventSource? b66b821#diff-be1fae0b59ee0da30023b30b6b25854eR15

I'm also not understanding the comment completely: Requires the authKey as a cookie and an Accept-header with the value text/event-stream.

After we solved this, I will include the PluginInstanceWithLog and the events endpoint into the npm package, so you can properly update the gui.

@sebinside
Copy link
Member

And we should include a big warning, when the EventSource API is not supported because it will probably be used for more when we're ready. Still good support though.

@cesmec
Copy link
Author

cesmec commented Jul 9, 2019

Depending on the future use cases, it might make sense to use websockets instead, which would also solve the IE/Edge support.
The main difference is that websockets allow for bi-directional communication but might require additional libraries in the backend (I just wanted to make a simple implementation for now as I have not used scala before).
Custom headers for authentication are not supported in both, so either a cookie or a parameter can be used instead and a parameter is a bit easier to implement as it can just be added to the url.

The comment about the Accept-header is just a hint that this value is expected and testing if the client expects a long running event stream. If the request does not include this header, it will be rejected: https://github.com/codeoverflow-org/chatoverflow/pull/78/files#diff-3acd087eb793d2cd5e40247c8f9a4379R48
I think it's not possible to document server sent events (or also websockets) correctly using OpenAPI, which is also the reason it is documented as returning Object: https://github.com/codeoverflow-org/chatoverflow/pull/78/files#diff-be1fae0b59ee0da30023b30b6b25854eR9

@sebinside sebinside added this to the pre-alpha 3 milestone Jul 11, 2019
@sebinside sebinside added the enhancement New feature or request label Jul 11, 2019
@sebinside
Copy link
Member

I would use SSE for the same reasons. But you're right, it's not compatible to swagger / open api: OAI/OpenAPI-Specification#396

The hint with the accept header is good idea. Beeing unable to set the authkey in the header is not the best, but I also agree that params are easier then cookies here.

@sebinside
Copy link
Member

I updated the npm package. PluginInstance contains now the log field by default, and the API contains an event endpoint following your controller definitions. I also updated the gui to match the new specifications and types.

https://www.npmjs.com/package/chatoverflow-api
https://github.com/codeoverflow-org/chatoverflow/tree/cesmec-feature/realtime-gui-updates
https://github.com/codeoverflow-org/chatoverflow-gui/tree/cesmec-feature/realtime-gui-updates

The usage of events is useful for future versions of the gui, probably for more than just instances and log events. There are a few more things to do:

  • Please overload the broadcast function to take native scala maps. This code is used repeatetly and could be encapsulated: https://github.com/codeoverflow-org/chatoverflow/pull/78/files#diff-63eaaad4b847c7b58af51a60562eaadcR36
  • Please document the dispatcher since it will probably be used from multiple endpoints in the future
  • Please refactor the client integration of the event handling. In the future, there will be more than just one page requireing events. The handling could be encapsulated in a separate service like the swagger services (ConfigService and so on) or the self written Crypto Service.

That's all. I'm looking forward to integrate this feature when you're ready!

@sebinside sebinside mentioned this pull request Jul 11, 2019
3 tasks
@cesmec cesmec marked this pull request as ready for review July 12, 2019 20:29
@sebinside sebinside merged commit e543485 into codeoverflow-org:develop Jul 13, 2019
@sebinside
Copy link
Member

Everything is fine. Thank you for this great addition!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants