Skip to content
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

Open
wants to merge 1 commit into
base: nightly
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
116 changes: 116 additions & 0 deletions README.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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``.
Copy link
Contributor

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?

Copy link
Collaborator Author

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.


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:
Copy link
Contributor

Choose a reason for hiding this comment

The 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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

https://medium.com/@darbyw/repost-why-two-spaces-after-a-period-isnt-wrong-or-the-lies-typographers-tell-about-history-4017b699bd20

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
Copy link
Collaborator Author

@arbrandes arbrandes Nov 7, 2024

Choose a reason for hiding this comment

The 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 COPY from a relative parent.

Copy link
Contributor

Choose a reason for hiding this comment

The 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 env.config.jsx in mfe/build/mfe. Specifically, if we go this route, each user will need to take similar steps as I did in Indigo. This means they’ll either have to fork tutor-mfe and modify env.config.jsx or copy env.config.jsx from their custom plugin into mfe/build/mfe.

Can we think of any workaround to update or change env.config.jsx more seamlessly without requiring each user to fork plugin or override this file?

OR I am unsure that this env.config.jsx file will raise any issue for indigo plugin.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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"))
Copy link
Contributor

Choose a reason for hiding this comment

The 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 {{ patch(...) }} statement for customization.

async function getConfig () {
      let config = {};
      try {
        const { DIRECT_PLUGIN, PLUGIN_OPERATIONS } = await import('@openedx/frontend-plugin-framework');
        config = {
          pluginSlots: {}
        }
        return config;
}

function getPluginSlots(direct_plugin, plugin_operations) {
    local slots = {};
    {{ patch("mfe-plugin-slots") }}
    return slots;
}

export default getConfig;

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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 favorable

The 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 getConfig() actually pull in configuration from an mfe_config-like micro-service somewhere. Or just a text file somewhere. (If you read through #199, this is actually where I think we should go with tutor-mfe after the frontend-base work lands, instead of using the mfe_config API.)

Counter-argument: the pre-npm-build patch will always exist. People that know what they're doing can do exactly what I proposed, with or without it being documented here.

2. Per-MFE configuration

Slots 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, frontend-plugin-framework does not provide a mechanism to do that.

In this PR I suggest a way to tackle this with a single env.config.jsx by leveraging javascript itself, giving the operator maximum control, which is the reason env.config.jsx exists in the first place. If instead we go with an mfe-plugin-slots patch - or, presumably, mfe-plugin-slots-* patches - we'll have to come up with some precedence logic that in all likelihood is going to be much more limited.

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 year

We'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 env.config.jsx works, I'll be glad to modify the PR accordingly.

Copy link
Collaborator Author

@arbrandes arbrandes Nov 8, 2024

Choose a reason for hiding this comment

The 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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I just went over Indigo's env.config.jsx, and I guess that's as good an example as any of my first point above. :)

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 env.config.jsx. Maybe all they can do is npm-install a plugin package via the post-npm-install patch, import the components via a new mfe-env-config-imports patch, then declare them via mfe-env-config-plugin-slots.

Copy link
Contributor

Choose a reason for hiding this comment

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

Multiple plugins modifying env.config.jsx is definitely a tricky problem, and the fact that tutor-indigo already leverages it makes it quite the pressing issue.

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 env.config.jsx file

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 2) is shown and the first component (the one that renders 1) is not.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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!

Copy link
Contributor

Choose a reason for hiding this comment

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

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."

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.
Copy link
Contributor

Choose a reason for hiding this comment

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

Really? Do you have a specific MFE in mind?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The first build failure if you try to use the same env.config.jsx with FPF everywhere is frontend-app-authn. There might be others.

(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
Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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 frontend-plugin-framework to their list of dependencies.

Copy link
Contributor

Choose a reason for hiding this comment

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

And even more so with frontend-base - which will include/replace FPF

Copy link
Contributor

Choose a reason for hiding this comment

The 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 frontend-plugin-framework (FPF), as well as standard elements like the header and footer.

My anticipation is that, even with frontend-base in use, we will still have the flexibility to update the header and footer packages individually. This would mean that packages such as the header, footer, and brand could still be customized and maintained as custom packages while being included in frontend-base.

async function getConfig () {
let config = {};

try {
const { DIRECT_PLUGIN, PLUGIN_OPERATIONS } = await import('@openedx/frontend-plugin-framework');
Copy link
Contributor

Choose a reason for hiding this comment

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

  try {
    const { DIRECT_PLUGIN, PLUGIN_OPERATIONS } = await import('@openedx/frontend-plugin-framework');
  catch {
    console.warn("⚠️ Frontend plugin framework unavailable in " + process.env.npm_package_name);
    return;
  }

(but with working code)

I'm a bit bothered by the very large try{...} catch{} statement.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note to reviewer: I considered adding env.config.jsx templates directly to tutor-mfe and then exposing a new set of patches that would reduce some of the boilerplate, but the trade-off in flexibility for convenience seems to me to be unfavorable. To take full advantage of frontend plugins, particularly more complicated ones, users will need access to the whole env.config.jsx file anyway.

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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:
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Expand Down