Discource-C
/
discourse
mirror of https://github.com/discourse/discourse.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

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
<PluginOutlet @name="site-logo" @defaultGlimmer={{true}} @outletArgs={{hash title=title}}>
  <h1>This is the default core implementation: {{title}}</h1>
</PluginOutlet>
```

A plugin/theme can then register a connector for the `site-logo` outlet:

```hbs
{{! connectors/site-logo/my-site-logo-override.hbs }}
<h2>This is the plugin implementation: {{@outletArgs.title}}</h2>
```

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.
This commit is contained in:
David Taylor 2023-09-25 14:56:06 +01:00 committed by GitHub
parent 88ec2320dc
commit af30536690
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
4 changed files with 166 additions and 8 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

18
app/assets/javascripts/discourse/app/components/plugin-outlet.hbs
View File

@ -4,7 +4,7 @@
But for now, this classic component wrapper takes care of the tagName.
}}
<this.wrapperComponent @tagName={{@tagName}}>
{{#each this.connectors as |c|}}
{{#each (this.getConnectors) as |c|}}
{{#if c.componentClass}}
<c.componentClass @outletArgs={{this.outletArgsWithDeprecations}} />
{{else if @defaultGlimmer}}
@ -24,11 +24,15 @@
</this.wrapperComponent>
{{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}}
<c.componentClass @outletArgs={{this.outletArgsWithDeprecations}} />
<c.componentClass
@outletArgs={{this.outletArgsWithDeprecations}}
>{{yield}}</c.componentClass>
{{else if @defaultGlimmer}}
<c.templateOnly @outletArgs={{this.outletArgsWithDeprecations}} />
<c.templateOnly
@outletArgs={{this.outletArgsWithDeprecations}}
>{{yield}}</c.templateOnly>
{{else}}
<PluginConnector
@connector={{c}}
@ -38,7 +42,11 @@
@class={{c.classicClassNames}}
@tagName={{or @connectorTagName ""}}
@layout={{c.template}}
/>
>{{yield}}</PluginConnector>
{{/if}}
{{else}}
{{yield}}
{{/each}}
{{else}}
{{yield}}
{{/if}}

22
app/assets/javascripts/discourse/app/components/plugin-outlet.js
View File

@ -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() {

6
app/assets/javascripts/discourse/app/lib/plugin-connectors.js
View File

@ -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)) {

128
app/assets/javascripts/discourse/tests/integration/components/plugin-outlet-test.js
View File

@ -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`<span class="conditional-render">I only render sometimes</span>`
);
registerTemporaryModule(
`${TEMPLATE_PREFIX}/outlet-with-default/my-connector`,
hbs`<span class='result'>Plugin implementation{{#if @outletArgs.yieldCore}} {{yield}}{{/if}}</span>`
);
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`
<PluginOutlet @name="outlet-with-default" @outletArgs={{hash shouldDisplay=this.shouldDisplay yieldCore=this.yieldCore enableClashingConnector=this.enableClashingConnector}}>
<span class='result'>Core implementation</span>
</PluginOutlet>
`;
});
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`
<PluginOutlet @name="outlet-with-no-registrations">
<span class='result'>Core implementation</span>
</PluginOutlet>
`
);
assert.dom(".result").hasText("Core implementation");
});
test("Reevaluates shouldRender for argument changes", async function (assert) {
this.set("shouldDisplay", false);
await render(