Happychat: Improve connection Redux lifecycle #12708
Conversation
Instead of halting HAPPYCHAT_CONNECTING actions when there's already a connection, this uses the HAPPYCHAT_CONNECT action as a trigger for the middleware side-effects, which then in turn decides whether to dispatch HAPPYCHAT_CONNECTING and ultimately HAPPYCHAT_CONNECTED.
matticbot
added
the
[Size] XL
mattwondra
added
Happychat
[Status] In Progress
and removed
[Size] XL
mattwondra
added
[Status] Needs Review
mattwondra
changed the title
…tokens have been exchanged
matticbot
added
the
[Size] XL
There was a problem hiding this comment.
Tested and code looks good to me 🚢
One thing I noticed (which is the same as master) is that Happychat connects on the /help/contact page even if the user isn't eligible for chat. This isn't really an issue since the UI prevents starting a chat, but maybe an eligibility check is something we could consider adding to the HappychatConnection component.
| @@ -20,7 +20,10 @@ class Connection extends EventEmitter { | |||
| const socket = new IO( url ); | |||
| socket | |||
| .once( 'connect', () => debug( 'connected' ) ) | |||
| .on( 'init', () => resolve( socket ) ) | |||
| .on( 'init', () => { | |||
There was a problem hiding this comment.
Should this be 'connected' instead of 'init'?
There was a problem hiding this comment.
I don't think either the base Socket.io or the Happychat service emit a connected event. connect is a core Socket.io event indicating a connection has been made, but happychat-service isn't actually ready until it emits an init event (which happens after it receives the connect event).
Ideally, I'd imagine we'd want happychat-service to suppress a connect event until it's actually ready for user interaction, rather than emit a connect and then later an init (which IMO is a bit confusing naming). I haven't dug in deep but it looks like HC service needs to authorize the user after connect.
You can see from the previous implementation that the init listener is what resolves the connection.open() promise, so in the previous implementation this is the event that triggered dispatch( setChatConnected() );. That's why I'm using it to emit a connected event here (so that in the middleware interface the listener names make a bit more sense). The dispatch should occur at the same place in the connection lifecycle it did before.
There was a problem hiding this comment.
@jordwest Since this PR is blocking a few others and it's functionally correct I'm going to merge, but I'd love to hear if you have thoughts on --^ and can make changes in another PR if there's a better solution.
jordwest
added
[Status] Ready to Merge
and removed
[Status] Needs Review
Love this idea. It did always seem strange to me that we needed to establish a full connection to Happychat just to determine whether it's available to a user. |
Refactors the Redux actions around Happychat opening a connection.
Previously, when a connection was desired a
HAPPYCHAT_CONNECTINGaction was dispatched. If HC hadn't been connected yet, the middleware would start the connection and the reducers would set the status as'connecting'. If HC had already connected, the middleware would throw the dispatch away, preventing reducers from receiving it.This PR does two things:
1. Creates a new action chain for HC connections
The new action chain for connecting HC looks like this:
HAPPYCHAT_CONNECT: Dispatched when an HC connection is desired. Middleware responds by starting a new connection if one hasn't already been established. The reducers receive this action but don't respond to it.HAPPYCHAT_CONNECTING: Dispatched by middleware when it starts establishing a connection. Reducers respond by setting the connection status to'connecting'.HAPPYCHAT_CONNECTED: Dispatched by middleware when a connection has been established. Reducers respond by setting the connection status to'connected'.It's also worth noting that this PR changes the initial HC connection status from
'disconnected'to'uninitialized'— this mirrors the same thing being done in #12706 and leaves'disconnected'open for upcoming work where we're informing users that a'connected'session actually becomes disconnected.2. Creates a "connection component" to manage the HC connection in components
Currently this just opens a connection if one hasn't already been opened. This component may not be entirely necessary at the moment (since it's now safe to dispatch
HAPPYCHAT_CONNECTafter a connection is made without side effects). But it does provide some opportunity in the future to do more detailed management of HC connections without digging through the whole app. For example, oncomponentWillUnmountwe could send an action that notifies the HC service that the session is not being actively looked at (or something similar).(Props to @southp for noting that stopping the dispatch in middleware felt a bit odd, and @omarjackman for pushing further and suggesting this alternative)
To test
Verify that the actions above are being dispatched through Redux dev tools
Start a session at
/me/chatand then navigate to/help/contactand start a new Happychat session, to verify that there's no side effects from multiple HC-connected components connecting during a browsing session