Skip to content

DEV: Allow PluginOutlets to 'wrap' a core implementation #23110

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Sep 25, 2023

Conversation

davidtaylorhq
Copy link
Member

@davidtaylorhq davidtaylorhq commented Aug 15, 2023

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:

<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:

{{! 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.

@davidtaylorhq davidtaylorhq force-pushed the poc-plugin-outlet-default branch from c0bd0b7 to 3adb1cc Compare August 18, 2023 11:52
@@ -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|}}
Copy link
Contributor

@jjaffeux jjaffeux Aug 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we need () here?

Is it more less or like calling a function? I think I saw something related to this weeks ago, but did forget about it, that's really nice

Copy link
Member Author

@davidtaylorhq davidtaylorhq Aug 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The brackets turn the simple property reference into a 'helper invocation'. And since plain old functions can now be helper yes, exactly as you said, it makes it a function invocation (with autotracking support!) 🚀

Copy link
Member

@ZogStriP ZogStriP Aug 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dumb question: what are "connectors" in this context?

@davidtaylorhq davidtaylorhq marked this pull request as ready for review August 21, 2023 12:39
@davidtaylorhq davidtaylorhq changed the title Proof of concept: plugin outlet with default implementation DEV: Allow PluginOutlets to 'wrap' a core implementation Aug 22, 2023
@davidtaylorhq davidtaylorhq force-pushed the poc-plugin-outlet-default branch 2 times, most recently from a534ea9 to b31808c Compare August 22, 2023 08:12
@@ -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|}}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is has-block coming from?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on lines 137 to 139
<PluginOutlet @name="outlet-with-default" @outletArgs={{hash shouldDisplay=this.shouldDisplay yieldCore=this.yieldCore enableClashingConnector=this.enableClashingConnector}}>
<span class='result'>Core implementation</span>
</PluginOutlet>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(off topic 😛) I wonder if we can make prettier-plugin-ember-template-tag work with hbs-tagged strings

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The answer is probably to "just" convert tests to gjs and use <template> tags - which do have prettier support.

I converted one test file to try it out: #23180 (I like it! 😃)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pretty neat indeed

Comment on lines 160 to 164
assert.strictEqual(
consoleErrorStub.callCount,
1,
"clash error reported to console"
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could have another error preventing the other to happen and making you think it's working correctly. Maybe we should try to check the actual message?

davidtaylorhq added a commit that referenced this pull request Sep 11, 2023
When using ember-template-tag (.gjs), templates are much more interlinked with the JS file they're defined in. Therefore, attempting to override their template with a 'non-strict-mode' template doesn't make sense, and will likely lead to problems.

This commit skips any such overrides, and introduces a clear console warning. In theme/plugin tests, an error will be thrown during app boot.

Going forward, we aim to provide alternative APIs to achieve the customizations people currently implement with template overrides. (e.g. #23110)
davidtaylorhq added a commit that referenced this pull request Sep 11, 2023
…23513)

When using ember-template-tag (.gjs), templates are much more interlinked with the JS file they're defined in. Therefore, attempting to override their template with a 'non-strict-mode' template doesn't make sense, and will likely lead to problems.

This commit skips any such overrides, and introduces a clear console warning. In theme/plugin tests, an error will be thrown during app boot.

Going forward, we aim to provide alternative APIs to achieve the customizations people currently implement with template overrides. (e.g. #23110)
@davidtaylorhq davidtaylorhq force-pushed the poc-plugin-outlet-default branch from b31808c to 023605d Compare September 12, 2023 12:20
@davidtaylorhq davidtaylorhq changed the base branch from main to theme-error-handler September 12, 2023 12:22
Base automatically changed from theme-error-handler to main September 12, 2023 13:07
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.
@davidtaylorhq davidtaylorhq force-pushed the poc-plugin-outlet-default branch from 023605d to c122ece Compare September 12, 2023 13:07
@davidtaylorhq davidtaylorhq merged commit af30536 into main Sep 25, 2023
@davidtaylorhq davidtaylorhq deleted the poc-plugin-outlet-default branch September 25, 2023 13:56
@discoursebot
Copy link

This pull request has been mentioned on Discourse Meta. There might be relevant details there:

https://meta.discourse.org/t/using-the-new-custom-homepage-feature/302496/9

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

6 participants