FIX: Fully re-render ads when navigating between pages (#188)

In the past, the ad plugin relied on two side-effects to achieve this behaviour: 1. Components being fully destroyed/rendered when navigating between pages. This stopped working when Discourse core moved to the more efficient 'loading slider' UI 2. The `listLoading` argument. This was an implementation detail of the old discovery routing infrastructure. Core recently overhauled this and removed the `listLoading` argument, because loading is now handled properly by the Ember router. Instead of these two properties, we can use the `currentRoute` property of Ember's router service to trigger changes when navigating between pages. A common `{{#each` trick is used to fully destroy/re-render components even if the ad network is unchanged.
discourse · Nov 7, 2023 · c88bb59 · c88bb59
1 parent 5ad841d
commit c88bb59
Show file tree

Hide file tree

Showing 11 changed files with 35 additions and 85 deletions.
diff --git a/assets/javascripts/discourse/components/ad-slot.hbs b/assets/javascripts/discourse/components/ad-slot.hbs
@@ -1,12 +1,13 @@
 {{#each adComponents as |adComponent|}}
-  {{component
-    adComponent
-    placement=placement
-    refreshOnChange=refreshOnChange
-    category=category
-    listLoading=listLoading
-    postNumber=postNumber
-    indexNumber=indexNumber
-    tagName=childTagName
-  }}
+  {{! Trick to force full destroy/re-render of component when route changes }}
+  {{#each (array this.router.currentRoute)}}
+    {{component
+      adComponent
+      placement=placement
+      category=category
+      postNumber=postNumber
+      indexNumber=indexNumber
+      tagName=childTagName
+    }}
+  {{/each}}
 {{/each}}
diff --git a/assets/javascripts/discourse/components/ad-slot.js b/assets/javascripts/discourse/components/ad-slot.js
@@ -1,6 +1,7 @@
 import EmberObject from "@ember/object";
+import { inject as service } from "@ember/service";
 import { isBlank } from "@ember/utils";
-import discourseComputed, { observes } from "discourse-common/utils/decorators";
+import discourseComputed from "discourse-common/utils/decorators";
 import AdComponent from "discourse/plugins/discourse-adplugin/discourse/components/ad-component";
 import {
   isNthPost,
@@ -173,7 +174,7 @@ export function slotContenders(
 }
 
 export default AdComponent.extend({
-  needsUpdate: false,
+  router: service(),
   tagName: "",
 
   /**
@@ -191,27 +192,14 @@ export default AdComponent.extend({
     );
   },
 
-  /**
-   * When house ads are configured to alternate with other ad networks, we
-   * need to trigger an update of which ad component is shown after
-   * navigating between topic lists or topics.
-   */
-  @observes("refreshOnChange")
-  changed() {
-    if (this.get("listLoading")) {
-      return;
-    }
-
-    // force adComponents to be recomputed
-    this.notifyPropertyChange("needsUpdate");
-  },
-
   /**
    * Returns a list of the names of ad components that should be rendered
    * in the given ad placement. It handles alternating between house ads
    * and other ad networks.
+   *
+   * Depends on `router.currentRoute` so that we refresh ads when navigating around.
    */
-  @discourseComputed("placement", "availableAdTypes", "needsUpdate")
+  @discourseComputed("placement", "availableAdTypes", "router.currentRoute")
   adComponents(placement, availableAdTypes) {
     if (
       !availableAdTypes.includes("house-ad") ||

diff --git a/assets/javascripts/discourse/components/adbutler-ad.js b/assets/javascripts/discourse/components/adbutler-ad.js
@@ -2,7 +2,7 @@ import { scheduleOnce } from "@ember/runloop";
 import RSVP from "rsvp";
 import loadScript from "discourse/lib/load-script";
 import { isTesting } from "discourse-common/config/environment";
-import discourseComputed, { observes } from "discourse-common/utils/decorators";
+import discourseComputed from "discourse-common/utils/decorators";
 import AdComponent from "discourse/plugins/discourse-adplugin/discourse/components/ad-component";
 
 let _loaded = false,
@@ -111,16 +111,6 @@ export default AdComponent.extend({
     scheduleOnce("afterRender", this, this._triggerAds);
   },
 
-  @observes("listLoading")
-  waitForLoad() {
-    if (this.get("adRequested")) {
-      return;
-    } // already requested that this ad unit be populated
-    if (!this.get("listLoading")) {
-      scheduleOnce("afterRender", this, this._triggerAds);
-    }
-  },
-
   @discourseComputed("currentUser.trust_level")
   showToTrustLevel(trustLevel) {
     return !(

diff --git a/assets/javascripts/discourse/components/google-adsense.js b/assets/javascripts/discourse/components/google-adsense.js
@@ -3,7 +3,7 @@ import { htmlSafe } from "@ember/template";
 import RSVP from "rsvp";
 import loadScript from "discourse/lib/load-script";
 import { isTesting } from "discourse-common/config/environment";
-import discourseComputed, { observes } from "discourse-common/utils/decorators";
+import discourseComputed from "discourse-common/utils/decorators";
 import AdComponent from "discourse/plugins/discourse-adplugin/discourse/components/ad-component";
 
 let _loaded = false,
@@ -163,23 +163,9 @@ export default AdComponent.extend({
       return;
     }
 
-    if (this.get("listLoading")) {
-      return;
-    }
-
     scheduleOnce("afterRender", this, this._triggerAds);
   },
 
-  @observes("listLoading")
-  waitForLoad() {
-    if (this.get("adRequested")) {
-      return;
-    } // already requested that this ad unit be populated
-    if (!this.get("listLoading")) {
-      scheduleOnce("afterRender", this, this._triggerAds);
-    }
-  },
-
   @discourseComputed("ad_width")
   isResponsive(adWidth) {
     return ["auto", "fluid"].includes(adWidth);

diff --git a/assets/javascripts/discourse/components/google-dfp-ad.js b/assets/javascripts/discourse/components/google-dfp-ad.js
@@ -242,7 +242,6 @@ export default AdComponent.extend({
   classNameBindings: ["adUnitClass"],
   classNames: ["google-dfp-ad"],
   loadedGoogletag: false,
-  refreshOnChange: null,
   lastAdRefresh: null,
   width: alias("size.width"),
   height: alias("size.height"),
@@ -353,7 +352,7 @@ export default AdComponent.extend({
 
   @on("didUpdate")
   updated() {
-    if (this.get("listLoading") || !this.shouldRefreshAd()) {
+    if (!this.shouldRefreshAd()) {
       return;
     }
 

diff --git a/assets/javascripts/discourse/components/house-ad.js b/assets/javascripts/discourse/components/house-ad.js
@@ -1,5 +1,5 @@
 import { isBlank } from "@ember/utils";
-import discourseComputed, { observes } from "discourse-common/utils/decorators";
+import discourseComputed from "discourse-common/utils/decorators";
 import AdComponent from "discourse/plugins/discourse-adplugin/discourse/components/ad-component";
 
 const adIndex = {
@@ -100,12 +100,7 @@ export default AdComponent.extend({
     }
   },
 
-  @observes("refreshOnChange")
   refreshAd() {
-    if (this.get("listLoading")) {
-      return;
-    }
-
     this.set("adHtml", this.chooseAdHtml());
   },
 
@@ -116,10 +111,6 @@ export default AdComponent.extend({
       return;
     }
 
-    if (this.get("listLoading")) {
-      return;
-    }
-
     if (adIndex.topic_list_top === null) {
       // start at a random spot in the ad inventory
       Object.keys(adIndex).forEach((placement) => {

diff --git a/assets/javascripts/discourse/connectors/after-topic-list-item/discourse-adplugin.hbs b/assets/javascripts/discourse/connectors/after-topic-list-item/discourse-adplugin.hbs
@@ -1,8 +1,6 @@
 {{ad-slot
   placement="topic-list-between"
-  refreshOnChange=listLoading
   category=category.slug
-  listLoading=listLoading
   indexNumber=index
   childTagName="td"
 }}
diff --git a/assets/javascripts/discourse/connectors/discovery-list-container-top/discourse-adplugin.hbs b/assets/javascripts/discourse/connectors/discovery-list-container-top/discourse-adplugin.hbs
@@ -1,6 +1 @@
-{{ad-slot
-  placement="topic-list-top"
-  refreshOnChange=listLoading
-  category=category.slug
-  listLoading=listLoading
-}}
+{{ad-slot placement="topic-list-top" category=category.slug}}
diff --git a/assets/javascripts/discourse/connectors/topic-above-post-stream/discourse-adplugin.hbs b/assets/javascripts/discourse/connectors/topic-above-post-stream/discourse-adplugin.hbs
@@ -1,5 +1 @@
-{{ad-slot
-  placement="topic-above-post-stream"
-  refreshOnChange=model.id
-  category=model.category.slug
-}}
+{{ad-slot placement="topic-above-post-stream" category=model.category.slug}}
diff --git a/assets/javascripts/discourse/connectors/topic-above-suggested/discourse-adplugin.hbs b/assets/javascripts/discourse/connectors/topic-above-suggested/discourse-adplugin.hbs
@@ -1,5 +1 @@
-{{ad-slot
-  placement="topic-above-suggested"
-  refreshOnChange=model.id
-  category=model.category.slug
-}}
+{{ad-slot placement="topic-above-suggested" category=model.category.slug}}
diff --git a/test/javascripts/acceptance/house-ad-test.js b/test/javascripts/acceptance/house-ad-test.js
@@ -2,6 +2,7 @@ import { visit } from "@ember/test-helpers";
 import { test } from "qunit";
 import {
   acceptance,
+  query,
   updateCurrentUser,
 } from "discourse/tests/helpers/qunit-helpers";
 
@@ -69,12 +70,21 @@ acceptance("House Ads", function (needs) {
     assert
       .dom(".h-topic-list")
       .exists({ count: 1 }, "it should render ad above topic list");
+    const originalTopAdElement = query(".h-topic-list");
 
-    await visit("/latest");
     assert
       .dom(".h-between-topic-list")
       .exists({ count: 5 }, "it should render 5 ads between topics");
 
+    await visit("/top");
+    const newTopAdElement = query(".h-topic-list");
+
+    assert.notStrictEqual(
+      originalTopAdElement,
+      newTopAdElement,
+      "ad is fully re-rendered when changing pages"
+    );
+
     await visit("/t/28830");
 
     assert