From 0aa6eb2b447160266ddc3234530db341c0aeba82 Mon Sep 17 00:00:00 2001
From: Alexander Skvortsov <38059171+askvortsov1@users.noreply.github.com>
Date: Fri, 2 Oct 2020 16:56:40 -0400
Subject: [PATCH] Use Link component for links instead of mithril route patch
 (#2315)

This new component now also supports external links.
---
 framework/core/js/src/common/compat.js        |  2 +
 .../core/js/src/common/components/Link.js     | 47 ++++++++++++++++
 .../js/src/common/components/LinkButton.js    |  3 +-
 .../core/js/src/common/utils/patchMithril.js  | 53 +++++--------------
 .../forum/components/DiscussionListItem.js    | 11 ++--
 .../components/DiscussionsSearchSource.js     |  5 +-
 .../src/forum/components/EditPostComposer.js  |  5 +-
 .../core/js/src/forum/components/EventPost.js |  5 +-
 .../js/src/forum/components/Notification.js   | 11 ++--
 .../src/forum/components/NotificationList.js  |  5 +-
 .../js/src/forum/components/PostPreview.js    |  5 +-
 .../core/js/src/forum/components/PostUser.js  |  5 +-
 .../js/src/forum/components/PostsUserPage.js  |  3 +-
 .../js/src/forum/components/ReplyComposer.js  |  5 +-
 .../core/js/src/forum/components/UserCard.js  |  5 +-
 .../src/forum/components/UsersSearchSource.js |  5 +-
 16 files changed, 104 insertions(+), 71 deletions(-)
 create mode 100644 framework/core/js/src/common/components/Link.js

diff --git a/framework/core/js/src/common/compat.js b/framework/core/js/src/common/compat.js
index c6fe7e63a..268f33982 100644
--- a/framework/core/js/src/common/compat.js
+++ b/framework/core/js/src/common/compat.js
@@ -47,6 +47,7 @@ import FieldSet from './components/FieldSet';
 import Select from './components/Select';
 import Navigation from './components/Navigation';
 import Alert from './components/Alert';
+import Link from './components/Link';
 import LinkButton from './components/LinkButton';
 import Checkbox from './components/Checkbox';
 import SelectDropdown from './components/SelectDropdown';
@@ -118,6 +119,7 @@ export default {
   'components/Select': Select,
   'components/Navigation': Navigation,
   'components/Alert': Alert,
+  'components/Link': Link,
   'components/LinkButton': LinkButton,
   'components/Checkbox': Checkbox,
   'components/SelectDropdown': SelectDropdown,
diff --git a/framework/core/js/src/common/components/Link.js b/framework/core/js/src/common/components/Link.js
new file mode 100644
index 000000000..deca94683
--- /dev/null
+++ b/framework/core/js/src/common/components/Link.js
@@ -0,0 +1,47 @@
+import Component from '../Component';
+import extract from '../utils/extract';
+
+/**
+ * The link component enables both internal and external links.
+ * It will return a regular HTML link for any links to external sites,
+ * and it will use Mithril's m.route.Link for any internal links.
+ *
+ * Links will default to internal; the 'external' attr must be set to
+ * `true` for the link to be external.
+ */
+export default class Link extends Component {
+  view(vnode) {
+    let { options = {}, ...attrs } = vnode.attrs;
+
+    attrs.href = attrs.href || '';
+
+    // For some reason, m.route.Link does not like vnode.text, so if present, we
+    // need to convert it to text vnodes and store it in children.
+    const children = vnode.children || { tag: '#', children: vnode.text };
+
+    if (attrs.external) {
+      return <a {...attrs}>{children}</a>;
+    }
+
+    // If the href URL of the link is the same as the current page path
+    // we will not add a new entry to the browser history.
+    // This allows us to still refresh the Page component
+    // without adding endless history entries.
+    if (attrs.href === m.route.get()) {
+      if (!('replace' in options)) options.replace = true;
+    }
+
+    // Mithril 2 does not completely rerender the page if a route change leads to the same route
+    // (or the same component handling a different route).
+    // Here, the `force` parameter will use Mithril's key system to force a full rerender
+    // see https://mithril.js.org/route.html#key-parameter
+    if (extract(attrs, 'force')) {
+      if (!('state' in options)) options.state = {};
+      if (!('key' in options.state)) options.state.key = Date.now();
+    }
+
+    attrs.options = options;
+
+    return <m.route.Link {...attrs}>{children}</m.route.Link>;
+  }
+}
diff --git a/framework/core/js/src/common/components/LinkButton.js b/framework/core/js/src/common/components/LinkButton.js
index a21cdd88c..2dd3f6f9b 100644
--- a/framework/core/js/src/common/components/LinkButton.js
+++ b/framework/core/js/src/common/components/LinkButton.js
@@ -1,4 +1,5 @@
 import Button from './Button';
+import Link from './Link';
 
 /**
  * The `LinkButton` component defines a `Button` which links to a route.
@@ -22,7 +23,7 @@ export default class LinkButton extends Button {
   view(vnode) {
     const vdom = super.view(vnode);
 
-    vdom.tag = m.route.Link;
+    vdom.tag = Link;
     vdom.attrs.active = String(vdom.attrs.active);
 
     return vdom;
diff --git a/framework/core/js/src/common/utils/patchMithril.js b/framework/core/js/src/common/utils/patchMithril.js
index b6743f696..8360e2b95 100644
--- a/framework/core/js/src/common/utils/patchMithril.js
+++ b/framework/core/js/src/common/utils/patchMithril.js
@@ -1,43 +1,15 @@
-import extract from './extract';
+import Link from '../components/Link';
 import withAttr from './withAttr';
 import Stream from './Stream';
 
 let deprecatedMPropWarned = false;
 let deprecatedMWithAttrWarned = false;
 
+let deprecatedRouteWarned = false;
+
 export default function patchMithril(global) {
   const defaultMithril = global.m;
 
-  /**
-   * If the href URL of the link is the same as the current page path
-   * we will not add a new entry to the browser history.
-   *
-   * This allows us to still refresh the Page component
-   * without adding endless history entries.
-   *
-   * We also add the `force` attribute that adds a custom state key
-   * for when you want to force a complete refresh of the Page
-   */
-  const defaultLinkView = defaultMithril.route.Link.view;
-  const modifiedLink = {
-    view: function (vnode) {
-      let { href, options = {} } = vnode.attrs;
-
-      if (href === m.route.get()) {
-        if (!('replace' in options)) options.replace = true;
-      }
-
-      if (extract(vnode.attrs, 'force')) {
-        if (!('state' in options)) options.state = {};
-        if (!('key' in options.state)) options.state.key = Date.now();
-      }
-
-      vnode.attrs.options = options;
-
-      return defaultLinkView(vnode);
-    },
-  };
-
   const modifiedMithril = function (comp, ...args) {
     const node = defaultMithril.apply(this, arguments);
 
@@ -48,28 +20,29 @@ export default function patchMithril(global) {
       modifiedMithril.bidi(node, node.attrs.bidi);
     }
 
+    // DEPRECATED, REMOVE BETA 15
+
     // Allows us to use a "route" attr on links, which will automatically convert the link to one which
     // supports linking to other pages in the SPA without refreshing the document.
     if (node.attrs.route) {
-      node.attrs.href = node.attrs.route;
-      node.tag = modifiedLink;
-
-      // For some reason, m.route.Link does not like vnode.text, so if present, we
-      // need to convert it to text vnodes and store it in children.
-      if (node.text) {
-        node.children = { tag: '#', children: node.text };
+      if (!deprecatedRouteWarned) {
+        deprecatedRouteWarned = true;
+        console.warn('The route attr patch for links is deprecated, please use the Link component (flarum/components/Link) instead.');
       }
 
+      node.attrs.href = node.attrs.route;
+      node.tag = Link;
+
       delete node.attrs.route;
     }
 
+    // END DEPRECATED
+
     return node;
   };
 
   Object.keys(defaultMithril).forEach((key) => (modifiedMithril[key] = defaultMithril[key]));
 
-  modifiedMithril.route.Link = modifiedLink;
-
   // BEGIN DEPRECATED MITHRIL 2 BC LAYER
   modifiedMithril.prop = modifiedMithril.stream = function (...args) {
     if (!deprecatedMPropWarned) {
diff --git a/framework/core/js/src/forum/components/DiscussionListItem.js b/framework/core/js/src/forum/components/DiscussionListItem.js
index 438d503b9..ef91d2520 100644
--- a/framework/core/js/src/forum/components/DiscussionListItem.js
+++ b/framework/core/js/src/forum/components/DiscussionListItem.js
@@ -1,4 +1,5 @@
 import Component from '../../common/Component';
+import Link from '../../common/components/Link';
 import avatar from '../../common/helpers/avatar';
 import listItems from '../../common/helpers/listItems';
 import highlight from '../../common/helpers/highlight';
@@ -98,8 +99,8 @@ export default class DiscussionListItem extends Component {
         </a>
 
         <div className={'DiscussionListItem-content Slidable-content' + (isUnread ? ' unread' : '') + (isRead ? ' read' : '')}>
-          <a
-            route={user ? app.route.user(user) : '#'}
+          <Link
+            href={user ? app.route.user(user) : '#'}
             className="DiscussionListItem-author"
             title={extractText(
               app.translator.trans('core.forum.discussion_list.started_text', { user: user, ago: humanTime(discussion.createdAt()) })
@@ -109,14 +110,14 @@ export default class DiscussionListItem extends Component {
             }}
           >
             {avatar(user, { title: '' })}
-          </a>
+          </Link>
 
           <ul className="DiscussionListItem-badges badges">{listItems(discussion.badges().toArray())}</ul>
 
-          <a route={app.route.discussion(discussion, jumpTo)} className="DiscussionListItem-main">
+          <Link href={app.route.discussion(discussion, jumpTo)} className="DiscussionListItem-main">
             <h3 className="DiscussionListItem-title">{highlight(discussion.title(), this.highlightRegExp)}</h3>
             <ul className="DiscussionListItem-info">{listItems(this.infoItems().toArray())}</ul>
-          </a>
+          </Link>
 
           <span
             className="DiscussionListItem-count"
diff --git a/framework/core/js/src/forum/components/DiscussionsSearchSource.js b/framework/core/js/src/forum/components/DiscussionsSearchSource.js
index 7f615fac3..39d5a4307 100644
--- a/framework/core/js/src/forum/components/DiscussionsSearchSource.js
+++ b/framework/core/js/src/forum/components/DiscussionsSearchSource.js
@@ -1,5 +1,6 @@
 import highlight from '../../common/helpers/highlight';
 import LinkButton from '../../common/components/LinkButton';
+import Link from '../../common/components/Link';
 
 /**
  * The `DiscussionsSearchSource` finds and displays discussion search results in
@@ -47,10 +48,10 @@ export default class DiscussionsSearchSource {
 
         return (
           <li className="DiscussionSearchResult" data-index={'discussions' + discussion.id()}>
-            <a route={app.route.discussion(discussion, mostRelevantPost && mostRelevantPost.number())}>
+            <Link href={app.route.discussion(discussion, mostRelevantPost && mostRelevantPost.number())}>
               <div className="DiscussionSearchResult-title">{highlight(discussion.title(), query)}</div>
               {mostRelevantPost ? <div className="DiscussionSearchResult-excerpt">{highlight(mostRelevantPost.contentPlain(), query, 100)}</div> : ''}
-            </a>
+            </Link>
           </li>
         );
       }),
diff --git a/framework/core/js/src/forum/components/EditPostComposer.js b/framework/core/js/src/forum/components/EditPostComposer.js
index 4d5d9dd11..41de12561 100644
--- a/framework/core/js/src/forum/components/EditPostComposer.js
+++ b/framework/core/js/src/forum/components/EditPostComposer.js
@@ -1,5 +1,6 @@
 import ComposerBody from './ComposerBody';
 import Button from '../../common/components/Button';
+import Link from '../../common/components/Link';
 import icon from '../../common/helpers/icon';
 
 function minimizeComposerIfFullScreen(e) {
@@ -39,9 +40,9 @@ export default class EditPostComposer extends ComposerBody {
       'title',
       <h3>
         {icon('fas fa-pencil-alt')}{' '}
-        <a route={app.route.discussion(post.discussion(), post.number())} onclick={minimizeComposerIfFullScreen}>
+        <Link href={app.route.discussion(post.discussion(), post.number())} onclick={minimizeComposerIfFullScreen}>
           {app.translator.trans('core.forum.composer_edit.post_link', { number: post.number(), discussion: post.discussion().title() })}
-        </a>
+        </Link>
       </h3>
     );
 
diff --git a/framework/core/js/src/forum/components/EventPost.js b/framework/core/js/src/forum/components/EventPost.js
index ab5edd51e..7016f0790 100644
--- a/framework/core/js/src/forum/components/EventPost.js
+++ b/framework/core/js/src/forum/components/EventPost.js
@@ -2,6 +2,7 @@ import Post from './Post';
 import { ucfirst } from '../../common/utils/string';
 import usernameHelper from '../../common/helpers/username';
 import icon from '../../common/helpers/icon';
+import Link from '../../common/components/Link';
 
 /**
  * The `EventPost` component displays a post which indicating a discussion
@@ -29,9 +30,9 @@ export default class EventPost extends Post {
     const data = Object.assign(this.descriptionData(), {
       user,
       username: user ? (
-        <a className="EventPost-user" route={app.route.user(user)}>
+        <Link className="EventPost-user" href={app.route.user(user)}>
           {username}
-        </a>
+        </Link>
       ) : (
         username
       ),
diff --git a/framework/core/js/src/forum/components/Notification.js b/framework/core/js/src/forum/components/Notification.js
index e011635fb..d32ef6edf 100644
--- a/framework/core/js/src/forum/components/Notification.js
+++ b/framework/core/js/src/forum/components/Notification.js
@@ -3,6 +3,7 @@ import avatar from '../../common/helpers/avatar';
 import icon from '../../common/helpers/icon';
 import humanTime from '../../common/helpers/humanTime';
 import Button from '../../common/components/Button';
+import Link from '../../common/components/Link';
 
 /**
  * The `Notification` component abstract displays a single notification.
@@ -19,13 +20,11 @@ export default class Notification extends Component {
     const notification = this.attrs.notification;
     const href = this.href();
 
-    const linkAttrs = {};
-    linkAttrs[href.indexOf('://') === -1 ? 'route' : 'href'] = href;
-
     return (
-      <a
+      <Link
         className={'Notification Notification--' + notification.contentType() + ' ' + (!notification.isRead() ? 'unread' : '')}
-        {...linkAttrs}
+        href={href}
+        external={href.indexOf('://') === -1}
         onclick={this.markAsRead.bind(this)}
       >
         {!notification.isRead() &&
@@ -45,7 +44,7 @@ export default class Notification extends Component {
         <span className="Notification-content">{this.content()}</span>
         {humanTime(notification.createdAt())}
         <div className="Notification-excerpt">{this.excerpt()}</div>
-      </a>
+      </Link>
     );
   }
 
diff --git a/framework/core/js/src/forum/components/NotificationList.js b/framework/core/js/src/forum/components/NotificationList.js
index 369a4732b..4eb7fea0d 100644
--- a/framework/core/js/src/forum/components/NotificationList.js
+++ b/framework/core/js/src/forum/components/NotificationList.js
@@ -1,6 +1,7 @@
 import Component from '../../common/Component';
 import listItems from '../../common/helpers/listItems';
 import Button from '../../common/components/Button';
+import Link from '../../common/components/Link';
 import LoadingIndicator from '../../common/components/LoadingIndicator';
 import Discussion from '../../common/models/Discussion';
 
@@ -63,10 +64,10 @@ export default class NotificationList extends Component {
                   return (
                     <div className="NotificationGroup">
                       {group.discussion ? (
-                        <a className="NotificationGroup-header" route={app.route.discussion(group.discussion)}>
+                        <Link className="NotificationGroup-header" href={app.route.discussion(group.discussion)}>
                           {badges && badges.length ? <ul className="NotificationGroup-badges badges">{listItems(badges)}</ul> : ''}
                           {group.discussion.title()}
-                        </a>
+                        </Link>
                       ) : (
                         <div className="NotificationGroup-header">{app.forum.attribute('title')}</div>
                       )}
diff --git a/framework/core/js/src/forum/components/PostPreview.js b/framework/core/js/src/forum/components/PostPreview.js
index e8cd5fe0f..14b51fccf 100644
--- a/framework/core/js/src/forum/components/PostPreview.js
+++ b/framework/core/js/src/forum/components/PostPreview.js
@@ -1,4 +1,5 @@
 import Component from '../../common/Component';
+import Link from '../../common/components/Link';
 import avatar from '../../common/helpers/avatar';
 import username from '../../common/helpers/username';
 import highlight from '../../common/helpers/highlight';
@@ -18,12 +19,12 @@ export default class PostPreview extends Component {
     const excerpt = highlight(post.contentPlain(), this.attrs.highlight, 300);
 
     return (
-      <a className="PostPreview" route={app.route.post(post)} onclick={this.attrs.onclick}>
+      <Link className="PostPreview" href={app.route.post(post)} onclick={this.attrs.onclick}>
         <span className="PostPreview-content">
           {avatar(user)}
           {username(user)} <span className="PostPreview-excerpt">{excerpt}</span>
         </span>
-      </a>
+      </Link>
     );
   }
 }
diff --git a/framework/core/js/src/forum/components/PostUser.js b/framework/core/js/src/forum/components/PostUser.js
index bb0b379de..667ee04b3 100644
--- a/framework/core/js/src/forum/components/PostUser.js
+++ b/framework/core/js/src/forum/components/PostUser.js
@@ -1,4 +1,5 @@
 import Component from '../../common/Component';
+import Link from '../../common/components/Link';
 import UserCard from './UserCard';
 import avatar from '../../common/helpers/avatar';
 import username from '../../common/helpers/username';
@@ -40,11 +41,11 @@ export default class PostUser extends Component {
     return (
       <div className="PostUser">
         <h3>
-          <a route={app.route.user(user)}>
+          <Link href={app.route.user(user)}>
             {avatar(user, { className: 'PostUser-avatar' })}
             {userOnline(user)}
             {username(user)}
-          </a>
+          </Link>
         </h3>
         <ul className="PostUser-badges badges">{listItems(user.badges().toArray())}</ul>
         {card}
diff --git a/framework/core/js/src/forum/components/PostsUserPage.js b/framework/core/js/src/forum/components/PostsUserPage.js
index f8861538e..eb420bfb2 100644
--- a/framework/core/js/src/forum/components/PostsUserPage.js
+++ b/framework/core/js/src/forum/components/PostsUserPage.js
@@ -1,6 +1,7 @@
 import UserPage from './UserPage';
 import LoadingIndicator from '../../common/components/LoadingIndicator';
 import Button from '../../common/components/Button';
+import Link from '../../common/components/Link';
 import Placeholder from '../../common/components/Placeholder';
 import CommentPost from './CommentPost';
 
@@ -73,7 +74,7 @@ export default class PostsUserPage extends UserPage {
             <li>
               <div className="PostsUserPage-discussion">
                 {app.translator.trans('core.forum.user.in_discussion_text', {
-                  discussion: <a route={app.route.post(post)}>{post.discussion().title()}</a>,
+                  discussion: <Link href={app.route.post(post)}>{post.discussion().title()}</Link>,
                 })}
               </div>
 
diff --git a/framework/core/js/src/forum/components/ReplyComposer.js b/framework/core/js/src/forum/components/ReplyComposer.js
index f0923fbde..a53862740 100644
--- a/framework/core/js/src/forum/components/ReplyComposer.js
+++ b/framework/core/js/src/forum/components/ReplyComposer.js
@@ -1,5 +1,6 @@
 import ComposerBody from './ComposerBody';
 import Button from '../../common/components/Button';
+import Link from '../../common/components/Link';
 import icon from '../../common/helpers/icon';
 import extractText from '../../common/utils/extractText';
 
@@ -36,9 +37,9 @@ export default class ReplyComposer extends ComposerBody {
       'title',
       <h3>
         {icon('fas fa-reply')}{' '}
-        <a route={app.route.discussion(discussion)} onclick={minimizeComposerIfFullScreen}>
+        <Link href={app.route.discussion(discussion)} onclick={minimizeComposerIfFullScreen}>
           {discussion.title()}
-        </a>
+        </Link>
       </h3>
     );
 
diff --git a/framework/core/js/src/forum/components/UserCard.js b/framework/core/js/src/forum/components/UserCard.js
index 754135216..b5ff52782 100644
--- a/framework/core/js/src/forum/components/UserCard.js
+++ b/framework/core/js/src/forum/components/UserCard.js
@@ -6,6 +6,7 @@ import avatar from '../../common/helpers/avatar';
 import username from '../../common/helpers/username';
 import icon from '../../common/helpers/icon';
 import Dropdown from '../../common/components/Dropdown';
+import Link from '../../common/components/Link';
 import AvatarEditor from './AvatarEditor';
 import listItems from '../../common/helpers/listItems';
 
@@ -50,10 +51,10 @@ export default class UserCard extends Component {
                 {this.attrs.editable ? (
                   [AvatarEditor.component({ user, className: 'UserCard-avatar' }), username(user)]
                 ) : (
-                  <a route={app.route.user(user)}>
+                  <Link href={app.route.user(user)}>
                     <div className="UserCard-avatar">{avatar(user)}</div>
                     {username(user)}
-                  </a>
+                  </Link>
                 )}
               </h2>
 
diff --git a/framework/core/js/src/forum/components/UsersSearchSource.js b/framework/core/js/src/forum/components/UsersSearchSource.js
index 461fbb477..de0a50635 100644
--- a/framework/core/js/src/forum/components/UsersSearchSource.js
+++ b/framework/core/js/src/forum/components/UsersSearchSource.js
@@ -1,6 +1,7 @@
 import highlight from '../../common/helpers/highlight';
 import avatar from '../../common/helpers/avatar';
 import username from '../../common/helpers/username';
+import Link from '../../common/components/Link';
 
 /**
  * The `UsersSearchSource` finds and displays user search results in the search
@@ -48,10 +49,10 @@ export default class UsersSearchResults {
 
         return (
           <li className="UserSearchResult" data-index={'users' + user.id()}>
-            <a route={app.route.user(user)}>
+            <Link href={app.route.user(user)}>
               {avatar(user)}
               {{ ...name, text: undefined, children }}
-            </a>
+            </Link>
           </li>
         );
       }),