-
-
Notifications
You must be signed in to change notification settings - Fork 583
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
Lazy load room members - Part I #667
Lazy load room members - Part I #667
Conversation
emit individual RoomState.members/newMember events for each lazily loaded member as batch events are not a thing. This makes updating the memberlist work
so MemberInfo can use it and take lazy loading into account
src/models/room-state.js
Outdated
@@ -287,7 +321,7 @@ RoomState.prototype.getLastModifiedTime = function() { | |||
/** | |||
* Get user IDs with the specified display name. | |||
* @param {string} displayName The display name to get user IDs from. | |||
* @return {string[]} An array of user IDs or an empty array. | |||
* @return {string[]} An array of user IDs or an empty array. |
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.
seems like a spurious trailing whitespace was added for no reason?
for all timelines in all timeline sets
dont just rely on member events, but just copy the member
better than braking timeline continuation
In order for the lazy loading logic not to bleed into all corners of the JS SDK, I moved some of the state copying between timelines over to the RoomState and EventTimeLine class.
want to do a general review if it's ready to merge, but all should be here for a first PR. |
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.
You need to fix the tests
src/models/room.js
Outdated
*/ | ||
Room.prototype.setLazilyLoadedMembers = async function(joinedMembersPromise) { | ||
this._membersNeedLoading = false; | ||
const members = await joinedMembersPromise; |
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.
You might want to catch the case that this promise fails - e.g. when something is weird with the connection or server
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.
Good point!
src/models/room.js
Outdated
this._membersNeedLoading = false; | ||
const members = await joinedMembersPromise; | ||
//wait 10 seconds | ||
await new Promise((resolve) => setTimeout(resolve, 5000)); |
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.
Why should this wait? await joinedMembersPromise
should make sure that it only continues executing once it is (successfully) done.
Besides that "5000" != 10 seconds ;)
@dbkr Right, the only edge case I can think off is when you'd back paginate before the LL members are loaded, but that's probably avoidable. Adding support for lazy loading in back-pagination is both the plan for client and server before releasing this, see element-hq/element-web#6611 (adding this to the description here as well), so you're right we don't really need to correct any other room state but the forward, live one. I'll change it back. Added a big XXX corners cut comment where we call /joined_members 👍 |
…meline since back-paginating will also support lazy loading the state needed to display that part of the timeline, and no user interaction is supposed to happen before the lazy loaded member are, well, loaded, applying the ll members to all timelines should not be neccessary.
when cloning the state, lazy loaded members are copied over with their rawDisplayName, which could originate from their userId if they don't have a displayname. the displayname algorithm would assume that the displayname is explicitly set, and see if we'd have to disambiguate. As a fix, if the display name is the same as the id, just return the id
Yeah, this feels a lot safer. Once we have a |
Just had a look at the /members endpoint, and as it returns state events, not just profile information, some of the changes here could probably be avoided. So I'll make the change as part of this PR. |
This commit is a substantial change, as /members returns state events, not profile information as /joined_members, and this allows to simplify the implementation quite a bit. We can assume again all members have a state event associated with it. I also changed most of the naming of lazy loaded members to out-of-band members to reflect that this is the relevant bit for most of the code, that the members didn't come through /sync but through another channel. This commit also addresses the race condition between /(joined_)members and /sync. /members returns the members at the point in the timeline at a given event id. Members are loaded at the last event in the live timeline, and all members that come in from sync in the mean time are marked as superseding the out of band members, so they won't be overwritten, even if the timeline is reset in the mean time. Members are also marked if they originate from an out-of-band channel (/members) so they can be stored accordingly (future PR). The loading status is kept in room state now, as this made resolving the race condition easier. One consequence is that the status needs to be shared across cloned instances of RoomState. When resetting the timeline (and cloning the room state) while lazy loading is in progress, one of the RoomStates could be left in progress indefinitely. Though that is more for clarity than avoiding any actual bugs.
…ership to determine where a room should show up in the room list, we need to know our membership type. But with lazy loading, we might not have our own member if we weren't recently active in the room. Using getSyncedMembership can be used to fallback if the users membership is not yet available.
@dbkr ready for another look. This now includes calling /members instead of /joined_members |
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.
OK - looks plausible: let's give it a go on the feature branch
Lazy loading of members, for now using the /joined_members endpoint until we have something better.
RoomMember.events...
into RoomMember so it can transparently take lazily loaded members into account./members
endpointParent issue: element-hq/element-web#6611