rtc: Cache RTC discovery in the MatrixClient - #5470
Conversation
| /** | ||
| * Defines a generic mechanism to fetch and cache values for the client. | ||
| */ | ||
| export abstract class ClientCachedValue<ValueType> { |
There was a problem hiding this comment.
This seems like more than just caching, it's actively re-fetching on a timer.
Also, I think we are trying to move away from inheritance as a way of getting functionality which I think is considered a bit of an anti-pattern in favour of aggregation, ie. instantiating an object that does the thing.
There was a problem hiding this comment.
True, I renamed to ClientPollingCachedValue, it is better but naming is hard.
I also did the refactoring to avoid inheritence, by having a concrete class doing the mecanism and having a factory with configuration option.
This is easier to review commit by commit I think, I am adding the commits for convinience:
6844353 Do the renaming
e08796b The factory refactoring
| * @param TTL The time-to-live in milliseconds. | ||
| */ | ||
| public setTTLMillis(TTL: number): void { | ||
| this.ttlMillis = TTL; |
There was a problem hiding this comment.
wouldn't we expect this to update the timeout?
There was a problem hiding this comment.
That's true, but given this was always set once and before the start, I moved it as a parameter of the start to avoid this caveat 0e6487c
| return false; | ||
| } | ||
|
|
||
| private async doFetch(force: boolean = false): Promise<void> { |
There was a problem hiding this comment.
docs please, it's not clear what 'force' means here
| this.fetchPromise = this.fetchWithRetryPolicy(); | ||
|
|
||
| try { | ||
| const value = await this.fetchPromise; |
There was a problem hiding this comment.
why use the value variable rather than just assigning to this.cached?
| if (this.ttlMillis) { | ||
| this.ttlTimeoutHandle = setTimeout(() => { | ||
| this.fetchPromise = undefined; | ||
| void this.doFetch(); |
There was a problem hiding this comment.
This in particular feels like a weird hybrid between periodic updating where you would probably try to refresh the value in advance of it expiring and a cache that expires but is then just repopulated the next time it's needed.
There was a problem hiding this comment.
Agree, I tried to keep it mostly to accomodate the pre-existing well-known behavior that was doing that, so I can refactor both value to use the new mecanism
| try { | ||
| await this.doFetch(); | ||
| } catch (error) { | ||
| // Ignore errors, as wait should not throw |
There was a problem hiding this comment.
shouldn't it? what if the value can't be fetched? This feels like something for the function's tsdoc.
dbkr
left a comment
dbkr
left a comment
There was a problem hiding this comment.
Thanks, yeah - it's the awkward situation of two things that share very similar logic but not the same.
|
The upstream fails because of a test I fixed in this PR https://github.com/element-hq/element-web/pull/34581/changes The test was accessing a non-public API? Replaced by a spyOn FWIW created an upstream PR resolve the problem element-hq/element-web#34618 |
2 participants
Allow the matrix-js-sdk to cache the
rtc/transportdiscovery result as it is doing with similar other server side calls (well-known or capabilities)This would allow the widget driver (in Element-web) to use this cached value instead of fetching every time element-hq/element-web#34393
EW has added it's own caching in CallStore, but I think this should be done in the js-sdk.
This is best reviewed commit by commit?
Checklist
public/exportedsymbols have accurate TSDoc documentation.