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

Add MatrixClient.doesServerSupportLogoutDevices() for MSC2457 #2297

Merged
merged 3 commits into from Apr 15, 2022

Conversation

hughns
Copy link
Member

@hughns hughns commented Apr 13, 2022


Here's what your changelog entry will look like:

✨ Features

  • Add MatrixClient.doesServerSupportLogoutDevices() for MSC2457 (#2297). Contributed by @hughns.

@hughns hughns added the T-Enhancement New features, changes in functionality, performance boosts, user-facing improvements label Apr 13, 2022
@hughns hughns marked this pull request as ready for review April 13, 2022 16:34
@hughns hughns requested a review from a team as a code owner April 13, 2022 16:34
src/client.ts Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Apr 13, 2022

Codecov Report

Merging #2297 (9a89b42) into develop (54661cc) will increase coverage by 0.01%.
The diff coverage is 57.14%.

@@             Coverage Diff             @@
##           develop    #2297      +/-   ##
===========================================
+ Coverage    59.71%   59.73%   +0.01%     
===========================================
  Files           91       91              
  Lines        16439    16442       +3     
  Branches      3794     3796       +2     
===========================================
+ Hits          9817     9822       +5     
+ Misses        6622     6620       -2     
Impacted Files Coverage Δ
src/models/room.ts 57.34% <50.00%> (ø)
src/client.ts 38.07% <60.00%> (+0.03%) ⬆️
src/models/thread.ts 76.25% <0.00%> (+2.15%) ⬆️

@turt2live
Copy link
Member

out of interest, why are we adding more of these doesServerSupportX functions for things that are released? We should be able to do it inline at the code site rather than have a dedicated function.

@t3chguy
Copy link
Member

t3chguy commented Apr 13, 2022

Multiple places call it, where you put the utility function might as well be js-sdk for re-use

Copy link
Member

@t3chguy t3chguy left a comment

Choose a reason for hiding this comment

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

:shipit:

@turt2live
Copy link
Member

it's quite questionable as a utility function even still, honestly. These functions make sense for unstable features, not for released things.

The check is also a bit awkward because it doesn't account for the fact that a server doesn't need to implement r0.6.1 but can implement v1.1 or v1.2. This isn't a problem unique to this change though, so won't require it being fixed here.

@t3chguy
Copy link
Member

t3chguy commented Apr 13, 2022

The check is also a bit awkward because it doesn't account for the fact that a server doesn't need to implement r0.6.1 but can implement v1.1 or v1.2. This isn't a problem unique to this change though, so won't require it being fixed here.

Agreed, even more reason for them to happen in js-sdk where they can be fixed holistically.

Though would be nice if they could be extracted from the MatrixClient into a separate Features class or similar to make this class less huge

@hughns
Copy link
Member Author

hughns commented Apr 13, 2022

Multiple places call it, where you put the utility function might as well be js-sdk for re-use

Exactly + ...

What I actually wanted to check is whether version >=r0.6.1 is supported. So that if a future server removed support for r0.6.1 but continued to support v1.1 then the function should still return true.

However, I didn't find such existing logic and so decided it would be better to leave the "flawed" version checking logic alongside the other "flawed" logic rather than strewn across the codebase (and without bloating my oneliner PR).

@turt2live
Copy link
Member

I would still argue that a purpose-built doesServerSupportX function is well over the top for the intended case here :p

@hughns
Copy link
Member Author

hughns commented Apr 13, 2022

Haha. Like great minds adrift in the night.

@hughns hughns merged commit 9f45986 into develop Apr 15, 2022
@hughns hughns deleted the hughns/logout-devices-control branch April 15, 2022 09:27
su-ex added a commit to SchildiChat/matrix-js-sdk that referenced this pull request Apr 30, 2022
* Add MatrixClient.doesServerSupportLogoutDevices() for MSC2457 ([\matrix-org#2297](matrix-org#2297)).
* Live location sharing - expose room liveBeaconIds ([\matrix-org#2296](matrix-org#2296)).
* Support for MSC2457 logout_devices param for setPassword() ([\matrix-org#2285](matrix-org#2285)).
* Stabilise token authenticated registration support ([\matrix-org#2181](matrix-org#2181)). Contributed by @govynnus.
* Live location sharing - Aggregate beacon locations on beacons ([\matrix-org#2268](matrix-org#2268)).
* Prevent duplicated re-emitter setups in event-mapper ([\matrix-org#2293](matrix-org#2293)).
* Make self membership less prone to races ([\matrix-org#2277](matrix-org#2277)). Fixes element-hq/element-web#21661.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-Enhancement New features, changes in functionality, performance boosts, user-facing improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants