Skip to content

Fix getLatestTimeline not working when the latest event in the room is a threaded message #2521

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.

Sign up for GitHub

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

Closed
MadLittleMods wants to merge 12 commits into from
Closed

Fix getLatestTimeline not working when the latest event in the room is a threaded message #2521

Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
5f4d10d
Fix getLatestTimeline not working when the latest even in the room wa…
MadLittleMods Jul 13, 2022
e8dc590
Adjust tests to not include problem events instead of undefined timeline
MadLittleMods Jul 13, 2022
9882eb6
Add tests for not being able to add mixed events
MadLittleMods Jul 13, 2022
fdeb9e0
Correct JSDoc and types for getEventTimeline
MadLittleMods Jul 13, 2022
78f8019
No longer need check since it should always return a timeline
MadLittleMods Jul 13, 2022
50b5bb1
Add getLatestTimeline test for latest event being thread
MadLittleMods Jul 13, 2022
a3457e1
Add non-optional client to Room instances
MadLittleMods Jul 13, 2022
cdef5cb
Fix lint
MadLittleMods Jul 13, 2022
cd41d7e
We expect void so return void
MadLittleMods Jul 19, 2022
a3ea213
Merge branch 'develop' into madlittlemods/refresh-timeline-when-we-se…
MadLittleMods Aug 29, 2022
b97de19
Merge branch 'develop' into madlittlemods/refresh-timeline-when-we-se…
MadLittleMods Oct 14, 2022
f82ea0c
Fix some lints
MadLittleMods Oct 14, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Correct JSDoc and types for getEventTimeline
  • Loading branch information
@MadLittleMods
MadLittleMods committed Jul 13, 2022
commit fdeb9e0df751efda307e92990e0364a75c720e7c
4 changes: 2 additions & 2 deletions src/client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5234,15 +5234,15 @@ export class MatrixClient extends TypedEventEmitter<EmittedEvents, ClientEventHa
* <p>If the EventTimelineSet object already has the given event in its store, the
* corresponding timeline will be returned. Otherwise, a /context request is
* made, and used to construct an EventTimeline.
* If the event does not belong to this EventTimelineSet then undefined will be returned.
* If the event does not belong to this EventTimelineSet then it will ignored.
Copy link
Member

@t3chguy t3chguy Jul 13, 2022

Choose a reason for hiding this comment

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

The event will be ignored? What does that even mean? This method isn't meant to process the given event, just use it as a pointer. What event timeline will be returned to the caller? The caller would now have need to manually check that the returned timeline is valid for what they asked for, I guess by your other change lower down by attempting to add an event and by asserting that it worked, that seems rather strange

Copy link
Contributor Author

@MadLittleMods MadLittleMods Jul 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Let's first discuss the previous behavior. Previously, when the event didn't belong to the timelineSet, we would return undefined and create no additional timelines. Returning undefined has no special meaning given that all of the usage is always fire and forget meaning we don't use the timeline returned by client.getEventTimeline() and only use the function to load the event so it's available to the client.

In our usage, there is only a single spot where the caller uses the timeline returned by client.getEventTimeline(). This usage should be replaced by the fire and forget pattern and use room.findEventById(eventId) because it doesn't even use the timeline, it just wants the event that was loaded in as well.

It's probably a misnomer to call it getEventTimeline(): timeline in the first place as it's more accurately used as loadEventInTimeline(): Promise<void> everywhere.

And my new refreshLiveTimeline and getLatestTimeline usage is the only one where it needs an actually timeline.

With the updates, we're only working within the given timelineSet that was passed in (seems reasonable). If client.getEventTimeline() is really meant to just give the timeline for the eventId, then we should just provide the room instead which can look at all of the timelineSets (room.timelineSets).

Previously, we would return undefined in the case where the event doesn't belong in the timelineSet. Now we're returning a timeline in the timelineSet where all of the events returned by /context can go. This means events that we fetched, are actually added and not wasted. And it means that the main room timeline can be populated regardless if the eventId passed in was a threaded reply.

By ignored, I mean if the eventId is a threaded reply, it won't be added to the main room timelineSet that was passed in for example.

Copy link
Member

@turt2live turt2live Aug 4, 2022

Choose a reason for hiding this comment

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

Our usage might be to be a fire-and-forget pattern (or wrong, in the case of needing to use findEventById instead), but as a public function and SDK we have to maintain a rationalized contract for the function name: it says it gets an event timeline, so it should do that (returning undefined if needed)

We can adjust our code to instead use a new updateEventTimeline() function or similar, but the existing getEventTimeline function can't realistically have a behavioural change like this.

Copy link
Contributor Author

@MadLittleMods MadLittleMods Aug 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

The return undefined part is part of supportsExperimentalThreads which changed in https://github.com/matrix-org/matrix-js-sdk/pull/2444/files

Can we change the experimental implementation?

We can adjust our code to instead use a new updateEventTimeline() function or similar, but the existing getEventTimeline function can't realistically have a behavioural change like this.

In any case, this can work

Copy link
Member

@turt2live turt2live Sep 10, 2022

Choose a reason for hiding this comment

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

I'm still not comfortable with the behavioural change here, sorry. While our usage might be fire-and-forget, we can't guarantee that all usages of the function are fire-and-forget. The documentation and function itself are not experimental in nature as well, preventing us from making arbitrary breaking changes.

Adding a function is more code, but I think it's worthwhile here. It can even call getEventTimeline() and ignore the return value - it looks a bit silly, but it's how we avoid unnecessary major version releases.

Copy link
Contributor Author

@MadLittleMods MadLittleMods Nov 4, 2022

Choose a reason for hiding this comment

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

Conversation continued at #2852 (comment)

*
* @param {EventTimelineSet} timelineSet The timelineSet to look for the event in, must be bound to a room
* @param {string} eventId The ID of the event to look for
*
* @return {Promise} Resolves:
* {@link module:models/event-timeline~EventTimeline} including the given event
*/
public async getEventTimeline(timelineSet: EventTimelineSet, eventId: string): Promise<EventTimeline | undefined> {
public async getEventTimeline(timelineSet: EventTimelineSet, eventId: string): Promise<EventTimeline> {
// don't allow any timeline support unless it's been enabled.
if (!this.timelineSupport) {
throw new Error("timeline support is disabled. Set the 'timelineSupport'" +
Expand Down