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 room upgrades #100

Merged
merged 14 commits into from
Mar 14, 2019
Merged

Support room upgrades #100

merged 14 commits into from
Mar 14, 2019

Conversation

Half-Shot
Copy link
Contributor

@Half-Shot Half-Shot commented Mar 11, 2019

Fixes #95

This works by listening for a m.room.tombstone event and attempts to join the referenced room (first by room_id, then by alias). If that succeeds it finds all entries by the old room id and replace with the new room id. If the join fails, it waits for a subsequent invite before trying to update the entries.

The entry update logic can be overriden by providing a callback, and bridges can hook into another callback once an upgrade has been completed.

This functionality is disabled by default and must be enabled by providing a config.

Signed-off-by: Will Hunt <will@half-shot.uk>
Signed-off-by: Will Hunt <will@half-shot.uk>
@Half-Shot Half-Shot requested a review from a team March 11, 2019 22:25
@Half-Shot Half-Shot changed the title Support room upgrades #95 Support room upgrades Mar 11, 2019
@turt2live turt2live self-requested a review March 11, 2019 22:34
Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

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

This also needs proper documentation outside of the code.

lib/bridge.js Outdated Show resolved Hide resolved
lib/components/intent.js Outdated Show resolved Hide resolved
lib/components/room-upgrade-handler.js Outdated Show resolved Hide resolved
lib/components/room-upgrade-handler.js Outdated Show resolved Hide resolved
lib/components/room-upgrade-handler.js Outdated Show resolved Hide resolved
*/
class RoomUpgradeHandler {
/**
* @param {RoomUpgradeHandler~Options} opts
Copy link
Member

Choose a reason for hiding this comment

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

what are they?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See below

Copy link
Member

Choose a reason for hiding this comment

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

They should be described here in the jsdoc if we expect people to create this object.

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'm not describing them 3 times, people have the ability to click on hyperlinks.

Copy link
Member

Choose a reason for hiding this comment

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

that's fair, I thought this was much higher in the stack. Do we even need jsdoc here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, otherwise we don't know the type that we are passing.

lib/bridge.js Outdated Show resolved Hide resolved
@turt2live
Copy link
Member

also this needs options for handling ghosts.

@Half-Shot
Copy link
Contributor Author

also this needs options for handling ghosts.

It doesn't. Ghosts are very particular to the bridge and should be handled on onRoomMigrated

Signed-off-by: Will Hunt <will@half-shot.uk>
Signed-off-by: Will Hunt <will@half-shot.uk>
Signed-off-by: Will Hunt <will@half-shot.uk>
Signed-off-by: Will Hunt <will@half-shot.uk>
Signed-off-by: Will Hunt <will@half-shot.uk>
@Half-Shot Half-Shot requested a review from a team March 12, 2019 13:01
Signed-off-by: Will Hunt <will@half-shot.uk>
Signed-off-by: Will Hunt <will@half-shot.uk>
@Half-Shot
Copy link
Contributor Author

This also needs proper documentation outside of the code.

More than JSDoc?

Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

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

I'm going to be picky and say that we need a ghost copy option on the upgrade handler. We're already cutting over the room entry in the store and the appservice user themselves - most bridges are going to want to blind copy the users in as well. There's a few bridges I can think of that won't want to do this, which is why it should be a flag.

As for documentation: A mention to what the room upgrade handler is in the README is all I'm looking for. We don't have good enough docs to support the actual level of documentation I would normally expect, so I won't block this on that.

lib/bridge.js Show resolved Hide resolved
*/
class RoomUpgradeHandler {
/**
* @param {RoomUpgradeHandler~Options} opts
Copy link
Member

Choose a reason for hiding this comment

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

that's fair, I thought this was much higher in the stack. Do we even need jsdoc here?

Signed-off-by: Will Hunt <will@half-shot.uk>
Signed-off-by: Will Hunt <will@half-shot.uk>
Signed-off-by: Will Hunt <will@half-shot.uk>
Signed-off-by: Will Hunt <will@half-shot.uk>
Signed-off-by: Will Hunt <will@half-shot.uk>
Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

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

There's not really any point in blocking this on the ghost stuff anymore - we need this for 1.0, and clearly there's resistance on adding it. Have opened #102 to wishlist it.

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.

None yet

2 participants