DEV: Improve api.decorateCookedElement implementation (#23543)

Previously, calling `decorateCookedElement` would re-open a number of components and introduce new event listeners. This kind of thing cannot be undone, and so we were forced to introduce the unique 'id' parameter. If a given decorator id had already been applied, we would skip re-applying it. This helped, but it was still problematic because all tests would be using the callback which was registered in the first test. If its closure had any references to the ApplicationInstance, then those references would be destroyed and useless in future tests.

This commit switches strategy to use `appEvents` instead of `klass.reopen`. This is a much more obvious system and, since appEvent registrations are reset for every ApplicationInstance, we can drop the requirement for unique ids on `decorateCookedElement` calls. The callback used will always be the one registered against the current ApplicationInstance.

This commit also updates our `wrapWithErrorHandler` implementation so that it throws errors in tests. This ensures that errors are not silently swallowed in CI.

app/assets/javascripts/discourse/app/components/composer-editor.js

@@ -948,7 +948,7 @@ export default Component.extend(ComposerUploadUppy, {
       preview.addEventListener("click", this._handleImageScaleButtonClick);
       this._registerImageAltTextButtonClick(preview);
 
-      this.trigger("previewRefreshed", preview);
+      this.appEvents.trigger("decorate-non-stream-cooked-element", preview);
       this.afterRefresh($preview);
     },
   },

app/assets/javascripts/discourse/app/components/discourse-banner.js

@@ -42,4 +42,8 @@ export default Component.extend({
       });
     }
   },
+
+  didInsertElement() {
+    this.appEvents.trigger("decorate-non-stream-cooked-element", this.element);
+  },
 });

app/assets/javascripts/discourse/app/components/user-stream.js

@@ -40,6 +40,7 @@ export default Component.extend(LoadMore, {
       return ClickTrack.trackClick(e, this.siteSettings);
     });
     this._updateLastDecoratedElement();
+    this.appEvents.trigger("decorate-non-stream-cooked-element", this.element);
   }),
 
   // This view is being removed. Shut down operations
@@ -130,6 +131,7 @@ export default Component.extend(LoadMore, {
         let element = this._lastDecoratedElement?.nextElementSibling;
         while (element) {
           this.trigger("user-stream:new-item-inserted", element);
+          this.appEvents.trigger("decorate-non-stream-cooked-element", element);
           element = element.nextElementSibling;
         }
         this._updateLastDecoratedElement();

app/assets/javascripts/discourse/app/lib/plugin-api.js

@@ -1,5 +1,5 @@
 import I18n from "I18n";
-import ComposerEditor, {
+import {
   addComposerUploadHandler,
   addComposerUploadMarkdownResolver,
   addComposerUploadPreProcessor,
@@ -41,7 +41,6 @@ import {
 import Composer, {
   registerCustomizationCallback,
 } from "discourse/models/composer";
-import DiscourseBanner from "discourse/components/discourse-banner";
 import KeyboardShortcuts from "discourse/lib/keyboard-shortcuts";
 import Sharing from "discourse/lib/sharing";
 import { addAdvancedSearchOptions } from "discourse/components/search-advanced-options";
@@ -83,7 +82,6 @@ import { getOwner } from "discourse-common/lib/get-owner";
 import { h } from "virtual-dom";
 import { includeAttributes } from "discourse/lib/transform-post";
 import { modifySelectKit } from "select-kit/mixins/plugin-api";
-import { on } from "@ember/object/evented";
 import { registerCustomAvatarHelper } from "discourse/helpers/user-avatar";
 import { registerCustomPostMessageCallback as registerCustomPostMessageCallback1 } from "discourse/controllers/topic";
 import {
@@ -129,6 +127,7 @@ import { registerFullPageSearchType } from "discourse/controllers/full-page-sear
 import { registerHashtagType } from "discourse/lib/hashtag-autocomplete";
 import { _addBulkButton } from "discourse/components/modal/topic-bulk-actions";
 import { addBeforeAuthCompleteCallback } from "discourse/instance-initializers/auth-complete";
+import { isTesting } from "discourse-common/config/environment";
 
 // If you add any methods to the API ensure you bump up the version number
 // based on Semantic Versioning 2.0.0. Please update the changelog at
@@ -167,6 +166,9 @@ function wrapWithErrorHandler(func, messageKey) {
           detail: { messageKey, error },
         })
       );
+      if (isTesting()) {
+        throw error;
+      }
       return;
     }
   };
@@ -368,13 +370,9 @@ class PluginApi {
    *
    * ```
    * api.decorateCookedElement(
-   *   elem => { elem.style.backgroundColor = 'yellow' },
-   *   { id: 'yellow-decorator' }
+   *   elem => { elem.style.backgroundColor = 'yellow' }
    * );
    * ```
-   *
-   * NOTE: To avoid memory leaks, it is highly recommended to pass a unique `id` parameter.
-   * You will receive a warning if you do not.
    **/
   decorateCookedElement(callback, opts) {
     opts = opts || {};
@@ -384,12 +382,7 @@ class PluginApi {
     addDecorator(callback, { afterAdopt: !!opts.afterAdopt });
 
     if (!opts.onlyStream) {
-      decorate(ComposerEditor, "previewRefreshed", callback, opts.id);
-      decorate(DiscourseBanner, "didInsertElement", callback, opts.id);
-      ["didInsertElement", "user-stream:new-item-inserted"].forEach((event) => {
-        const klass = this.container.factoryFor("component:user-stream").class;
-        decorate(klass, event, callback, opts.id);
-      });
+      this.onAppEvent("decorate-non-stream-cooked-element", callback);
     }
   }
 
@@ -2552,42 +2545,3 @@ export function withPluginApi(version, apiCodeCallback, opts) {
     return apiCodeCallback(api, opts);
   }
 }
-
-let _decorateId = 0;
-let _decorated = new WeakMap();
-
-function decorate(klass, evt, cb, id) {
-  if (!id) {
-    // eslint-disable-next-line no-console
-    console.warn(
-      consolePrefix(),
-      "`decorateCooked` should be supplied with an `id` option to avoid memory leaks in test mode. The id will be used to ensure the decorator is only applied once."
-    );
-  } else {
-    if (!_decorated.has(klass)) {
-      _decorated.set(klass, new Set());
-    }
-    id = `${id}:${evt}`;
-    let set = _decorated.get(klass);
-    if (set.has(id)) {
-      return;
-    }
-    set.add(id);
-  }
-
-  const mixin = {};
-  let name = `_decorate_${_decorateId++}`;
-
-  if (id) {
-    name += `_${id.replaceAll(/\W/g, "_")}`;
-  }
-
-  mixin[name] = on(evt, function (elem) {
-    elem = elem || this.element;
-    if (elem) {
-      cb(elem);
-    }
-  });
-
-  klass.reopen(mixin);
-}

app/assets/javascripts/discourse/tests/acceptance/topic-post-decorate-cooked-test.js

@@ -47,7 +47,7 @@ acceptance("Acceptance | decorateCookedElement", function () {
             hbs` with more content from glimmer`
           );
         },
-        { id: "render-glimmer-test" }
+        { id: "render-glimmer-test", onlyStream: true }
       );
     });