Use Link component for links instead of mithril route patch

[askvortsov1]

This PR:

• Separates out the patched mithril link into an explicit component
• Adds functionality so that this component can also process external links (so we can remove the hacky workaround in Notification and in fof links)
• Deprecates the route mithril patch. This is deprecated and not removed because:
  • It existed in Mithril 0.2
  • Deprecating allows us to test these changes locally without immediately converting ALL bundled and fof extensions

[franzliedke]

This is deprecated and not removed because: It existed in Mithril 0.2

Huh? I thought we changed all occurrences of <a href="#" config={m.route}> into <a route="#">? IMO, the BC layer is fine for testing this out (and before enforcing it for all extensions), but it should be removed again before release - no point in creating BC layers for never-released features from between two releases, right?

[askvortsov1]

I'm fine with that, I just want to get the core structure finalized before updating 30 extensions.

[clarkwinkelmann]

I'm globally ok with the change, I'm just unsure how much we want to document the changes to LinkButton?

Previously all links would be handled internally, unless an empty config method was passed to override the default m.route.

Now the new implementation uses what I see at line 26 of the Link component that is based on absolute vs relative links.

Don't we also sometime use absolute links for internal routing? Like for Mentions. That would be a change of behavior. I'm not entirely convinced it's a good idea to change the type on link based on whether the link is absolute. Also is the includes() test completely resilient against :// appearing elsewhere in the URL?

[askvortsov1]

I'm globally ok with the change, I'm just unsure how much we want to document the changes to LinkButton?

Previously all links would be handled internally, unless an empty config method was passed to override the default m.route.

Now the new implementation uses what I see at line 26 of the Link component that is based on absolute vs relative links.

Don't we also sometime use absolute links for internal routing? Like for Mentions. That would be a change of behavior. I'm not entirely convinced it's a good idea to change the type on link based on whether the link is absolute. Also is the includes() test completely resilient against :// appearing elsewhere in the URL?

You bring up a good point. The absolute/relative link thing would be cool, but needs more testing and discussion to make sure it is right. Let's remove that part for now, but keep the rest?

[clarkwinkelmann]

You bring up a good point. The absolute/relative link thing would be cool, but needs more testing and discussion to make sure it is right. Let's remove that part for now, but keep the rest?

That would seem good to me 👍

We just need to add some way for LinkButton to be used for external links in the meantime. Maybe some external: true or internal:false attr?

[askvortsov1]

Yeah that sounds good... Actually.... I suppose we could pass that "external" flag all the way to Link, and use that instead of ://? Would still be nicer than what we're doing now for notifications

[dsevillamartin]

Any reason why we are specifying external links? Does m.route.Link not work with external links? Apart from that, LGTM.

[askvortsov1]

From what I remember, it doesn't

[dsevillamartin]

I approved, but I do also wonder why we decided to not extend m.route.Link. It's not important tho, and without extending it we can handle external links as well.

[askvortsov1]

Given that the goal is to make things more direct, I thought that explictly including one link or another by composition would be preferable