This repository has been archived by the owner on Sep 11, 2024. It is now read-only.
-
-
Notifications
You must be signed in to change notification settings - Fork 827
Support local permalinks for unfederated instances #3500
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Just a little bit of refactoring to make the feature of custom prefixes a bit easier.
This allows for Riot permalinks to be introduced without if-else ladders everywhere.
Without the requirement for a room to work
Originally we were planning on using the current location as the permalink prefix, but that doesn't work if the user is a desktop user.
See code for rationale
Also adds some safety around the Riot URL to ensure it mostly looks like a URL
They might not be matrix.to anymore, so parse them more correctly.
Now that permalinks could be not-matrix.to we should be safer in what we'll allow.
The file handles more than just a RoomPermalinkCreator, so we should name it accordingly.
OR doesn't give us the right thing, but AND does.
dbkr
approved these changes
Oct 1, 2019
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.
This also looks much better in terms of general code hygiene and keeping all the permalink stuff in one place rather than scattered over the SDK - thanks!
🙌 |
Woot! :D This should also address an earlier #7659 |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes element-hq/element-web#8149
Reviewer: This is a complicated changeset (sorry). My best suggestion is for the code to be reviewed commit by commit, and to bear in mind that a concern you have early in the review process may be solved later in a later commit (there's at least 3 rounds of refactoring done throughout the commits).
Also there's a brain dump of information below which might be useful. Or not. You're welcome.
There's no screenshots because there's no visual changes here. A bunch of testing information is included at the end of the description if you're interested though.
This exposes a new config option (documented here: element-hq/element-web#11007) to allow self-hosted instances of Riot to use local URLs instead of matrix.to for permalinks. This is primarily intended for instances which aren't federated, as matrix.to is unlikely to work or be happy. This may cause incompatibility with the mobile apps due to the potential for random web servers to be hosting "permalinks" (ie: mobile will have no idea what to do with
https://riot.custom.example.org/#/room/!example:example.org
).This is strictly not intended for use on riot.im/app or /develop (etc) or any other federated instance.
Code overview (because it's complicated)
There's two types of permalinks we support:
Spec
(matrix.to) andRiot
(whatever you've configured that as). These are represented by their own respectivePermalinkConstructor
classes (withPermalinkConstructor
itself being a stub for a future TypeScript interface). These classes have all the meat in them for handling permalinks, andPermalinks.js
(RoomPermalinkCreator
) just forwards it's request for permalink stuff over to the appropriatePermalinkConstructor
. This lends itself to a potential third permalink type, if we're ever crazy enough to do that, as well as easier maintenance on our existing permalink types.Previously we just had a bunch of regular expressions laying around and executed those when we needed to, making the change to support Riot permalinks harder. This has now been centralized into the
Permalinks
class asparsePermalink
.All told, the changes aren't that complex when looking at the stuff from a high level point of view. They get complicated with the parsing and usage pattern of the new system, particularly when compared to the old way of doing things.
Testing (for future adventurers in this area)
As of writing, the major test cases to cover when changing how permalinks are:
href
href
, and workshref
, and workshref
, and works/goto https://matrix.to/...
works as expected/goto <custom permalink>
works as expectedFilePanel
works as expectedThe manual tests may also need to be repeated for the Desktop app.