DEV: Update homepage URL handling (#24373)

We want / to display one of our discovery routes/controllers, but we don't want to register it as `discovery.index` because that would break themes/plugins which check the route name. Previously, this was handled using a variety of approaches throughout the codebase (in discourse-location, discourse-url and mapping-router). But even then, it didn't work consistently. For example, if you used an Ember method like `router.transitionTo("/")`, an empty `discovery.index` page would be rendered.

This commit switches up the approach. `discovery.index` is now defined as a real route, and redirects to the desired homepage. To preserve the `/` as a 'vanity url', we patch the method on the router responsible for persisting URLs to the Ember Router and the browser. The patch identifies a relevant transition by looking for a magic query parameter.

In an ideal world, we wouldn't be patching the router at all. But at least with this commit, the workaround is all in one place, and works consistently for all navigation methods. The new strategy is also much better tested.

app/assets/javascripts/discourse/app/lib/discourse-location.js

@@ -1,7 +1,6 @@
 import EmberObject from "@ember/object";
 import { guidFor } from "@ember/object/internals";
 import $ from "jquery";
-import { defaultHomepage } from "discourse/lib/utilities";
 import { withoutPrefix } from "discourse-common/lib/get-url";
 
 let popstateFired = false;
@@ -96,10 +95,7 @@ const DiscourseLocation = EmberObject.extend({
     path = this.formatURL(path);
 
     if (state && state.path !== path) {
-      const paths = [path, state.path];
-      if (!(paths.includes("/") && paths.includes(`/${defaultHomepage()}`))) {
-        this.pushState(path);
-      }
+      this.pushState(path);
     }
   },
 

app/assets/javascripts/discourse/app/lib/homepage-router-overrides.js

@@ -0,0 +1,41 @@
+import { defaultHomepage } from "discourse/lib/utilities";
+
+/**
+ * We want / to display one of our discovery routes/controllers, but we don't
+ * want to register it as `discovery.index` because that would break themes/plugins which
+ * check the route name.
+ *
+ * Instead, `discovery.index` 'redirects' to a magic URL which we watch for in the router.
+ * When detected, we rewrite the URL to `/` before saving it to the Ember router and the browser.
+ */
+export default function applyRouterHomepageOverrides(router) {
+  const microLib = router._routerMicrolib;
+
+  for (const method of ["updateURL", "replaceURL"]) {
+    const original = microLib[method].bind(microLib);
+    microLib[method] = function (url) {
+      url = rewriteIfNeeded(url, this.activeTransition);
+      return original(url);
+    };
+  }
+}
+
+/**
+ * Returns a magic URL which `discovery-index` will redirect to.
+ * We watch for this, and then perform the rewrite in the router.
+ */
+export function homepageDestination() {
+  return `/${defaultHomepage()}?_discourse_homepage_rewrite=1`;
+}
+
+function rewriteIfNeeded(url, transition) {
+  const intentUrl = transition?.intent?.url;
+  if (intentUrl?.startsWith(homepageDestination())) {
+    const params = url.split("?", 2)[1];
+    url = "/";
+    if (params) {
+      url += `?${params}`;
+    }
+  }
+  return url;
+}

app/assets/javascripts/discourse/app/lib/url.js

@@ -255,17 +255,11 @@ const DiscourseURL = EmberObject.extend({
       return;
     }
 
-    if (oldPath === path) {
-      // If navigating to the same path send an app event.
-      // Views can watch it and tell their controllers to refresh
+    if (oldPath === path || this.refreshedHomepage(oldPath, path)) {
+      // If navigating to the same path, refresh the route
       this.routerService.refresh();
     }
 
-    // TODO: Extract into rules we can inject into the URL handler
-    if (this.navigatedToHome(oldPath, path, opts)) {
-      return;
-    }
-
     // Navigating to empty string is the same as root
     if (path === "") {
       path = "/";
@@ -389,20 +383,13 @@ const DiscourseURL = EmberObject.extend({
     @param {String} oldPath the previous path we were on
     @param {String} path the path we're navigating to
   **/
-  navigatedToHome(oldPath, path) {
+  refreshedHomepage(oldPath, path) {
     const homepage = defaultHomepage();
 
-    if (
-      window.history &&
-      window.history.pushState &&
+    return (
       (path === "/" || path === "/" + homepage) &&
       (oldPath === "/" || oldPath === "/" + homepage)
-    ) {
-      this.routerService.refresh();
-      return true;
-    }
-
-    return false;
+    );
   },
 
   // This has been extracted so it can be tested.
@@ -452,15 +439,7 @@ const DiscourseURL = EmberObject.extend({
       elementId = split[1];
     }
 
-    // The default path has a hack to allow `/` to default to defaultHomepage
-    // via BareRouter.handleUrl
-    let transition;
-    if (path === "/" || path.substring(0, 2) === "/?") {
-      router._routerMicrolib.updateURL(path);
-      transition = router.handleURL(path);
-    } else {
-      transition = router.transitionTo(path);
-    }
+    const transition = router.transitionTo(path);
 
     transition._discourse_intercepted = true;
     transition._discourse_anchor = elementId;

app/assets/javascripts/discourse/app/mapping-router.js

@@ -1,24 +1,20 @@
 import EmberRouter from "@ember/routing/router";
-import { defaultHomepage } from "discourse/lib/utilities";
 import Site from "discourse/models/site";
 import { isTesting } from "discourse-common/config/environment";
 import getURL from "discourse-common/lib/get-url";
+import applyRouterHomepageOverrides from "./lib/homepage-router-overrides";
 
-const BareRouter = EmberRouter.extend({
-  location: isTesting() ? "none" : "discourse-location",
+class BareRouter extends EmberRouter {
+  location = isTesting() ? "none" : "discourse-location";
 
-  handleURL(url) {
-    const params = url.split("?");
-
-    if (params[0] === "/" || params[0] === "") {
-      url = defaultHomepage();
-      if (params[1] && params[1].length) {
-        url = `${url}?${params[1]}`;
-      }
+  setupRouter() {
+    const didSetup = super.setupRouter(...arguments);
+    if (didSetup) {
+      applyRouterHomepageOverrides(this);
     }
-    return this._super(url);
-  },
-});
+    return didSetup;
+  }
+}
 
 // Ember's router can't be extended. We need to allow plugins to add routes to routes that were defined
 // in the core app. This class has the same API as Ember's `Router.map` but saves the results in a tree.

app/assets/javascripts/discourse/app/routes/discovery-index.js

@@ -0,0 +1,19 @@
+import { inject as service } from "@ember/service";
+import { homepageDestination } from "discourse/lib/homepage-router-overrides";
+import { disableImplicitInjections } from "discourse/lib/implicit-injections";
+import DiscourseRoute from "./discourse";
+
+@disableImplicitInjections
+export default class DiscoveryIndex extends DiscourseRoute {
+  @service router;
+
+  beforeModel(transition) {
+    const url = transition.intent.url;
+    const params = url.split("?", 2)[1];
+    let destination = homepageDestination();
+    if (params) {
+      destination += `&${params}`;
+    }
+    this.router.transitionTo(destination);
+  }
+}

app/assets/javascripts/discourse/tests/acceptance/homepage-test.js

@@ -0,0 +1,71 @@
+import { getOwner } from "@ember/application";
+import { click, visit } from "@ember/test-helpers";
+import { test } from "qunit";
+import { setDefaultHomepage } from "discourse/lib/utilities";
+import { acceptance } from "discourse/tests/helpers/qunit-helpers";
+
+acceptance("Dynamic homepage handling", function () {
+  test("it works when set to latest", async function (assert) {
+    const router = getOwner(this).lookup("service:router");
+
+    function assertOnLatest(path) {
+      assert.strictEqual(
+        router.currentRouteName,
+        "discovery.latest",
+        "is on latest route"
+      );
+      assert.strictEqual(router.currentURL, path, `path is ${path}`);
+      assert
+        .dom(".nav-item_latest a")
+        .hasClass("active", "discovery-latest is displayed");
+    }
+
+    await visit("/");
+    assertOnLatest("/");
+
+    await click("#site-logo");
+    assertOnLatest("/");
+
+    await router.transitionTo("/").followRedirects();
+    assertOnLatest("/");
+
+    await click(".nav-item_latest a");
+    assertOnLatest("/latest");
+
+    await visit("/?order=posts");
+    assertOnLatest("/?order=posts");
+    assert
+      .dom("th.posts.sortable")
+      .hasClass("sorting", "query params are passed through");
+  });
+
+  test("it works when set to categories", async function (assert) {
+    setDefaultHomepage("categories");
+
+    const router = getOwner(this).lookup("service:router");
+
+    function assertOnCategories(path) {
+      assert.strictEqual(
+        router.currentRouteName,
+        "discovery.categories",
+        "is on categories route"
+      );
+      assert.strictEqual(router.currentURL, path, `path is ${path}`);
+      assert
+        .dom(".nav-item_categories a")
+        .hasClass("active", "discovery-categories is displayed");
+    }
+
+    await visit("/");
+    assertOnCategories("/");
+
+    await click("#site-logo");
+    assertOnCategories("/");
+
+    await router.transitionTo("/").followRedirects();
+    assertOnCategories("/");
+
+    await click(".nav-item_categories a");
+    assertOnCategories("/categories");
+  });
+});

app/assets/javascripts/discourse/tests/acceptance/topic-discovery-test.js

@@ -157,8 +157,8 @@ acceptance("Topic Discovery", function (needs) {
   });
 
   test("refreshing tabs", async function (assert) {
-    const assertShowingLatest = () => {
-      assert.strictEqual(currentURL(), "/latest", "stays on latest");
+    const assertShowingLatest = (url) => {
+      assert.strictEqual(currentURL(), url, "stays on latest");
       const el = query(".topic-list-body .topic-list-item:first-of-type");
       assert.strictEqual(el.closest(".hidden"), null, "topic list is visible");
       assert.strictEqual(
@@ -169,13 +169,13 @@ acceptance("Topic Discovery", function (needs) {
     };
 
     await visit("/latest");
-    assertShowingLatest();
+    assertShowingLatest("/latest");
 
     await click(".navigation-container a[href='/latest']");
-    assertShowingLatest();
+    assertShowingLatest("/latest");
 
     await click("#site-logo");
-    assertShowingLatest();
+    assertShowingLatest("/");
   });
 });