From 78c6fc6e4365fdcaaaf763778d11118ae015c5c4 Mon Sep 17 00:00:00 2001 From: David Taylor Date: Tue, 11 Jan 2022 18:43:13 +0000 Subject: [PATCH] DEV: Use Ember router to determine currentURL This means that our DiscourseURL logic will work consistently in tests, where `window.location` doesn't get updated. To make it work properly, our `replaceState` implementation needed to be updated so that it writes the new URL to Ember's router, rather than bypassing the router and going straight to the `location` API. A couple of tests needed updating following this fix: - the composer-test was asserting that the new reply should be missing from the DOM... when really it **should** be in the DOM, and this fix to the test environment makes it so - the topic-test was making a fake topic fixture based on the data from a topic with a different id. This was causing the topic route to get confused, and 'fix' the currentURL. This commit updates it to use a fixture with consistent data. This commit also removes the feature detection of `window.history`. It's feature-detected within `discourse-location`. Plus, we don't support any browsers without it. --- .../javascripts/discourse/app/lib/url.js | 18 ++++++------------ .../tests/acceptance/composer-test.js | 2 +- .../discourse/tests/acceptance/topic-test.js | 14 +++++--------- .../discourse/tests/unit/lib/url-test.js | 5 +++++ 4 files changed, 17 insertions(+), 22 deletions(-) diff --git a/app/assets/javascripts/discourse/app/lib/url.js b/app/assets/javascripts/discourse/app/lib/url.js index 1b2fd5b7bf4..f9bc2565ca2 100644 --- a/app/assets/javascripts/discourse/app/lib/url.js +++ b/app/assets/javascripts/discourse/app/lib/url.js @@ -178,22 +178,15 @@ const DiscourseURL = EmberObject.extend({ }); }, - // Browser aware replaceState. Will only be invoked if the browser supports it. replaceState(path) { - if ( - window.history && - window.history.pushState && - window.history.replaceState && - window.location.pathname !== path - ) { + if (this.router.currentURL !== path) { // Always use replaceState in the next runloop to prevent weird routes changing // while URLs are loading. For example, while a topic loads it sets `currentPost` // which triggers a replaceState even though the topic hasn't fully loaded yet! next(() => { - const location = this.get("router.location"); - if (location && location.replaceURL) { - location.replaceURL(path); - } + // Using the private `_routerMicrolib` is not ideal, but Ember doesn't provide + // any other way for us to do `history.replaceState` without a full transition + this.router._routerMicrolib.replaceURL(path); }); } }, @@ -249,7 +242,8 @@ const DiscourseURL = EmberObject.extend({ return this.replaceState(path); } - const oldPath = `${window.location.pathname}${window.location.search}`; + const oldPath = this.router.currentURL; + path = path.replace(/(https?\:)?\/\/[^\/]+/, ""); // Rewrite /my/* urls diff --git a/app/assets/javascripts/discourse/tests/acceptance/composer-test.js b/app/assets/javascripts/discourse/tests/acceptance/composer-test.js index 5496cff6e97..1b4fb5d3459 100644 --- a/app/assets/javascripts/discourse/tests/acceptance/composer-test.js +++ b/app/assets/javascripts/discourse/tests/acceptance/composer-test.js @@ -202,7 +202,7 @@ acceptance("Composer", function (needs) { await click("#reply-control button.create"); assert.strictEqual( queryAll(".cooked:last p").text(), - "If you use gettext format you could leverage Launchpad 13 translations and the community behind it." + "this is the content of my reply" ); }); diff --git a/app/assets/javascripts/discourse/tests/acceptance/topic-test.js b/app/assets/javascripts/discourse/tests/acceptance/topic-test.js index 5caa7455f2e..9a9903a20f3 100644 --- a/app/assets/javascripts/discourse/tests/acceptance/topic-test.js +++ b/app/assets/javascripts/discourse/tests/acceptance/topic-test.js @@ -586,19 +586,14 @@ acceptance("Navigating between topics", function (needs) { const firstPost = topicResponse.post_stream.posts[0]; firstPost.cooked += `\nLink 1`; firstPost.cooked += `\nLink 2`; - firstPost.cooked += `\nLink 3`; - firstPost.cooked += `\nLink 3`; + firstPost.cooked += `\nLink 3`; + firstPost.cooked += `\nLink 4`; firstPost.cooked += `\nLink to Post`; server.get("/t/280.json", () => helper.response(topicResponse)); server.get("/t/280/:post_number.json", () => helper.response(topicResponse) ); - - server.get("/t/281.json", () => helper.response(topicResponse)); - server.get("/t/281/:post_number.json", () => - helper.response(topicResponse) - ); }); test("clicking slug-less URLs within the same topic", async function (assert) { @@ -613,10 +608,11 @@ acceptance("Navigating between topics", function (needs) { test("clicking slug-less URLs to a different topic", async function (assert) { await visit("/t/-/280"); await click("a.diff-topic-slugless"); - assert.ok(currentURL().includes("/281")); + assert.ok(currentURL().includes("/28830")); + await visit("/t/-/280"); await click("a.diff-topic-slugless-post"); - assert.ok(currentURL().includes("/281")); + assert.ok(currentURL().includes("/28830")); }); test("clicking post URLs", async function (assert) { diff --git a/app/assets/javascripts/discourse/tests/unit/lib/url-test.js b/app/assets/javascripts/discourse/tests/unit/lib/url-test.js index cc11a7d6cbe..61ee568ec0a 100644 --- a/app/assets/javascripts/discourse/tests/unit/lib/url-test.js +++ b/app/assets/javascripts/discourse/tests/unit/lib/url-test.js @@ -77,6 +77,11 @@ module("Unit | Utility | url", function () { logIn(); const user = User.current(); + sinon.stub(DiscourseURL, "router").get(() => { + return { + currentURL: "/forum", + }; + }); sinon.stub(DiscourseURL, "handleURL"); DiscourseURL.routeTo("/my/messages"); assert.ok(