-
Notifications
You must be signed in to change notification settings - Fork 8.6k
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
Conversation
c0bd0b7
to
3adb1cc
Compare
@@ -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|}} |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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!) 🚀
There was a problem hiding this comment.
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?
a534ea9
to
b31808c
Compare
@@ -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|}} |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<PluginOutlet @name="outlet-with-default" @outletArgs={{hash shouldDisplay=this.shouldDisplay yieldCore=this.yieldCore enableClashingConnector=this.enableClashingConnector}}> | ||
<span class='result'>Core implementation</span> | ||
</PluginOutlet> |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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! 😃)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pretty neat indeed
assert.strictEqual( | ||
consoleErrorStub.callCount, | ||
1, | ||
"clash error reported to console" | ||
); |
There was a problem hiding this comment.
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?
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)
…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)
b31808c
to
023605d
Compare
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.
023605d
to
c122ece
Compare
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 |
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:
A plugin/theme can then register a connector for the
site-logo
outlet:Care should be taken when introducing new wrapper PluginOutlets. We need to ensure that
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
The
@outletArgs
are carefully chosen. It may be tempting to pass through lots of core implementation into the outletArgs (or worse, usethis
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.