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 getRoomMessages API #265

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Half-Shot
Copy link
Contributor

@Half-Shot Half-Shot commented Oct 6, 2022

Fixes #250

Checklist

  • Tests written for all new code
  • Linter has been satisfied
  • Sign-off given on the changes (see CONTRIBUTING.md)

Signed off by: Will Hunt [email protected]

* @param from The token to start returning events from.
* This token can be obtained from a prev_batch or next_batch token returned by the /sync endpoint,
* or from an end token returned by a previous call to this function.
* @param filter A JSON RoomEventFilter to filter returned events with.
Copy link
Owner

Choose a reason for hiding this comment

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

We shouldn't be forcing the caller to encode their filter for us

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes, that would be silly.

@@ -17,6 +17,48 @@ export interface TypicalUnsigned {
[prop: string]: any;
}

export interface RoomEventData<T extends (Object | unknown) = unknown> {
Copy link
Owner

Choose a reason for hiding this comment

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

The SDK isn't really set up to have these interfaces exist: events are either any (deliberately) or an instance of MatrixEvent - neither of which rely on the interfaces existing. Eventually the SDK will publish a breaking change that actually uses MatrixEvent instances everywhere, however until then it's expected that new code does the conversion. See getEventContext for an example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll update the code to use MatrixEvent for now then, to make that transition easier.

* This token can be obtained from a `prev_batch` or `next_batch` token returned by the /sync endpoint,
* or from an end token returned by a previous call to this function.
*/
public async getRoomMessages(roomId: string, dir: 'f'|'b', from: string, opts: {
Copy link
Owner

Choose a reason for hiding this comment

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

We can probably assume that b is going to be the default and set it accordingly.

Copy link
Owner

Choose a reason for hiding this comment

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

The function name should also be getRoomHistory or getRoomEvents or similar - we don't have to match the endpoint name, particularly when it's confusing to consumers.

*
* It uses pagination query parameters to paginate history in the room.
*
* @param roomId the room ID to get the event in
Copy link
Owner

Choose a reason for hiding this comment

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

wrong docs?

/**
* This returns a list of message and state events for a room.
*
* It uses pagination query parameters to paginate history in the room.
Copy link
Owner

Choose a reason for hiding this comment

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

yes, that's how pagination works :p

Possibly something like this for the overall description?

Retrieve a (paginated) chunk of history from the room. The direction in which the server is
asked to find events in can be changed, with b (backwards) being "find events older than
the provided token".

We really shouldn't be copy/pasting from the spec, particularly when it doesn't make sense.

import { RoomEventData, RoomStateEventData } from "./events/RoomEvent";

/**
* The options available when creating a room.
Copy link
Owner

Choose a reason for hiding this comment

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

Not the right docs. However, given the translation we'll need to do on the returned events anyways this interface can be deleted.

@turt2live
Copy link
Owner

@Half-Shot is this still on your radar?

@AndrewKvalheim AndrewKvalheim mentioned this pull request May 2, 2023
3 tasks
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.

Add support for fetching room messages
2 participants