From 8efb787d8758cbe2ddc6a42b79cbdfa0e0b1909a Mon Sep 17 00:00:00 2001 From: David Taylor Date: Mon, 13 Feb 2023 09:43:16 +0000 Subject: [PATCH] DEV: Introduce support for Glimmer PluginOutlet connectors (#20108) Previously, all plugin connector templates would be rendered using the PluginConnector classic component definition. This commit introduces three key changes: 1. PluginOutlets can be passed `@defaultGlimmer={{true}}`, which will cause all connectors to be rendered as highly-performant 'template only glimmer components'. For now, to avoid breaking backwards compatibility, this is only intended for use on newly introduced PluginOutlets. 2. Connector js files can now directly export component definitions. This allows connectors on existing outlets to start using Glimmer components (template-only, or otherwise) straight away. It also makes it much more ergonomic to introduce custom logic to outlets. `shouldRender` continues to be supported (as a static class method). 3. Outlet arguments are now made available as `@outletArgs` in classic, glimmer and template-only-glimmer connectors. In glimmer and template-only-glimmer connectors, this is the only way to access the outlet's arguments. In classic connectors, the old methods still function - `@outletArgs` exists as a path for incremental migration --- .../app/components/plugin-connector.js | 23 ++- .../discourse/app/lib/plugin-connectors.js | 107 ++++++++++--- .../templates/components/plugin-outlet.hbs | 44 ++++-- .../components/plugin-outlet-test.js | 143 ++++++++++++++++++ 4 files changed, 271 insertions(+), 46 deletions(-) diff --git a/app/assets/javascripts/discourse/app/components/plugin-connector.js b/app/assets/javascripts/discourse/app/components/plugin-connector.js index 42f829efbfc..5ae2459f6dd 100644 --- a/app/assets/javascripts/discourse/app/components/plugin-connector.js +++ b/app/assets/javascripts/discourse/app/components/plugin-connector.js @@ -20,9 +20,6 @@ export default Component.extend({ init() { this._super(...arguments); - const connector = this.connector; - this.set("layoutName", connector.templateName); - const args = this.args || {}; Object.keys(args).forEach((key) => { defineProperty( @@ -50,15 +47,17 @@ export default Component.extend({ ); }); - const connectorClass = this.get("connector.connectorClass"); - this.set("actions", connectorClass.actions); + const connectorClass = this.connector.connectorClass; + this.set("actions", connectorClass?.actions); - for (const [name, action] of Object.entries(this.actions)) { - this.set(name, action.bind(this)); + if (this.actions) { + for (const [name, action] of Object.entries(this.actions)) { + this.set(name, action.bind(this)); + } } const merged = buildArgsWithDeprecations(args, deprecatedArgs); - connectorClass.setupComponent.call(this, merged, this); + connectorClass?.setupComponent?.call(this, merged, this); }, didReceiveAttrs() { @@ -77,13 +76,13 @@ export default Component.extend({ willDestroyElement() { this._super(...arguments); - const connectorClass = this.get("connector.connectorClass"); - connectorClass.teardownComponent.call(this, this); + const connectorClass = this.connector.connectorClass; + connectorClass?.teardownComponent?.call(this, this); }, send(name, ...args) { - const connectorClass = this.get("connector.connectorClass"); - const action = connectorClass.actions[name]; + const connectorClass = this.connector.connectorClass; + const action = connectorClass?.actions?.[name]; return action ? action.call(this, ...args) : this._super(name, ...args); }, }); diff --git a/app/assets/javascripts/discourse/app/lib/plugin-connectors.js b/app/assets/javascripts/discourse/app/lib/plugin-connectors.js index 0a9e07146c8..1315bbcc1b2 100644 --- a/app/assets/javascripts/discourse/app/lib/plugin-connectors.js +++ b/app/assets/javascripts/discourse/app/lib/plugin-connectors.js @@ -1,6 +1,12 @@ import { buildRawConnectorCache } from "discourse-common/lib/raw-templates"; import deprecated from "discourse-common/lib/deprecated"; import DiscourseTemplateMap from "discourse-common/lib/discourse-template-map"; +import { + getComponentTemplate, + hasInternalComponentManager, + setComponentTemplate, +} from "@glimmer/manager"; +import templateOnly from "@ember/component/template-only"; let _connectorCache; let _rawConnectorCache; @@ -18,13 +24,6 @@ export function extraConnectorClass(name, obj) { _extraConnectorClasses[name] = obj; } -const DefaultConnectorClass = { - actions: {}, - shouldRender: () => true, - setupComponent() {}, - teardownComponent() {}, -}; - function findOutlets(keys, callback) { keys.forEach(function (res) { const segments = res.split("/"); @@ -58,9 +57,20 @@ function findClass(outletName, uniqueName) { const id = `${outletName}/${uniqueName}`; let foundClass = _extraConnectorClasses[id] || _classPaths[id]; - return foundClass - ? Object.assign({}, DefaultConnectorClass, foundClass) - : DefaultConnectorClass; + return foundClass; +} + +/** + * Sets component template, ignoring errors if it's already set to the same template + */ +function safeSetComponentTemplate(template, component) { + try { + setComponentTemplate(template, component); + } catch (e) { + if (getComponentTemplate(component) !== template) { + throw e; + } + } } /** @@ -71,21 +81,77 @@ export function expireConnectorCache() { _connectorCache = null; } +class ConnectorInfo { + #componentClass; + #templateOnly; + + constructor(outletName, connectorName, connectorClass, template) { + this.outletName = outletName; + this.connectorName = connectorName; + this.connectorClass = connectorClass; + this.template = template; + } + + get componentClass() { + return (this.#componentClass ??= this.#buildComponentClass()); + } + + get templateOnly() { + return (this.#templateOnly ??= this.#buildTemplateOnlyClass()); + } + + get classicClassNames() { + return `${this.outletName}-outlet ${this.connectorName}`; + } + + #buildComponentClass() { + const klass = this.connectorClass; + if (klass && hasInternalComponentManager(klass)) { + safeSetComponentTemplate(this.template, klass); + this.#warnUnusableHooks(); + return klass; + } else { + return false; + } + } + + #buildTemplateOnlyClass() { + const component = templateOnly(); + setComponentTemplate(this.template, component); + this.#warnUnusableHooks(); + return component; + } + + #warnUnusableHooks() { + for (const methodName of [ + "actions", + "setupComponent", + "teardownComponent", + ]) { + if (this.connectorClass?.[methodName]) { + deprecated( + `actions, setupComponent and teardownComponent hooks cannot be used with Glimmer plugin outlets. Define a component class instead. (${this.outletName}/${this.connectorName}).`, + { id: "discourse.plugin-outlet-classic-hooks" } + ); + } + } + } +} + function buildConnectorCache() { _connectorCache = {}; findOutlets( DiscourseTemplateMap.keys(), - (outletName, resource, uniqueName) => { - _connectorCache[outletName] = _connectorCache[outletName] || []; + (outletName, resource, connectorName) => { + _connectorCache[outletName] ||= []; - _connectorCache[outletName].push({ - outletName, - templateName: resource, - template: require(DiscourseTemplateMap.resolve(resource)).default, - classNames: `${outletName}-outlet ${uniqueName}`, - connectorClass: findClass(outletName, uniqueName), - }); + const template = require(DiscourseTemplateMap.resolve(resource)).default; + const connectorClass = findClass(outletName, connectorName); + + _connectorCache[outletName].push( + new ConnectorInfo(outletName, connectorName, connectorClass, template) + ); } ); } @@ -99,7 +165,8 @@ export function connectorsFor(outletName) { export function renderedConnectorsFor(outletName, args, context) { return connectorsFor(outletName).filter((con) => { - return con.connectorClass.shouldRender(args, context); + const shouldRender = con.connectorClass?.shouldRender; + return !shouldRender || shouldRender(args, context); }); } diff --git a/app/assets/javascripts/discourse/app/templates/components/plugin-outlet.hbs b/app/assets/javascripts/discourse/app/templates/components/plugin-outlet.hbs index 900b9effa43..2639a58e88a 100644 --- a/app/assets/javascripts/discourse/app/templates/components/plugin-outlet.hbs +++ b/app/assets/javascripts/discourse/app/templates/components/plugin-outlet.hbs @@ -5,24 +5,40 @@ }} {{#each this.connectors as |c|}} - + {{#if c.componentClass}} + + {{else if @defaultGlimmer}} + + {{else}} + + {{/if}} {{/each}} {{else}} {{! The modern path: no wrapper element = no classic component }} {{#each this.connectors as |c|}} - + {{#if c.componentClass}} + + {{else if @defaultGlimmer}} + + {{else}} + + {{/if}} {{/each}} {{/if}} \ No newline at end of file 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 5d65655b8b3..a3cf9ff01b8 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 @@ -7,6 +7,9 @@ import { extraConnectorClass } from "discourse/lib/plugin-connectors"; import { hbs } from "ember-cli-htmlbars"; import { registerTemporaryModule } from "discourse/tests/helpers/temporary-module-helper"; import { getOwner } from "discourse-common/lib/get-owner"; +import Component from "@glimmer/component"; +import templateOnly from "@ember/component/template-only"; +import { withSilencedDeprecationsAsync } from "discourse-common/lib/deprecated"; const PREFIX = "discourse/plugins/some-plugin/templates/connectors"; @@ -147,3 +150,143 @@ module("Integration | Component | plugin-outlet", function (hooks) { ); }); }); + +module( + "Integration | Component | plugin-outlet | connector class definitions", + function (hooks) { + setupRenderingTest(hooks); + + hooks.beforeEach(function () { + registerTemporaryModule( + `${PREFIX}/test-name/my-connector`, + hbs`{{@outletArgs.hello}}{{this.hello}}` + ); + }); + + test("uses classic PluginConnector by default", async function (assert) { + await render( + hbs`` + ); + + assert.dom(".outletArgHelloValue").hasText("world"); + assert.dom(".thisHelloValue").hasText("world"); + }); + + test("uses templateOnly by default when @defaultGlimmer=true", async function (assert) { + await render( + hbs`` + ); + + assert.dom(".outletArgHelloValue").hasText("world"); + assert.dom(".thisHelloValue").hasText(""); // `this.` unavailable in templateOnly components + }); + + test("uses simple object if provided", async function (assert) { + this.set("someBoolean", true); + + extraConnectorClass("test-name/my-connector", { + shouldRender(args) { + return args.someBoolean; + }, + + setupComponent(args, component) { + component.reopen({ + get hello() { + return args.hello + " from setupComponent"; + }, + }); + }, + }); + + await render( + hbs`` + ); + + assert.dom(".outletArgHelloValue").hasText("world"); + assert.dom(".thisHelloValue").hasText("world from setupComponent"); + + this.set("someBoolean", false); + await settled(); + + assert.dom(".outletArgHelloValue").doesNotExist(); + }); + + test("ignores classic hooks for glimmer components", async function (assert) { + extraConnectorClass("test-name/my-connector", { + setupComponent(args, component) { + component.reopen({ + get hello() { + return args.hello + " from setupComponent"; + }, + }); + }, + }); + + await withSilencedDeprecationsAsync( + "discourse.plugin-outlet-classic-hooks", + async () => { + await render( + hbs`` + ); + } + ); + + assert.dom(".outletArgHelloValue").hasText("world"); + assert.dom(".thisHelloValue").hasText(""); + }); + + test("uses custom component class if provided", async function (assert) { + this.set("someBoolean", true); + + extraConnectorClass( + "test-name/my-connector", + class MyOutlet extends Component { + static shouldRender(args) { + return args.someBoolean; + } + + get hello() { + return this.args.outletArgs.hello + " from custom component"; + } + } + ); + + await render( + hbs`` + ); + + assert.dom(".outletArgHelloValue").hasText("world"); + assert.dom(".thisHelloValue").hasText("world from custom component"); + + this.set("someBoolean", false); + await settled(); + + assert.dom(".outletArgHelloValue").doesNotExist(); + }); + + test("uses custom templateOnly() if provided", async function (assert) { + this.set("someBoolean", true); + + extraConnectorClass( + "test-name/my-connector", + Object.assign(templateOnly(), { + shouldRender(args) { + return args.someBoolean; + }, + }) + ); + + await render( + hbs`` + ); + + assert.dom(".outletArgHelloValue").hasText("world"); + assert.dom(".thisHelloValue").hasText(""); // `this.` unavailable in templateOnly components + + this.set("someBoolean", false); + await settled(); + + assert.dom(".outletArgHelloValue").doesNotExist(); + }); + } +);