From af305366909fd712362d727d0fa92f380d71d4cd Mon Sep 17 00:00:00 2001 From: David Taylor Date: Mon, 25 Sep 2023 14:56:06 +0100 Subject: [PATCH] DEV: Allow PluginOutlets to 'wrap' a core implementation (#23110) Our existing PluginOutlet system allows theme/plugin developers to easily insert new content into Discourse. Another common requirement is to **replace** existing content in Discourse. Previously this could be achieved either using template overrides, or by introducing new content via a PluginOutlet and then hiding the old implementation with CSS. Neither of these patterns are ideal from a maintainability or performance standpoint. This commit introduces a new mode for PluginOutlets. They can now be used to 'wrap' blocks of content in core. If a plugin/theme registers a connector for the outlet, then it will be rendered **instead of** the core implementation. If needed, outlets can use `{{yield}}` to render the core implementation inside their own implementation (e.g. to add a wrapper element). In this 'wrapper' mode, only one connector can be registered for each outlet. If more than one is registered, only one will be used, and an error will be printed to the console. To introduce a new PluginOutlet wrapper, this kind of thing can be added to a core template: ```hbs

This is the default core implementation: {{title}}

``` A plugin/theme can then register a connector for the `site-logo` outlet: ```hbs {{! connectors/site-logo/my-site-logo-override.hbs }}

This is the plugin implementation: {{@outletArgs.title}}

``` Care should be taken when introducing new wrapper PluginOutlets. We need to ensure that 1) They are properly sized. In general it's preferable for each outlet to wrap a small amount of core code, so that plugin/themes only need to re-implement what they want to change 2) The `@outletArgs` are carefully chosen. It may be tempting to pass through lots of core implementation into the outletArgs (or worse, use `this` to pass a reference to the wrapping component/controller). Doing this will significantly increase the API surface area, and make it hard to refactor core. Instead, we should aim to keep `@outletArgs` to a minimum, even if that means re-implementing some very simple things in themes/plugins. --- .../app/components/plugin-outlet.hbs | 18 ++- .../discourse/app/components/plugin-outlet.js | 22 ++- .../discourse/app/lib/plugin-connectors.js | 6 + .../components/plugin-outlet-test.js | 128 ++++++++++++++++++ 4 files changed, 166 insertions(+), 8 deletions(-) diff --git a/app/assets/javascripts/discourse/app/components/plugin-outlet.hbs b/app/assets/javascripts/discourse/app/components/plugin-outlet.hbs index acf2e087f05..852facad01f 100644 --- a/app/assets/javascripts/discourse/app/components/plugin-outlet.hbs +++ b/app/assets/javascripts/discourse/app/components/plugin-outlet.hbs @@ -4,7 +4,7 @@ But for now, this classic component wrapper takes care of the tagName. }} - {{#each this.connectors as |c|}} + {{#each (this.getConnectors) as |c|}} {{#if c.componentClass}} {{else if @defaultGlimmer}} @@ -24,11 +24,15 @@ {{else if this.connectorsExist}} {{! The modern path: no wrapper element = no classic component }} - {{#each this.connectors as |c|}} + {{#each (this.getConnectors hasBlock=(has-block)) as |c|}} {{#if c.componentClass}} - + {{yield}} {{else if @defaultGlimmer}} - + {{yield}} {{else}} + >{{yield}} {{/if}} + {{else}} + {{yield}} {{/each}} +{{else}} + {{yield}} {{/if}} \ No newline at end of file diff --git a/app/assets/javascripts/discourse/app/components/plugin-outlet.js b/app/assets/javascripts/discourse/app/components/plugin-outlet.js index 0fc1af240fb..48621322c81 100644 --- a/app/assets/javascripts/discourse/app/components/plugin-outlet.js +++ b/app/assets/javascripts/discourse/app/components/plugin-outlet.js @@ -10,13 +10,15 @@ import { helperContext } from "discourse-common/lib/helpers"; import deprecated from "discourse-common/lib/deprecated"; import { get } from "@ember/object"; import { cached } from "@glimmer/tracking"; +import { bind } from "discourse-common/utils/decorators"; +import { inject as service } from "@ember/service"; const PARENT_VIEW_DEPRECATION_MSG = "parentView should not be used within plugin outlets. Use the available outlet arguments, or inject a service which can provide the context you need."; const GET_DEPRECATION_MSG = "Plugin outlet context is no longer an EmberObject - using `get()` is deprecated."; const TAG_NAME_DEPRECATION_MSG = - "The `tagName` argument to PluginOutlet is deprecated. If a wrapper element is required, define it manually around the outlet call."; + "The `tagName` argument to PluginOutlet is deprecated. If a wrapper element is required, define it manually around the outlet call. Using tagName will prevent wrapper PluginOutlets from functioning correctly."; const ARGS_DEPRECATION_MSG = "PluginOutlet arguments should now be passed using `@outletArgs=` instead of `@args=`"; @@ -48,6 +50,8 @@ const ARGS_DEPRECATION_MSG = **/ export default class PluginOutletComponent extends GlimmerComponentWithDeprecatedParentView { + @service clientErrorHandler; + context = { ...helperContext(), get parentView() { @@ -79,12 +83,24 @@ export default class PluginOutletComponent extends GlimmerComponentWithDeprecate return result; } - get connectors() { - return renderedConnectorsFor( + @bind + getConnectors({ hasBlock }) { + const connectors = renderedConnectorsFor( this.args.name, this.outletArgsWithDeprecations, this.context ); + if (connectors.length > 1 && hasBlock) { + const message = `Multiple connectors were registered for the ${this.args.name} outlet. Using the first.`; + this.clientErrorHandler.displayErrorNotice(message); + // eslint-disable-next-line no-console + console.error( + message, + connectors.map((c) => c.humanReadableName) + ); + return [connectors[0]]; + } + return connectors; } get connectorsExist() { diff --git a/app/assets/javascripts/discourse/app/lib/plugin-connectors.js b/app/assets/javascripts/discourse/app/lib/plugin-connectors.js index 8960ab848b7..dfa0d0e6b71 100644 --- a/app/assets/javascripts/discourse/app/lib/plugin-connectors.js +++ b/app/assets/javascripts/discourse/app/lib/plugin-connectors.js @@ -99,6 +99,12 @@ class ConnectorInfo { } } + get humanReadableName() { + return `${this.outletName}/${this.connectorName} (${ + this.classModule || this.templateModule + })`; + } + #buildComponentClass() { const klass = this.connectorClass; if (klass && hasInternalComponentManager(klass)) { diff --git a/app/assets/javascripts/discourse/tests/integration/components/plugin-outlet-test.js b/app/assets/javascripts/discourse/tests/integration/components/plugin-outlet-test.js index d880d2ad88a..96043470ea4 100644 --- a/app/assets/javascripts/discourse/tests/integration/components/plugin-outlet-test.js +++ b/app/assets/javascripts/discourse/tests/integration/components/plugin-outlet-test.js @@ -11,6 +11,7 @@ import Component from "@glimmer/component"; import templateOnly from "@ember/component/template-only"; import { withSilencedDeprecationsAsync } from "discourse-common/lib/deprecated"; import { setComponentTemplate } from "@glimmer/manager"; +import sinon from "sinon"; const TEMPLATE_PREFIX = "discourse/plugins/some-plugin/templates/connectors"; const CLASS_PREFIX = "discourse/plugins/some-plugin/connectors"; @@ -69,6 +70,15 @@ module("Integration | Component | plugin-outlet", function (hooks) { `${TEMPLATE_PREFIX}/test-name/conditional-render`, hbs`I only render sometimes` ); + + registerTemporaryModule( + `${TEMPLATE_PREFIX}/outlet-with-default/my-connector`, + hbs`Plugin implementation{{#if @outletArgs.yieldCore}} {{yield}}{{/if}}` + ); + registerTemporaryModule( + `${TEMPLATE_PREFIX}/outlet-with-default/clashing-connector`, + hbs`This will override my-connector and raise an error` + ); }); test("Renders a template into the outlet", async function (assert) { @@ -103,6 +113,124 @@ module("Integration | Component | plugin-outlet", function (hooks) { ); }); + module( + "as a wrapper around a default core implementation", + function (innerHooks) { + innerHooks.beforeEach(function () { + this.consoleErrorStub = sinon.stub(console, "error"); + + this.set("shouldDisplay", false); + this.set("yieldCore", false); + this.set("enableClashingConnector", false); + + extraConnectorClass("outlet-with-default/my-connector", { + shouldRender(args) { + return args.shouldDisplay; + }, + }); + + extraConnectorClass("outlet-with-default/clashing-connector", { + shouldRender(args) { + return args.enableClashingConnector; + }, + }); + + this.template = hbs` + + Core implementation + + `; + }); + + test("Can act as a wrapper around core implementation", async function (assert) { + await render(this.template); + + assert.dom(".result").hasText("Core implementation"); + + this.set("shouldDisplay", true); + await settled(); + + assert.dom(".result").hasText("Plugin implementation"); + + this.set("yieldCore", true); + await settled(); + + assert + .dom(".result") + .hasText("Plugin implementation Core implementation"); + + assert.strictEqual( + this.consoleErrorStub.callCount, + 0, + "no errors in console" + ); + }); + + test("clashing connectors for regular users", async function (assert) { + await render(this.template); + + this.set("shouldDisplay", true); + this.set("enableClashingConnector", true); + await settled(); + + assert.strictEqual( + this.consoleErrorStub.callCount, + 1, + "clash error reported to console" + ); + + assert.true( + this.consoleErrorStub + .getCall(0) + .args[0].includes("Multiple connectors"), + "console error includes message about multiple connectors" + ); + + assert + .dom(".broken-theme-alert-banner") + .doesNotExist("Banner is not shown to regular users"); + }); + + test("clashing connectors for admins", async function (assert) { + this.set("currentUser.admin", true); + await render(this.template); + + this.set("shouldDisplay", true); + this.set("enableClashingConnector", true); + await settled(); + + assert.strictEqual( + this.consoleErrorStub.callCount, + 1, + "clash error reported to console" + ); + + assert.true( + this.consoleErrorStub + .getCall(0) + .args[0].includes("Multiple connectors"), + "console error includes message about multiple connectors" + ); + + assert + .dom(".broken-theme-alert-banner") + .exists("Error banner is shown to admins"); + }); + } + ); + + test("Renders wrapped implementation if no connectors are registered", async function (assert) { + await render( + hbs` + + Core implementation + + ` + ); + + assert.dom(".result").hasText("Core implementation"); + }); + test("Reevaluates shouldRender for argument changes", async function (assert) { this.set("shouldDisplay", false); await render(