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

Support MSC4140: Delayed events (Futures) #4294

Merged
merged 1 commit into from
Jul 30, 2024
Merged

Support MSC4140: Delayed events (Futures) #4294

merged 1 commit into from
Jul 30, 2024

Conversation

AndrewFerr
Copy link
Member

@AndrewFerr AndrewFerr commented Jun 28, 2024

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

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).

Copy link
Contributor

@toger5 toger5 left a comment

Choose a reason for hiding this comment

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

I left a couple of comments.
This looks good already however.

src/@types/requests.ts Outdated Show resolved Hide resolved
src/client.ts Outdated Show resolved Hide resolved
src/client.ts Outdated Show resolved Hide resolved
src/client.ts Outdated Show resolved Hide resolved
src/embedded.ts Outdated Show resolved Hide resolved
src/matrixrtc/MatrixRTCSession.ts Outdated Show resolved Hide resolved
src/client.ts Outdated Show resolved Hide resolved
src/@types/requests.ts Outdated Show resolved Hide resolved
src/client.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@toger5 toger5 left a comment

Choose a reason for hiding this comment

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

Thanks for also adding the tests.
Looks good to me now.

@AndrewFerr
Copy link
Member Author

I finally got the code coverage of MatrixRTCSession.ts that I was after, so barring further review, I'm done with pushing more commits here.

Copy link
Contributor

@toger5 toger5 left a comment

Choose a reason for hiding this comment

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

This looks very good thanks.
I think, i cannot follow why delayed events are entangled with threads (is it just because of the method overloading?

Reading through this code again I am very surpriced that this does not use a props interface param for the function paramters

myOverloadedFunction(props: {delay?: number, threadId?: string, eventType: EventType ...})

spec/unit/matrix-client.spec.ts Show resolved Hide resolved
src/@types/requests.ts Outdated Show resolved Hide resolved
src/client.ts Show resolved Hide resolved
@AndrewFerr
Copy link
Member Author

Since this PR is already reviewed, I'll squash & rebase its commits before merging, because

  1. all of the sub-commits are too granular to keep in history
  2. the latest develop fixes one of the tests that are failing on this PR right now

and use them for more reliable MatrixRTC session membership events.

Also implement "parent" delayed events, which were in a previous version
of the MSC and may be reintroduced or be part of a new MSC later.

NOTE: Still missing is support for sending encrypted delayed events.
@AndrewFerr AndrewFerr added this pull request to the merge queue Jul 30, 2024
Merged via the queue into develop with commit 687d08d Jul 30, 2024
26 checks passed
@AndrewFerr AndrewFerr deleted the af/msc4140 branch July 30, 2024 13:01
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.

3 participants