Mattermost-server: Move webapp to use websocket event handling from mattermost-redux

[esethna]

If you're interested please comment here and come join our "Contributors" community channel on our daily build server, where you can discuss questions with community members and the Mattermost core team. For technical advice or questions, please join our "Developers" community channel.

New contributors please see our Developer's Guide, specifically for machine setup and for developer workflow.

---

Notes: Jira ticket

The webapp still uses it's own implementation of the websocket event handling.

Replace the websocket event handling in mattermost-webapp/actions/websocket_actions.jsx with the implementation in mattermost-redux/src/actions/websocket.js.

Add events, actions and store dispatches to the mattermost-redux implementation as needed.

This will be an involved ticket. A good understanding of the webapp and mattermost-redux is recommended.

Ask @joram on community.mattermost.com in the Redux channel for help or advice if needed.

[anindha]

@jwilander @hanzei I can take this ticket.

[hanzei]

Awesome, thanks @anindha :+1:

[AGMETEOR]

@hanzei I might be wrong on this, but I think the webapp doesn't use a lot of the functions in the mattermost-webapp/actions/websocket_actions.jsx.

[hanzei]

@jwilander Might know more about this

[jwilander]

That's definitely possible, we should remove dead code if found

[anindha]

I was working on another issue, so a little delayed getting to this. Have started working on this now.

[anindha]

@jwilander @sudheerDev I looked through the websocket code for both redux and the webapp and made some notes.

The two code bases diverge for about 50% of the event handlers. Do you have any suggestions on a good architecture to allow the custom code to be run on the webapp but still have the common code to run.

Some possible solutions:

1. The webapp websocket_actions.jsx wraps the redux websocket.ts and reuses code where possible
2. Pass an additional event handler to the redux websocket.ts that the webapp would pass in and handle additional logic
3. Webapp listens to either redux events, or EventEmitter events and performs additional logic.

[jwilander]

To me, 3) feels the cleanest but I have a feeling that achieving that might be very hard and would almost require specific WebSocket events being emitted which would feel like a duplication of places handling websocket events.

What about re-building the handler in websocket.ts as a class with all the different event handlers as methods on that class. The web app can then import that class and just override any of the methods it wants needs to?

As a first step though I think it might be a good idea to go look at all the cases where the webapp and redux websocket handlers diverge and ask if they really need to be diverged or if they can actually be the same. My guess is that they majority of the time the answer is that they can be the same and the only reason they are different is organic independent growth. I may be wrong about that but it feels like the case to me

Thoughts?

[anindha]

My guess is that they majority of the time the answer is that they can be the same and the only reason they are different is organic independent growth.

@jwilander I made a table of functions with comments on how they diverge here. Half of the events require additional functionality on the web app.

I like the idea of having a class that is passed into websocket.ts. Would you have an interface method per event or one interface method that handles all events?

[sudheerDev]

@anindha Thanks for the interest on this and making a big effort with the doc so far. It is much easier to get the bigger picture with the way we handled events on both mobile and web.

Webapp listens to either redux events, or EventEmitter events and performs additional logic.

i am in favour of redux events feels much natural to me and it becomes easier for clients to handle them based on that. I went through the doc and most of the functionality differences are because of missing functionality. They should be similar other than any view state dependent events(Like closing RHS, modals etc).

We might want to bring them closer by adding them so mobile comes in parity with web.

[hmhealey]

We could try using redux here, but I'd be hesitant about that in all cases since redux actions aren't events. You can't respond to a redux action to trigger async calls or anything else that's necessary.

I'd look at either turning the websocket handler into an EventEmitter with two listeners (redux and the web app) or into a class that can be extended by the web app.

The only thing that might make that more difficult is if the web app ever needs something from the redux version to not happen. I don't think that will happen, but if that's the case, we could have the mobile app also listen to or extend the websocket handler and move any of that logic to it.

[jwilander]

@hmhealey for your last point if the we're using a class that the web app is extending, it can just completely override the redux methods anywhere it wants to do something completely different. So I think it fits really nicely.

@anindha I think a single interface method should be fine. All the arguments to each handler are identical (just the WS message at this point). Also awesome doc 👍 I'm going to go through it and add thoughts where we diverge

[hanzei]

HI @anindha,

I just want to touch bases and check if you are still working on this issue :wink:

[hmhealey]

This ticket may no longer be valid since mattermost-redux will no longer be shared between the mobile and web apps. I'll try to keep this updated in case that happens.

[sudheerDev]

@hmhealey at this point I am inclined to closing this and later remove the redux version of socket code. Thoughts?

[hmhealey]

I didn't realize this was still open actually. Thanks for pointing that out.