Fix useSession return value stability
#1230
Conversation
🦋 Changeset detectedLatest commit: 424bc0b 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 📦
|
| /** | ||
| * Given two TokenSourceFetchOptions values, check to see if they are deep equal. | ||
| * | ||
| * FIXME: swap this for an import from livekit-client once | ||
| * https://github.com/livekit/client-sdk-js/pull/1733 is merged and published! | ||
| * */ | ||
| function areTokenSourceFetchOptionsEqual(a: TokenSourceFetchOptions, b: TokenSourceFetchOptions) { | ||
| const allKeysSet = new Set([...Object.keys(a), ...Object.keys(b)]) as Set< | ||
| keyof TokenSourceFetchOptions | ||
| >; | ||
|
|
||
| for (const key of allKeysSet) { | ||
| switch (key) { | ||
| case 'roomName': | ||
| case 'participantName': | ||
| case 'participantIdentity': | ||
| case 'participantMetadata': | ||
| case 'participantAttributes': | ||
| case 'agentName': | ||
| case 'agentMetadata': | ||
| if (a[key] !== b[key]) { | ||
| return false; | ||
| } | ||
| break; | ||
| default: | ||
| // ref: https://stackoverflow.com/a/58009992 | ||
| const exhaustiveCheckedKey: never = key; | ||
| throw new Error(`Options key ${exhaustiveCheckedKey} not being checked for equality!`); | ||
| } | ||
| } | ||
|
|
||
| return true; | ||
| } |
There was a problem hiding this comment.
chore: This has been copied from here: livekit/client-sdk-js#1733. I did this so I could test the change but IMO this copied code shouldn't be merged.
So, before this should be merged, get the above linked pull request deployed into a new version on npm, then delete this code and update it to be an import.
There was a problem hiding this comment.
Oops, I forgot to do this before merging 🤦 . I will do this as part of the follow up change.
| if (!memoizedTokenFetchOptions) { | ||
| throw new Error( | ||
| `AgentSession - memoized token fetch options are not set, but the passed tokenSource was an instance of TokenSourceConfigurable. If you are seeing this please make a new GitHub issue!`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
thought: There might be a better way to word this error message. This should be an invariant that I want to make clear should never happen in actual use but isn't easily representable in another way.
(clarifying a bit further - I don't think this can happen in actual use, but I'm not 100% sure reasoning through it in my head. So if somebody can make this happen down the line I would be curious to get the reproduction instructions!)
| ) { | ||
| const isConfigurable = tokenSource instanceof TokenSourceConfigurable; | ||
|
|
||
| const [memoizedTokenFetchOptions, setMemoizedTokenFetchOptions] = |
There was a problem hiding this comment.
suggestion (if-minor): there's a nice util library that would allow us to use your compare function directly to compare the deps.
Internally it uses a ref for comparison, which I personally feel would be slightly preferable to the useEffect pattern in case you don't want to use that library directly. Main reason is that the useEffect will always only run on the subsequent tick and makes the flow a bit harder to reason about with multiple dependencies
There was a problem hiding this comment.
Sounds good - migrated to a ref instead!
There was a problem hiding this comment.
Ah, that was a misunderstanding, I meant that the ref usage would make things cleaner because it means we can get rid of the useEffect altogether.
If we need to keep the useEffect, then your previous solution works just as well.
FWIW, I think using this approach would allow us to get rid of the useEffect (which was my primary motivation to suggest this change)
… ref instead of state
|
I just tested this and it looks like session itself is still highly unstable due to I think this needs some general refactoring to not wrap the whole return value in a memo but rather the field values returned on it. |
I'm not sure what I could be doing to make this more stable... you are right that I am spreading Taking a step back, I'm not sure what the bar for you is for "highly unstable", but inevitably, the reference will have to change periodically as the session state changes, so it can't just be 1. With the way data is currently being modeled, about 5-6 at minimum must happen during the agent connection process. For reference, here's what I see on my end when I add a Untitled.movEspecially given somebody could further scope down Maybe I'm misunderstanding your objection, but if so let's chat on a call because I'm not really sure what the next steps to get this change merged would look like given your comment, and I think there may be a bit of mounting time pressure. |
These don't really change anything in practice but just out of cleanliness since they aren't used I removed them.
|
Discussed with lukas offline - the concern he was raising was mostly about |
In a comment, @thomasyuill-livekit had mentioned that
useSession's return value seemed to not be stable (link). I looked into it and this was because the options being passed intotokenSource.fetch(...)for configurable token sources were not stable inherintly because of the api being expiosed (they were passed in on each render as a hook parameter) and that was cascading downwards and causing the reference instability.So, to fix this, I've reintroduced the
areTokenSourceFetchOptionsEqualfunction which can be run against each new set of options that are received every hook run and compute a stable value, which in my testing leads to much fewer downstreamsessionreference changes.