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

fix(Room): Use subscribe_reset for subscribe_info (instead of just subscribe) #4276

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

toger5
Copy link
Contributor

@toger5 toger5 commented Nov 18, 2024

When calling subscribe_info users will most likely expect to get the info and not to wait for the next info change.

Has been discussed with @bnjbvr.

  • Public API changes documented in changelogs (optional)

Signed-off-by:

@toger5 toger5 requested a review from bnjbvr November 18, 2024 09:41
@toger5 toger5 changed the base branch from toger5/mock-matrix-server-widget-tests to main November 18, 2024 09:42
@toger5 toger5 force-pushed the toger5/subscribe-reset-for-room-info branch from 6514003 to 0fec0af Compare November 18, 2024 09:44
@toger5 toger5 changed the title Toger5/subscribe-reset-for-room-info fix(Room): Use subscribe_reset for subscribe_info (instead of just subscribe) Nov 18, 2024
@toger5 toger5 changed the title fix(Room): Use subscribe_reset for subscribe_info (instead of just subscribe) fix(Room): Use subscribe_reset for subscribe_info (instead of just subscribe) Nov 18, 2024
@bnjbvr bnjbvr requested review from a team and removed request for bnjbvr and a team November 18, 2024 09:49
Copy link
Member

@bnjbvr bnjbvr left a comment

Choose a reason for hiding this comment

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

OK on principle, but this is a draft and not passing tests, so this will require a bit more work.

@toger5
Copy link
Contributor Author

toger5 commented Nov 18, 2024

Because this is an intentionally breaking change, we just adapt the tests right?

@bnjbvr
Copy link
Member

bnjbvr commented Nov 18, 2024

Yes! Would need to make it super explicit that it's a breaking change, please have a look at our contribution guidelines :)

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

Successfully merging this pull request may close these issues.

2 participants