Update examples with useSession/useAgent hooks #1242
Conversation
🦋 Changeset detectedLatest commit: f35d5a1 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 |
size-limit report 📦
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| console.error('Failed to end session:', err); | ||
| }); | ||
| } | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps |
There was a problem hiding this comment.
Also a thought:, but maybe it could make sense to destructure the session to get those start / end values out? I think you had said a few weeks back that does silence the warning?
| ); | ||
| }; | ||
|
|
||
| session.internal.emitter.on(SessionEvent.MediaDevicesError, handleMediaDevicesError); |
There was a problem hiding this comment.
thought: @1egoman maybe we should think about a cleaner high level helper for this.
using the internal emitters in the examples doesn't feel great.
There was a problem hiding this comment.
There was a better way to do this proposed, useEvents, though it didn't get much discussion because I figured it was fairly non controversial - https://github.com/livekit/components-js/blob/main/packages/react/src/hooks/useEvents.ts#L5.
So the thinking is the above could be something like the below instead:
useEvents(session, SessionEvent.MediaDevicesError, handleMediaDevicesError);| roomName, | ||
| participantIdentity: userIdentity, | ||
| participantName: userIdentity, | ||
| room, |
There was a problem hiding this comment.
question: @1egoman is there anything ensuring that session is still "in charge" of the room connection cycle when users use this pattern?
There was a problem hiding this comment.
Not currently, no. It's an "escape hatch" you can use to provide a custom room and if the caller modifies the room's state extensively out of band it will definitely cause things to break. This definitely needs to be documented better than it currently is.
| ); | ||
| } | ||
|
|
||
| const VoiceAssistantExample: NextPage = () => { |
There was a problem hiding this comment.
nit: would be nice to rename this to agent example both in the method names and file
| const tokenSource = useMemo(() => { | ||
| return TokenSource.endpoint(process.env.NEXT_PUBLIC_LK_TOKEN_ENDPOINT!); | ||
| }, []); |
There was a problem hiding this comment.
suggestion: I think if at all possible, the examples (where it can be done) should define the TokenSource outside the component as a constant rather than in the component in a memoized fashion.
Making sure that the TokenSource doesn't change reference proved to be a challenge when Thom was porting over agent-starter-react to use these new apis, and I'd like to try to push people away from injecting local component state into TokenSources (configurable token sources should include all the metadata somebody would need in TokenSource.custom cases) unless you are doing something truly weird like agent-starter-react does to handle sandbox environments properly.
| const [room] = useState(new Room()); | ||
|
|
||
| const tokenSource = useMemo(() => { | ||
| return TokenSource.endpoint(process.env.NEXT_PUBLIC_LK_TOKEN_ENDPOINT!); | ||
| }, []); | ||
|
|
||
| const session = useSession(tokenSource, { | ||
| roomName, | ||
| participantIdentity: userIdentity, | ||
| participantName: userIdentity, | ||
| room, | ||
| }); |
There was a problem hiding this comment.
suggestion: I think you can get rid of the room parameter here since it is just new Room()?
|
Thank you for the feedback! I'll probably tackle that early next week to bring it to production standard. |
Also, a little change to
useSessionfor the camera track...