Skip to content
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

MatrixRTC: Rename MembershipConfig parameters #4714

Open
wants to merge 17 commits into
base: develop
Choose a base branch
from

Conversation

toger5
Copy link
Contributor

@toger5 toger5 commented Feb 17, 2025

Checklist

  • Tests written for new code (and old code if feasible).
  • New or updated public/exported symbols have accurate TSDoc documentation.
  • Linter and other CI checks pass.
  • Sign-off given on the changes (see CONTRIBUTING.md).

@toger5
Copy link
Contributor Author

toger5 commented Feb 19, 2025

Not sure what the merge order should be here in relation to: #4726 and #4713

@toger5 toger5 requested a review from hughns February 19, 2025 18:07
@hughns hughns changed the title Rename MembershipConfig parameters MatrixRTC: Rename MembershipConfig parameters Feb 21, 2025
@hughns hughns marked this pull request as draft February 28, 2025 14:23
@toger5
Copy link
Contributor Author

toger5 commented Feb 28, 2025

Do we want to make this backwards compatible and introduce a couple of deprecated old properties?
I argue: yes we want to keep the old names for backwards compatibility as deprecated options.

@toger5 toger5 force-pushed the toger5/rename-membership-config branch from db55170 to c569893 Compare March 24, 2025 18:01
@toger5 toger5 changed the base branch from develop to toger5/membership-manager-one-send-delayed-action March 24, 2025 18:02
@toger5 toger5 marked this pull request as ready for review March 24, 2025 18:24
@toger5
Copy link
Contributor Author

toger5 commented Mar 24, 2025

Not sure what the merge order should be here in relation to: #4726 and #4713

This will be merged after: #4747

@toger5
Copy link
Contributor Author

toger5 commented Mar 24, 2025

Can we make sonar cloud happy event though we use the deprecated fields as fallback?

toger5 added 10 commits March 25, 2025 12:14
We do already have the state `hasMemberEvent` that allows to distinguish the two cases. No need to create two dedicated actions.
 - deprecate isJoined (replaced by isActivated)
 - move Interface types to types.ts
@toger5 toger5 force-pushed the toger5/membership-manager-one-send-delayed-action branch from 5acd034 to 052cef9 Compare March 25, 2025 11:14
@toger5 toger5 force-pushed the toger5/rename-membership-config branch from fc35b3c to 3d60f53 Compare March 25, 2025 11:15
Base automatically changed from toger5/membership-manager-one-send-delayed-action to develop March 25, 2025 13:05
@toger5 toger5 removed the X-Blocked label Mar 25, 2025
Comment on lines -90 to -94
/**
* The period (in milliseconds) with which we check that our membership event still exists on the
* server. If it is not found we create it again.
*/
memberEventCheckPeriod?: number;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happened to this one? Should it be deprecated?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was not used anymore (even before this pr it was not used anymore) and I also dont see a reason why we need it. The member event will be observed by listening to state changes. A check period is at best a workaround especially with state_after.

So it should have been deprecated for the last couple of versions and then get completely removed now. (because it was not used in the last versions)

Do you think it is still better to deprecate this and then remove it in the next release?
To me it seemed fine to just drop it now since it had no effect for some time.

@toger5 toger5 requested a review from robintown April 1, 2025 07:53
@toger5
Copy link
Contributor Author

toger5 commented Apr 1, 2025

I am wondering if this should get a breaking change label because of the one property robin mentioned got removed. Technically it does not change anything however since it will continue to be a property with no effect but one can keep it in the config file.

Does it make more sense to add the breaking change label to the version that then removes the deprecated fields?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants