-
Notifications
You must be signed in to change notification settings - Fork 79
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix useRemoteParticipant
re-rendering on participant events
#891
Conversation
🦋 Changeset detectedLatest commit: ae6bd2c The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for flagging this.
It's very unfortunate that this is needed, but I cannot think of a better workaround, so I think your changes look good.
Only a couple of nits.
@@ -2,7 +2,7 @@ import { connectedParticipantObserver } from '@livekit/components-core'; | |||
import type { ParticipantEvent, RemoteParticipant } from 'livekit-client'; | |||
import * as React from 'react'; | |||
import { useRoomContext } from '../context'; | |||
import { useObservableState } from './internal'; | |||
// import { useObservableState } from './internal'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you delete this import?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
); | ||
return participant; | ||
|
||
const [participantWrapper, setParticipantWrapper] = React.useState({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a comment outlining why this is needed? It'll make it easier to understand in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, is it understandable?
Looks good, thanks! |
Hi,
This PR fixes the problem of not re-rendering in
useRemoteParticipant
for triggered participant events.Currently, this hook uses
useObservableState
, which itself uses auseState
to execute a re-render after each emit value in theobservable
. But we know that the reference of participant object does not change when new events are fired, and thus re-rendering does not occur. So, to solve this problem, we need to put the participant in a wrapper object to useuseState
.P.S:
Maybe a cleaner coding to solve this problem:
(although it requires the 'map' operator from rxjs)