matrix-org / matrix-js-sdk

Matrix Client-Server SDK for JavaScript
Apache License 2.0
1.49k stars 578 forks source link

Improve compliance with MSC3266 #4155

Closed AndrewFerr closed 2 months ago

AndrewFerr commented 2 months ago

This fixes some non-compliance with MSC 3266, and exports the payload type.

Signed-off-by: Andrew Ferrazzutti andrewf@element.io

Checklist

t3chguy commented 2 months ago

Please improve the PR title given it will be used in the changelog verbatim

AndrewFerr commented 2 months ago

Please improve the PR title given it will be used in the changelog verbatim

Done -- let me know if the title still needs changing.

I am also willing to add an integration test for this.

t3chguy commented 2 months ago

Yeah looks like CI won't let it in without testing

AndrewFerr commented 2 months ago

Good thing CI is strict, because this PR requires a Synapse change too: https://github.com/element-hq/synapse/pull/17078

t3chguy commented 2 months ago

So we'll likely need fallback code then for older Synapse support

AndrewFerr commented 2 months ago

So we'll likely need fallback code then for older Synapse support

The Synapse PR has a fallback: it responds to both the old & new endpoint used by getRoomSummary.

Then again, getRoomSummary could also have a fallback: if it gets a 404 response for the new endpoint, it can retry at the old endpoint. I'll add that now.

AndrewFerr commented 2 months ago

Hm, even with the integration test, the code analysis check is failing. What needs to be added to fix that?

Other than that, bd09442 adds the fallback, so once the check is fixed, this should PR be ready.

t3chguy commented 2 months ago

Hm, even with the integration test, the code analysis check is failing

It says you reached 33.3% coverage, out of the required 80%.

image

Things marked red are untested.

AndrewFerr commented 2 months ago

Alright, code coverage for new code is now at 100%, so this should be done.

AndrewFerr commented 2 months ago

Do I need to rebase this PR branch before it can be merged?