fix(react): keep chat stable with inline options - #1450
Conversation
🦋 Changeset detectedLatest commit: 8e2d0c4 The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 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 |
|
@fatihcvs is attempting to deploy a commit to the LiveKit Team on Vercel. A member of the Team first needs to authorize it. |
1egoman
left a comment
1egoman
left a comment
There was a problem hiding this comment.
Generally looks good to me! One small suggestion to make some docstring comment updates.
| export function useChat(options?: ChatOptions & { room?: Room }) { | ||
| const room = useEnsureRoom(options?.room); | ||
| const { channelTopic, updateChannelTopic, messageEncoder, messageDecoder } = options ?? {}; |
There was a problem hiding this comment.
Both messageEncoder and messageDecoder here are functions. It's probably fairly likely somebody would think they could do something like this:
useChat({
room,
messageDecoder: (payload) => JSON.parse(new TextDecoder().decode(payload)),
});If they did, then they run into the same issue where there needs to be memoization done to the function with useCallback.
Given both of these are deprecated, I think it's probably fine to just add a docstring comment on each in ChatOptions which calls out that they need to be memoized. Would you be able to do this?
(The other option which I think could work would be something like on every render, storing messageEncoder and messageDecoder into refs, and then using these refs imperatively everywhere instead of the prop values. However, this is a lot of extra infrastructure to add and since the params are deprecated I think it makes more sense to keep things stable and not change behavior for existing downstream users.)
There was a problem hiding this comment.
Added the note to both ChatOptions callbacks in 8e2d0c4: memoize with useCallback or define them outside the component, with an explanation of the history reset/render-loop risk. Kept the existing behavior as suggested. Prettier and git diff --check pass.
1egoman
left a comment
1egoman
left a comment
There was a problem hiding this comment.
Thanks for the contribution!
|
You're welcome! Thanks for the review and the suggestion. |
2 participants
Passing inline options such as
useChat({ room })recreates chat setup on every render. Resetting the message observable then updates state with a new empty array, triggering another render and setup. Equivalent options can therefore cause a render loop and discard message history.Depend on the individual option values rather than the containing object, while retaining the existing room/disconnect reset behavior.
Validation:
Roomevents in JSDOM, with no server connection or hook mocks.pnpm build:react, React lint (warnings, no errors),pnpm format:check, and React API check pass.Includes a React patch changeset. This is independent of #1449: it changes React option identity handling, not core per-topic registration. Developed and verified with OpenAI Codex assistance.