Add new useRoomConnection hook for managing room connections
#1197
Conversation
It seems initially like modelling a room connection with a `useEffect(()
=> { room.connect(); return () => room.disconnect(); }, [...])` would be
natural and work well, however if dependencies in that `useEffect` change
rapidly, then the room can thrash back and forth between connected and
disconnected, and because room.disconnect() is async, sometimes
resulting in connects and disconnects happening at the same time.
There's some thinking being done to address this at the client-sdk-js
level but for now, introduce a hook folks can use to at least stem the
bleeding a bit. Once the issue is fixed in `client-sdk-js` then this
hook can be updated to just use a `useEffect` again.
size-limit report 📦
|
|
| try { | ||
| await Promise.all([ | ||
| room.localParticipant.setMicrophoneEnabled( | ||
| true, |
There was a problem hiding this comment.
this limits the usage of this hook to scenarios where users connect to rooms with their mic (and only their mic) enabled 🤔
There was a problem hiding this comment.
Yea, you are correct, but this doesn't seem bad to me / was intentional.
I suppose maybe it could be nice to be able to configure whether this setMicrophoneEnabled runs here or not, to enable / disable the audio buffering behavior before connection? But besides that, is there a scenario where a user would want to connect to a room with something else like video enabled before room.connect(...) runs?
| connectDisconnectLock.lock().then(async (unlock) => { | ||
| if (connected) { | ||
| setStatus('connecting'); | ||
| const connectionDetails = await options.getConnectionDetails(); |
There was a problem hiding this comment.
if we employ this pattern we have to think about what happens if retrieving connection details here fails. E.g. is a user expected to flip connected to false and back to true on an error?
Also wondering if decoupling the two (connection and connectionDetails) makes more sense, as it can speed up the actual connection if connection Details retrieval happens in advance (e.g. like prefetch before you actually click)
| }); | ||
| }, [connected]); | ||
|
|
||
| return { room, status }; |
There was a problem hiding this comment.
returning the room here might lead to patterns that we don't really want to encourage, i.e. trying to call methods on the room itself again.
There was a problem hiding this comment.
Ah, I had figured that was kinda the whole point, I'm not too well versed with this package yet. It sounds like the intention maybe is that the room would always be in the RoomContext and that is how other operations should be performed on it, including imperative updates?
|
Digging around, I actually discovered something that already exists which pretty closely matches what I think I was aiming to create here: https://github.com/livekit/components-js/blob/main/packages/react/src/hooks/useLiveKitRoom.ts and also https://github.com/livekit/components-js/blob/main/packages/react/src/components/LiveKitRoom.tsx. It's probably on me for missing this, it's pretty clearly called out in the docs! I think given this discovery, I'm going to close this pull request and migrate the connection behavior from here into this pre-existing |
…nnect in series Context around why this is important: #1197
…nnect in series Context around why this is important: #1197
It seems initially like modeling a room connection with a
useEffect(() => { room.connect(); return () => room.disconnect(); }, [...])would be natural and work well, however if dependencies in thatuseEffectchange rapidly, then the room can thrash back and forth between connected and disconnected. Becauseroom.disconnect()is async, this sometimes results in connects and disconnects happening at the same time, which can cause edge cases in the SDK, redulting in sometimes very weird errors due to state from past connection attempts being considered as current.There's some thinking being done to address this at the client-sdk-js level but for now, introduce a hook folks can use to at least stem the bleeding in the short term a bit. Once the issue is fixed in
client-sdk-jsthen this hook can be updated to just use auseEffectagain.Note that this hook inherently must make a tradeoff - the existing
room.connect()/room.disconnect()logic only wraps part of each operation in a mutex, meaning that operations are supposed to be able to overlap to try to keep performance up at the expense of reliability. This hook takes the other side of that tradeoff, resulting in slower but much more predictable behavior ifconnect()s anddisconnect()s occur rapidly.Usage example: livekit-examples/agent-starter-react#225