Update examples with useSession/useAgent hooks #1242
Conversation
🦋 Changeset detectedLatest commit: 1383d77 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?
examples/nextjs/pages/clubhouse.tsx
Outdated
| ); | ||
| }; | ||
|
|
||
| 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);There was a problem hiding this comment.
Migrated all instances of session.internal (which all happened to be even listener related) over to use this useEvents pattern.
| 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.
examples/nextjs/pages/audio-only.tsx
Outdated
| 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.
|
Thank you for the feedback! I'll probably tackle that early next week to bring it to production standard. |
|
@pblazej FYI I applied all the changes lukas and I suggested, so I think this should be good to merge. |
|
@1egoman thanks for handling that! |
Also, a little change to
useSessionfor the camera track...