From b7cafdc07fa505436eea973d8ee9aa4475118c59 Mon Sep 17 00:00:00 2001 From: David Taylor Date: Mon, 30 Oct 2023 11:07:17 +0000 Subject: [PATCH] DEV: Deprecate `api.decoratePluginOutlet` (#24145) This API is not used by any known themes/plugins, and is problematic for a few reasons - It doesn't work on modern plugin connectors which have no wrapper element - Making modifications to Ember-rendered DOM elements can lead to catastrophic and surprising errors - It doesn't re-run when arguments to a plugin outlet change This commit adds the deprecation notice, and refactors the tests so that they do not rely on any real core plugin outlets --- .../discourse/app/lib/plugin-api.js | 8 +++ .../plugin-outlet-decorator-test.js | 68 ------------------- .../plugin-outlet-decorator-test.gjs | 65 ++++++++++++++++++ 3 files changed, 73 insertions(+), 68 deletions(-) delete mode 100644 app/assets/javascripts/discourse/tests/acceptance/plugin-outlet-decorator-test.js create mode 100644 app/assets/javascripts/discourse/tests/integration/components/plugin-outlet-decorator-test.gjs diff --git a/app/assets/javascripts/discourse/app/lib/plugin-api.js b/app/assets/javascripts/discourse/app/lib/plugin-api.js index 4a353a724e0..18ad15ca488 100644 --- a/app/assets/javascripts/discourse/app/lib/plugin-api.js +++ b/app/assets/javascripts/discourse/app/lib/plugin-api.js @@ -1610,7 +1610,9 @@ class PluginApi { addDocumentTitleCounter(counterFunction) { addPluginDocumentTitleCounter(counterFunction); } + /** + * * Used for decorating the rendered HTML content of a plugin-outlet after it's been rendered * * `callback` will be called when it is time to decorate it. @@ -1628,8 +1630,14 @@ class PluginApi { * ); * ``` * + * @deprecated because modifying an Ember-rendered DOM tree can lead to very unexpected errors. Use CSS or plugin outlet connectors instead + * **/ decoratePluginOutlet(outletName, callback, opts) { + deprecated( + "decoratePluginOutlet is deprecated because modifying an Ember-rendered DOM tree can lead to very unexpected errors. Use CSS or plugin outlet connectors instead", + { id: "discourse.decorate-plugin-outlet" } + ); addPluginOutletDecorator(outletName, callback, opts || {}); } diff --git a/app/assets/javascripts/discourse/tests/acceptance/plugin-outlet-decorator-test.js b/app/assets/javascripts/discourse/tests/acceptance/plugin-outlet-decorator-test.js deleted file mode 100644 index 1c2d986c716..00000000000 --- a/app/assets/javascripts/discourse/tests/acceptance/plugin-outlet-decorator-test.js +++ /dev/null @@ -1,68 +0,0 @@ -import { visit } from "@ember/test-helpers"; -import { hbs } from "ember-cli-htmlbars"; -import { test } from "qunit"; -import { withPluginApi } from "discourse/lib/plugin-api"; -import { - acceptance, - exists, - queryAll, -} from "discourse/tests/helpers/qunit-helpers"; -import { registerTemporaryModule } from "../helpers/temporary-module-helper"; - -const PREFIX = "discourse/plugins/some-plugin/templates/connectors"; - -acceptance("Plugin Outlet - Decorator", function (needs) { - needs.user(); - - needs.hooks.beforeEach(() => { - registerTemporaryModule( - `${PREFIX}/discovery-list-container-top/foo`, - hbs`FOO` - ); - registerTemporaryModule( - `${PREFIX}/discovery-list-container-top/bar`, - hbs`BAR` - ); - - withPluginApi("0.8.38", (api) => { - api.decoratePluginOutlet( - "discovery-list-container-top", - (elem, args) => { - if (elem.classList.contains("foo")) { - elem.style.backgroundColor = "yellow"; - - if (args.category) { - elem.classList.add("in-category"); - } else { - elem.classList.remove("in-category"); - } - } - }, - { id: "yellow-decorator" } - ); - }); - }); - - test("Calls the plugin callback with the rendered outlet", async function (assert) { - await visit("/"); - - const fooConnector = queryAll( - ".discovery-list-container-top-outlet.foo " - )[0]; - const barConnector = queryAll( - ".discovery-list-container-top-outlet.bar " - )[0]; - - assert.ok(exists(fooConnector)); - assert.strictEqual(fooConnector.style.backgroundColor, "yellow"); - assert.strictEqual(barConnector.style.backgroundColor, ""); - - await visit("/c/bug"); - - assert.ok(fooConnector.classList.contains("in-category")); - - await visit("/"); - - assert.notOk(fooConnector.classList.contains("in-category")); - }); -}); diff --git a/app/assets/javascripts/discourse/tests/integration/components/plugin-outlet-decorator-test.gjs b/app/assets/javascripts/discourse/tests/integration/components/plugin-outlet-decorator-test.gjs new file mode 100644 index 00000000000..9bc58a7b03b --- /dev/null +++ b/app/assets/javascripts/discourse/tests/integration/components/plugin-outlet-decorator-test.gjs @@ -0,0 +1,65 @@ +import { hash } from "@ember/helper"; +import { render } from "@ember/test-helpers"; +import { hbs } from "ember-cli-htmlbars"; +import { module, test } from "qunit"; +import PluginOutlet from "discourse/components/plugin-outlet"; +import { withPluginApi } from "discourse/lib/plugin-api"; +import { setupRenderingTest } from "discourse/tests/helpers/component-test"; +import { query } from "discourse/tests/helpers/qunit-helpers"; +import { withSilencedDeprecations } from "discourse-common/lib/deprecated"; +import { registerTemporaryModule } from "../../helpers/temporary-module-helper"; + +const PREFIX = "discourse/plugins/some-plugin/templates/connectors"; + +module("Plugin Outlet - Decorator", function (hooks) { + setupRenderingTest(hooks); + + hooks.beforeEach(() => { + registerTemporaryModule(`${PREFIX}/my-outlet-name/foo`, hbs`FOO`); + registerTemporaryModule(`${PREFIX}/my-outlet-name/bar`, hbs`BAR`); + + withPluginApi("0.8.38", (api) => { + withSilencedDeprecations("discourse.decorate-plugin-outlet", () => { + api.decoratePluginOutlet( + "my-outlet-name", + (elem, args) => { + if (elem.classList.contains("foo")) { + elem.style.backgroundColor = "yellow"; + elem.classList.toggle("has-value", !!args.value); + } + }, + { id: "yellow-decorator" } + ); + }); + }); + }); + + test("Calls the plugin callback with the rendered outlet", async function (assert) { + await render(); + + const fooConnector = query(".my-outlet-name-outlet.foo"); + const barConnector = query(".my-outlet-name-outlet.bar"); + + assert.dom(fooConnector).exists(); + assert.strictEqual(fooConnector.style.backgroundColor, "yellow"); + assert.strictEqual(barConnector.style.backgroundColor, ""); + + await render(); + + assert.dom(".my-outlet-name-outlet.foo").hasClass("has-value"); + + await render(); + + assert.dom(".my-outlet-name-outlet.foo").doesNotHaveClass("has-value"); + }); +});