-
Notifications
You must be signed in to change notification settings - Fork 95
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
docs: Add info on how to use frontend plugin slots #233
base: nightly
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -308,6 +308,122 @@ In case you need to run additional instructions just before the build step you c | |
|
||
You can find more patches in the `patch catalog <#template-patch-catalog>`_ below. | ||
|
||
Using Frontend Plugin Slots | ||
~~~~~~~~~~~~~~~~~~~~~~~~~~~ | ||
|
||
The ``mfe-dockerfile-pre-npm-build`` patch is also suitable for taking advantage of the ``env.config.jsx`` mechanism, which lets you define and use frontend plugins. For instance, you can use it to modify the footer across all MFEs *without* having to maintain a fork of ``frontend-component-footer``. | ||
|
||
Start by following the Tutor plugin tutorial at the `Adding new templates <https://docs.tutor.edly.io/tutorials/plugin.html#adding-new-templates>`_ section. Instead of adding a whole new ``Dockerfile``, though, you'll just modify the exiting one. Your ``myplugin.py`` should look something like this: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see so many people adding double blank space after a dot: ". " Why is that? Is that some IDE feature? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you mean after the period at the end of the sentence? Until very recently, this used to be the standard for typography (in English), up to and including word-processed text using a computer. A significant portion of people, myself included, were trained to do so. There are discussions about why this was so, and it comes down to readability of monospaced text. If you're interested, the best write-up I can find is this repost of a since-lost blog post: Anyway, I'm certainly not going to die on this hill. If you prefer the standard for the Tutor codebase to be one space, I ain't arguing. :) |
||
|
||
.. code-block:: python | ||
|
||
import os | ||
from tutor import hooks | ||
|
||
# Add the template root folder | ||
template_folder = os.path.join(os.path.dirname(__file__), "templates") | ||
hooks.Filters.ENV_TEMPLATE_ROOTS.add_item(template_folder) | ||
|
||
# env.config.jsx files should live in `mfe/build/mfe`, which is also where the | ||
# tutor-mfe Dockerfile goes. This is so it's more straightforward to copy it | ||
# in, as seen below. | ||
Comment on lines
+327
to
+329
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I realize it's iffy to recommend a plugin put a file directly into one of tutor-mfe's own directories, but for some reason I couldn't get the Dockerfile to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In Indigo, I implemented a similar setup, but I have some concerns regarding the inclusion of Can we think of any workaround to update or change OR I am unsure that this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, I didn't look at how indigo does it. Let me check. But yeah, the fact multiple plugins may want to modify env.config.jsx is not something I took into account, here. |
||
hooks.Filters.ENV_TEMPLATE_TARGETS.add_item(("mfe/build/mfe", "plugins")) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that this approach is really confusing. Also, it doesn't allow multiple tutor plugins to add their own MFE plugins, which is a major limitation. I suggest an alternative, which is to provide a default, no-op env.config.jsx file with all the right plumbing already in place and a
Then we would include information about how to implement the "mfe-plugin-slots" patch. Would that work? Do you think it would be simpler to implement for plugin developers? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Something like this was actually my original intention, so I'm not opposed off the bat. But let me pose the reasons I went with a generic env.config.jsx recommendation instead. 1. The tradeoff in convenience for flexibility isn't particularly favorableThe boiler plate, while a little arcane (mostly because of that try/catch, which is hopefully going away soon) is not that big if compared with the plugin definitions themselves. (Yes, I know, we should do something about the syntax. See the next point. ;) ) But if we don't expose the whole file, we're also limiting how people can use it. It is just javascript, and in theory you could use it to do cool things such as have Counter-argument: the 2. Per-MFE configurationSlots like the header and footer are present in more than one MFE, so there should be a way for the operator to configure them in a different way for each. However, In this PR I suggest a way to tackle this with a single Counter-argument: So it's not just JSON. Are we now expecting operators to know Javascript? ;P 3. Plugin declaration is likely going to change over the next yearWe'll have "true" plugins relatively soon, where operators just declare the plugins they want, not having to know or decide where components go on each page. (Maybe not by Teak, but likely Ulmo.) If we introduce a specific Tutor patch, this could mean an API change/deprecation in Tutor itself, too. Counter-argument: frontend-base is going to require a lot of refactoring anyway, so we might as well make people's lives easier in the interim. As you can see by the counter-arguments, it's ultimately a product decision. I'll let you decide. :) Now that I know a single There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I just realized I didn't address one major point you bring up that @hinakhadim hammered home afterwards. That multiple plugins may want to modify env.config.jsx. I have to think some more about this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I just went over Indigo's The question I'm ruminating is: how do we let plugins do all that in a single file and still allow multiple plugins to coexist there? Maybe the answer is that we should not allow plugin authors to run wild with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Multiple plugins modifying The first thought that came to my mind was providing a way for plugins to "add their changes to the pile" so to speak. I tested the following import { DIRECT_PLUGIN, PLUGIN_OPERATIONS } from '@openedx/frontend-plugin-framework';
const config = {
pluginSlots: {
desktop_header_slot: {
keepDefault: false,
plugins: [
{
op: PLUGIN_OPERATIONS.Insert,
widget: {
id: 'custom_header_component_one',
type: DIRECT_PLUGIN,
RenderWidget: () => (
<h1>1</h1>
),
},
},
]
},
desktop_header_slot: {
keepDefault: false,
plugins: [
{
op: PLUGIN_OPERATIONS.Insert,
widget: {
id: 'custom_header_component_two',
type: DIRECT_PLUGIN,
RenderWidget: () => (
<h1>2</h1>
),
},
},
]
},
},
}
export default config; and found that FPF handles it somewhat gracefully - the second component (the one that renders In a world where each tutor plugin only touches one FPF slot this could be helpful - we could provide a way to "add changes to the pile" and have operators make sure the plugins are doing so in their preferred order. The obvious problem with that, however, is tutor plugins that utilize multiple FPF slots. If plugins A and B both replace the header and footer, and a site operator wants the header from A and footer from B this falls apart. I'll keep thinking about this. It is definitely not a simple problem. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the productive discussion and @brian-smith-tcril for testing. I’m currently facing a challenge with the scenario you mentioned: "If plugins A and B both replace the header and footer, and a site operator wants the header from A and the footer from B, this falls apart." There was a similar instance where a user needed to use both tutor-indigo and a second plugin for additional customizations. From what I understand, the priority of each plugin’s components is determined by the order of activation. If tutor-indigo is enabled first, followed by the second plugin, the components from the second plugin will have priority. ...
RUN npm install @edx/frontend-component-header@npm:indigo-header
RUN npm install @edx/frontend-component-footer@npm:indigo-footer
RUN npm install @edx/frontend-component-header@npm: custom-header
RUN npm install @edx/frontend-component-footer@npm: custom-footer
... Conversely, if the second plugin is enabled first, tutor-indigo components will take precedence (means we use indigo header/footer in react js project). ...
RUN npm install @edx/frontend-component-header@npm: custom-header
RUN npm install @edx/frontend-component-footer@npm: custom-footer
RUN npm install @edx/frontend-component-header@npm: indigo-header
RUN npm install @edx/frontend-component-footer@npm: indigo-footer
... In cases where the operator needs the header from tutor-indigo (assuming that user cannot alter plugin) and the footer from the custom plugin (Plugin B), they would need to adjust Plugin B to exclude its header component, keeping only the footer. This way, Plugin A (Indigo) provides the header, and Plugin B supplies the footer without conflicts. RUN npm install @edx/frontend-component-header@npm: indigo-header
RUN npm install @edx/frontend-component-footer@npm: indigo-footer
RUN npm install @edx/frontend-component-footer@npm: custom-footer Any further insights would be greatly appreciated! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
It is my understanding that this scenario is handled fully in #234 |
||
|
||
# This patch gets inserted into the tutor-mfe Dockerfile. It copies our | ||
# env.config.jsx into the root of each and every MFE before the npm build step. | ||
hooks.Filters.ENV_PATCHES.add_item(("mfe-dockerfile-pre-npm-build", | ||
"COPY env.config.jsx /openedx/app" | ||
)) | ||
|
||
As for ``env.config.jsx``, as noted above, it should be put into a ``templates/mfe/build/mfe`` subdirectory below ``myplugin.py``. This is so it's easy to ``COPY`` it into the Docker image. | ||
|
||
In order for ``env.config.jsx`` to work with all current MFEs (including ones that don't yet have any plugin slots), it needs to be defined in a particular way. This is what it should look like: | ||
|
||
.. code-block:: javascript | ||
|
||
/* We can't just assume FPF exists, as it's not declared as a dependency in all | ||
* MFEs. Therefore, we have to use dynamic imports to check if it's available. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Really? Do you have a specific MFE in mind? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The first build failure if you try to use the same (This will all go away once we're in frontend-base land, or sooner, if all MFEs have slots.) |
||
* | ||
* We also have to use a `try` block (as opposed to `.then()`), otherwise | ||
* Webpack won't treat the import as optional. Hence the async function. | ||
*/ | ||
Comment on lines
+344
to
+349
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note to reviewer: this particular workaround will no longer be necessary once all supported MFEs have at least one defined slot, forcing them to add There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And even more so with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @brian-smith-tcril , Thank you for the clarification on the role of frontend-base. To confirm my understanding for future development, frontend-base will function as a shell that eventually replaces the My anticipation is that, even with |
||
async function getConfig () { | ||
let config = {}; | ||
|
||
try { | ||
const { DIRECT_PLUGIN, PLUGIN_OPERATIONS } = await import('@openedx/frontend-plugin-framework'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the FPF is not available, then we can't use plugins, right? So, could we write instead something like:
(but with working code) I'm a bit bothered by the very large There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure, I'll try that - not sure how webpack is going to behave. Bundlers are weird with conditional imports. I'll just object to the console warning in the browser, though. I'm usually against browser console logging in production code. There is often little reason why an end user should be able to see debug messages. A warning if something's broken, sure, but a learner doesn't need to know about FPF. |
||
|
||
config = { | ||
pluginSlots: { | ||
footer_slot: { | ||
plugins: [ | ||
{ | ||
/* Hide the default footer. */ | ||
op: PLUGIN_OPERATIONS.Hide, | ||
widgetId: 'default_contents', | ||
}, | ||
{ | ||
/* Insert a custom footer. */ | ||
op: PLUGIN_OPERATIONS.Insert, | ||
widget: { | ||
id: 'custom_footer', | ||
type: DIRECT_PLUGIN, | ||
RenderWidget: () => ( | ||
<h1 style={{ "{{textAlign: 'center'}}" }}>This is the footer.</h1> | ||
regisb marked this conversation as resolved.
Show resolved
Hide resolved
|
||
), | ||
}, | ||
}, | ||
Comment on lines
+360
to
+375
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note to reviewer: I considered adding There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you please clarify your approach here? I may not have fully grasped the patches thing and the trade-offs you're mentioning regarding flexibility and boilerplate reduction. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @hinakhadim, sure, I replied more fully to Regis here: #233 (comment) |
||
] | ||
} | ||
} | ||
}; | ||
|
||
/* NPM allows us to introspect the package name at build time, which in turn | ||
* lets us tailor the configuration for individual MFEs. | ||
*/ | ||
if (process.env.npm_package_name == '@edx/frontend-app-profile') { | ||
config.pluginSlots.footer_slot.plugins[1].widget.RenderWidget = () => ( | ||
<h1 style={{ "{{textAlign: 'center'}}" }}>This is the profile MFE's footer.</h1> | ||
); | ||
} | ||
} catch { }; | ||
|
||
return config; | ||
} | ||
|
||
export default getConfig; | ||
|
||
As you can see, by using the ``process.env.npm_package_name`` variable you can use a single ``env.config.jsx`` to selectively apply configuration to different MFEs. | ||
|
||
If, on the other hand, you prefer to maintain entirely separate ``env.config.jsx`` files, one for each mfe, that can also be done. For that you can rely on the ``mfe-dockerfile-pre-npm-build-*`` patches. For instance: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we should suggest an alternate solution here, it will just confuse people. I'd rather have a single, well-documented approach that everyone is aligned on. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, no objections! |
||
|
||
.. code-block:: python | ||
|
||
hooks.Filters.ENV_PATCHES.add_items([ | ||
( | ||
"mfe-dockerfile-pre-npm-build", | ||
"COPY env.config.jsx /openedx/app" | ||
), | ||
( | ||
"mfe-dockerfile-pre-npm-build-account", | ||
"COPY env-account.config.jsx /openedx/app/env.config.jsx" | ||
), | ||
( | ||
"mfe-dockerfile-pre-npm-build-profile", | ||
"COPY env-profile.config.jsx /openedx/app/env.config.jsx" | ||
) | ||
]) | ||
|
||
In this case, there's a global ``env.config.jsx``, but it would get overwritten by the specific ones for the Account and Profile MFEs. Note that while they originally have different names, they should all still be copied to ``env.config.jsx``. | ||
|
||
You can find what slots a library or MFE supports by inspecting its ``src/plugin-slots`` directory on Github. For example: | ||
|
||
- `Header slots <https://github.com/openedx/frontend-component-header/tree/master/src/plugin-slots>`_ | ||
- `Learning MFE slots <https://github.com/openedx/frontend-app-learning/tree/master/src/plugin-slots>`_ | ||
- `Learner dashboard MFE slots <https://github.com/openedx/frontend-app-learner-dashboard/tree/master/src/plugin-slots>`_ | ||
|
||
For more information on how frontend plugin slots work, refer to the `Frontend Plugin Framework README <https://github.com/openedx/frontend-plugin-framework/?tab=readme-ov-file>`_. | ||
|
||
Installing from a private npm registry | ||
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | ||
|
||
|
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.
Is there a link to some upstream documentation about env.config.jsx that we could use here?
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.
There's the ADR. I'll link to it, sure.