-
-
Notifications
You must be signed in to change notification settings - Fork 590
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow configuration of MatrixRTC timers when calling joinRoomSession() #4510
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks very good. Thanks.
Not sure what you think about the naming preference I mentioned in the review.
Merging as it is now is also fine to me.
* The timeout (in milliseconds) after which the server will consider the membership to have expired if it | ||
* has not received a keep-alive from the client. | ||
*/ | ||
membershipServerSideExpiryTimeout?: number; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think it is beneficial to use sth close to the delay
event terminology since this is what one would look out for to configure the even delay timeout?
/** | ||
* The period (in milliseconds) that the client will send membership keep-alives to the server. | ||
*/ | ||
membershipKeepAlivePeriod?: number; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here something with refresh
My thinking on the naming is that we choose things that make sense with respect to MatrixRTC concepts. But, as you say it can be argued out and refactored later if needed. |
Not all of the new config options are being tested, but the code coverage is still reached.
The use of
Math.random()
that is flagged by SonarCloud is acceptable.Checklist
public
/exported
symbols have accurate TSDoc documentation.