Reconnect on network change #105
Conversation
| if (!client) return; | ||
|
|
||
| const onReconnectionStarted: ClientEvents<PeerMetadata, TrackMetadata>["reconnectionStarted"] = () => { | ||
| console.log("%c" + "reconnectionStarted", "color:green"); |
There was a problem hiding this comment.
👀 niiice, I didn't know you can have colors in logs 🌈
| @@ -69,6 +71,15 @@ export interface ClientEvents<PeerMetadata, TrackMetadata> { | |||
| /** Emitted when the connection is closed */ | |||
| disconnected: (client: ClientApi<PeerMetadata, TrackMetadata>) => void; | |||
|
|
|||
| /** Emitted when on successful reconnection */ | |||
| reconnected: (client: ClientApi<PeerMetadata, TrackMetadata>) => void; | |||
There was a problem hiding this comment.
Is this the user-facing API? I'd imagine in react we'd have a hook like useReconnection, something like this, feel free to change this is only an example:
const { isReconnecting, reconnectionFailed } = useReconnection();
So that user can display a nice notification that the app is currently trying to reconnect or reconnection failed. Imo it's more react-like than setting listeners.
Also google meet also tells me that the internet connection is unstable - maybe it's worth adding later?
There was a problem hiding this comment.
Exposing events allows me to simplify a lot of logic in Videoroom; I removed a few useEffect hooks because of that.
And you are right that it's not the React way, so I've introduced the useReconnection hook with these fields if someone needs only the current state. So, events for state changes and a hook for the current state.
export type UseReconnection = {
status: ReconnectionStatus;
isReconnecting: boolean;
isError: boolean;
isIdle: boolean;
};
src/create.tsx
Outdated
| if ( | ||
| client.status === "joined" && | ||
| event.mediaDeviceType === "userMedia" && | ||
| !pending && | ||
| !client.isReconnecting() | ||
| ) { |
There was a problem hiding this comment.
This if is long and it's repeated and I don't know what exatly it means in general, maybe refactor it into a function or something?
There was a problem hiding this comment.
I've extracted this logic to separate functions isBroadcastedTrackChanged and isBroadcastedTrackStopped, refactored other fragments in the useSetupMedia hook, and extracted this hook to a separate file. I think the code could be simplified, but I don't want to do it right now.
| @@ -61,6 +62,7 @@ const autostartAtom = atomWithStorage<boolean>("autostart", false, undefined, { | |||
|
|
|||
| export const MainControls = () => { | |||
| const [token, setToken] = useAtom(tokenAtom); | |||
| useReconnectLogs(); | |||
There was a problem hiding this comment.
this example is weird, is it a testing app or an example for the user? Because users generally don't need to log reconnection attempts into the console...
There was a problem hiding this comment.
It's a testing app, but unfortunately it's located in the example directory. I've added a // for debugging comment to indicate that it's not necessary because I don't want to spend more time creating a whole new testing app.
| status: ReconnectionStatus; | ||
| isReconnecting: boolean; | ||
| isError: boolean; | ||
| isIdle: boolean; |
There was a problem hiding this comment.
Why do we need isReconnecting, isError etc. if user can just:
if(status == 'reconnecting')
?
There was a problem hiding this comment.
Just for convenience
| status === "joined" && | ||
| event.mediaDeviceType === expectedMediaDeviceType && | ||
| event.trackType === expectedTrackType && | ||
| stream; |
There was a problem hiding this comment.
So this checks if track is stopped? 👀 I don't see anything related to stopping here
There was a problem hiding this comment.
I think we could implement it later
Description
fishjam-dev/ts-client-sdk#53