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

Lazy loading: don't assume we have our own member available #677

Merged

Conversation

bwindels
Copy link
Contributor

@bwindels bwindels commented Aug 2, 2018

No description provided.

@bwindels bwindels requested a review from a team August 2, 2018 10:17
@bwindels bwindels changed the title Lazy loading: don't assume we have our own membership available Lazy loading: don't assume we have our own member available Aug 2, 2018
@bwindels
Copy link
Contributor Author

bwindels commented Aug 2, 2018

Perhaps a bit controversial, but I moved the syncing user id into room, as it makes it makes room more self-contained, the methods getMyMembership and recalculate don't need a user id anymore, which I think makes sense. Also less logic in react-sdk like this. Let me know what you think.

Copy link
Member

@dbkr dbkr left a comment

Choose a reason for hiding this comment

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

Yes, I think this makes sense. I was trying to figure out if we declare Room as public bit of API anywhere: I can't imagine why an API user would ever construct one, so I don't think this is a breaking change.

@bwindels
Copy link
Contributor Author

bwindels commented Aug 3, 2018

@dbkr The room constructor is called once from react-sdk (from VectorConferenceHandler.js:87), but that should be addressed by the PR that you already merged. https://github.com/matrix-org/matrix-react-sdk/pull/2102/files

@bwindels bwindels merged commit 7f8be3d into bwindels/feature_lazyloading Aug 3, 2018
@bwindels bwindels deleted the bwindels/selfmembershipchecks branch August 3, 2018 11:57
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.

2 participants