-
Notifications
You must be signed in to change notification settings - Fork 10
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
Move SSE to Matrix #2116
Move SSE to Matrix #2116
Conversation
Host Test Results 1 files ±0 1 suites ±0 25m 8s ⏱️ +6s Results for commit dfab792. ± Comparison against base commit 160dab0. This pull request removes 1 and adds 1 tests. Note that renamed tests count towards both.
♻️ This comment has been updated with latest results. |
2288094
to
b2d57db
Compare
b2d57db
to
7cd9d2c
Compare
The adapter seems like the place but some interface changes will be needed to make it work cross-environment.
This probably needs to move into another hook…? Or should test realm setup hooks error if it hasn’t been called?
It’s probably at the point that I should stop changing tests to add this setup and make it built-in…???
This was causing the count to be off, the original didn’t have an assertion, just an error.
I hate it??? But what is to be done.
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.
I don't have time to try this out right now, but the code looks good. Very excited to see the test improvements land!
# Conflicts: # packages/host/tests/acceptance/code-submode/schema-editor-test.ts
I don’t understand why this is happening 😞
1c305e4
to
90144f7
Compare
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.
ditto with what luke said. overall looks great, but too large for me to get deep into it.
# Conflicts: # packages/host/app/resources/room.ts # packages/host/app/services/matrix-service.ts
Why???
This repurposes existing Matrix “session rooms” (where the auth conversation is conducted) to also broadcast “realm events”, which were formerly transmitted via Server-Sent Events. It includes several TODOs to completely remove the traces of SSE, including:
service:message-service
no longer needs to exist$REALM/…_message
endpoint for subscribing can be removedIt’s unfortunately a large PR but there were no obvious seams to further decompose it at. Thankfully most of the changes are mechanical, replicated across many files.
The high-level changes to
host
tests are:setupServerEvents
and theTestContextWithSSE
that provided itexpectEvents
, no more callbacksettled
andwaitUntil
to ensure realm events have propagatedsetupMockMatrix
when missing, theMockUtils
it returns is required for realm setup nowOtherwise the changes include:
expectEvent
helper in the realm server’srealm-endpoints-test