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

feat: support frontend plugins via env.config.jsx #234

Open
wants to merge 1 commit into
base: nightly
Choose a base branch
from

Conversation

arbrandes
Copy link
Collaborator

@arbrandes arbrandes commented Nov 11, 2024

This provides a mechanism to configure frontend plugins using patches to a base env.config.jsx, in such a way that multiple plugins can take advantage of the file (including for purposes beyond frontend plugins) without clobbering each other.

This is an alternate approach to #233.

@arbrandes arbrandes changed the base branch from master to nightly November 11, 2024 21:59
@arbrandes arbrandes changed the title frontend plugin support feat: support frontend plugins via env.config.jsx Nov 11, 2024
This provides a mechanism to configure frontend plugins using patches to
a base `env.config.jsx`, in such a way that multiple plugins can take
advantage of the file (including for purposes beyond frontend plugins)
without clobbering each other.
@arbrandes
Copy link
Collaborator Author

@hinakhadim, I'm particularly interested in your take on whether the provided patches are sufficient for Indigo.

Copy link
Contributor

@brian-smith-tcril brian-smith-tcril left a comment

Choose a reason for hiding this comment

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

Overall this is wonderful! Extremely well documented. Thank you so much!

I left one comment with a small suggestion, and I think one other thing isn't documented here.

We discussed having an "escape hatch" for site operators that want to have total control over the env.config.jsx files. I think it'd be nice to add documentation for the "escape hatch" strategy including:

  • Where to put env.config.jsx files
  • How to get the tutor-mfe image build to pick up those env.config.jsx files
  • Downsides to using the "escape hatch" method over the recommended patch method
    • Example: needing to manually add code to env.config.jsx files that plugins would add automatically.

Comment on lines +525 to +539
mfe-env-config-static-imports
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Use this patch for any additional static imports you need in ``env.config.jsx``. They will be available here if you used the `mfe-docker-post-npm-install patch <#mfe-docker-post-npm-install>`_ to install an NPM package globally.

It gets rendered at the very top of the file. You should use normal `ES6 import syntax <https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/import>`_.

File changed: ``tutormfe/templates/mfe/build/mfe/env.config.jsx``

mfe-env-config-head
~~~~~~~~~~~~~~~~~~~

Use this patch for arbitrary ``env.config.jsx`` javascript code. It gets rendered immediately after static imports, and is particularly useful for defining slightly more complex components for use in plugin slots.

File changed: ``tutormfe/templates/mfe/build/mfe/env.config.jsx``
Copy link
Contributor

Choose a reason for hiding this comment

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

Since mfe-env-config-static-imports-{} and mfe-env-config-dynamic-imports-{} don't exist, it might be nice to note these are only available as a "all MFEs" thing and not possible to use on a per-MFE basis.

Edit: same applies to mfe-env-config-tail

@hinakhadim
Copy link
Contributor

This is an excellent approach! It enables plugin developers to make adjustments to env.config.jsx indirectly, preserving the original file. In essence, it should function well even when multiple plugins are involved and will work well for indigo. The documentation is very well done, outstanding work!

  • If possible, could we include an example of this approach in the documentation for better understanding?
  • One consideration: as the list of plugin_slots grows, it might lead to a large number of patches. I’m not entirely sure about the implications of this. In other words, we have to manage the list of plugin_slots. If a plugin_slot name changes, then we have to update it here too. I am assuming it is based on the point 3 in this conversation.

    I’ll test this out and will be happy to share feedback based on testing with tutor-indigo.

Copy link

@sarina sarina left a comment

Choose a reason for hiding this comment

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

Reviewed just the README, looks good.


Let's take a closer look at what's happening here, starting with the patch name: the prefix is ``mfe-env-config-plugin-`` followed by the name of the slot you're trying to configure. In this case, ``footer_slot``. (There's a full list of `possible slot names <#mfe-env-config-plugin-slot-catalog>`_ below.) Next, we're hiding the default contents of the footer with a ``PLUGIN_OPERATIONS.Hide``. (Refer to the `frontend-plugin-framework README <https://github.com/openedx/frontend-plugin-framework/#>`_ for a full description of the possible plugin types and operations.) The ``default_contents`` widget ID always refers to what's in an unconfigured slot. Finally, we use ``PLUGIN_OPERATIONS.Insert`` to add our custom JSX component, composed of a simple ``<h1>`` message. We give it a widgetID of ``custom_footer``.

That's it! If you rebuild the ``mfe`` image after enabling the plugin, "This is the footer." should appear at the bottom of every MFE.
Copy link

Choose a reason for hiding this comment

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

Could this be linked to instructions to rebuild the mfe image?


.. _mfe-env-config-plugin-slot-catalog:

Here's a list of the currently available frontend plugin slots, separated by category. The documentation of each slot can be found by following the corresponding link.
Copy link

Choose a reason for hiding this comment

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

What's the strategy to keep this document up to date? Is there a template for new plugin documentation within the MFE that might include a note to update the tutor readme?

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 figure the strategy can be similar to the one for new feature and waffle flags in edx-platform. Which is to say, one of two things happens:

  • As soon as a new flag/slot is merged in the upstream repo, somebody also opens a PR to Tutor to support it. This could indeed benefit from some sort of automated reminder, but given that new MFE features are more and more commonly required to work with PR sandboxes as part of the review process, and that PR sandboxes are built with Tutor, adding these to Tutor nightly will simply be a built-in step in getting the slot merged.

  • When Tutor and its plugins are updated when a new new release is coming out, part of the work is combing through upstream repos for added and changed features. This should become less and less necessary as Tutor nightly gets "automatically" updated as described above, but if by any chance a slot or two were missed, this is where they would be added.

Copy link

Choose a reason for hiding this comment

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

Sounds good, ty

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh, and one neat thing about this PR is that it will make it possible for PR sandboxes to configure slots for testing (once the Grove plugin is updated to match).

Copy link
Contributor

Choose a reason for hiding this comment

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

This approach has one major drawback: Implementing a new slot will require making an upstream PR to tutor-mfe: this will cause a lot of back-and-forth, especially as we intend to have many more plugin slots in Open edX.

Another minor drawback is the requirement to create a Tutor plugin for users who implement their own plugin slots.

As far as I understand, there is little benefit to having an explicit list of slots here. All it gives us is some syntax sugar. We can achieve a similar result with the following MFE-specific filter:

tutormfe.hooks.Filters.PLUGIN_SLOTS.add_item((
            "profile", "footer_slot",
            """
              {
                /* Hide the global footer we defined earlier. */
                op: PLUGIN_OPERATIONS.Hide,
                widgetId: 'custom_footer',
              }"""
))
tutormfe.hooks.Filters.PLUGIN_SLOTS.add_item((
            "profile", "footer_slot",
              """{
                /* Insert a custom footer specific to the Profile MFE. */
                op: PLUGIN_OPERATIONS.Insert,
                widget: {
                  id: 'custom_profile_footer',
                  type: DIRECT_PLUGIN,
                  RenderWidget: () => (
                    <h1>This is the Profile MFE's footer.</h1>
                  )
                }
              }
))"""

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, but listing all the slots gives us one majoir advantage: it will be very evident what slots are actually available for a particular version of Tutor. Sure, we can still document them here anyway, but if we make it so a slot only works if it's explicitly supported, then it's also obligatory to document a slot when it's added here.

In any case, I'll give the PLUGIN_SLOTS filter a shot. I'm not sure yet if it's going to work with the single env.config.jsx approach I tried here.


{%- for app_name, app in iter_mfes() %}
if (process.env.npm_package_name == '@edx/frontend-app-{{ app_name }}') {
{%- if patch("mfe-env-config-dynamic-imports-{}".format(app_name)) %}
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 realized this if is redundant. I had another try-catch here before, but it's not necessary, and no longer is the if. I'll rip it out.

Copy link
Contributor

@regisb regisb left a comment

Choose a reason for hiding this comment

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

I like this approach better but I have an issue with the explicit list of plugin slots. Let me know what you think of my suggestion.

@tutor_hooks.Actions.PLUGIN_LOADED.add()
def _clear_get_mfes_cache(_name: str) -> None:
"""
Don't forget to clear cache, or we'll have some strange surprises...
"""
get_mfes.cache_clear()
get_slots.cache_clear()
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's not do that. This will break in some edge cases. Instead, both get_slots and get_mfes should use tutor.hooks.lru_cache.


.. _mfe-env-config-plugin-slot-catalog:

Here's a list of the currently available frontend plugin slots, separated by category. The documentation of each slot can be found by following the corresponding link.
Copy link
Contributor

Choose a reason for hiding this comment

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

This approach has one major drawback: Implementing a new slot will require making an upstream PR to tutor-mfe: this will cause a lot of back-and-forth, especially as we intend to have many more plugin slots in Open edX.

Another minor drawback is the requirement to create a Tutor plugin for users who implement their own plugin slots.

As far as I understand, there is little benefit to having an explicit list of slots here. All it gives us is some syntax sugar. We can achieve a similar result with the following MFE-specific filter:

tutormfe.hooks.Filters.PLUGIN_SLOTS.add_item((
            "profile", "footer_slot",
            """
              {
                /* Hide the global footer we defined earlier. */
                op: PLUGIN_OPERATIONS.Hide,
                widgetId: 'custom_footer',
              }"""
))
tutormfe.hooks.Filters.PLUGIN_SLOTS.add_item((
            "profile", "footer_slot",
              """{
                /* Insert a custom footer specific to the Profile MFE. */
                op: PLUGIN_OPERATIONS.Insert,
                widget: {
                  id: 'custom_profile_footer',
                  type: DIRECT_PLUGIN,
                  RenderWidget: () => (
                    <h1>This is the Profile MFE's footer.</h1>
                  )
                }
              }
))"""

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

Successfully merging this pull request may close these issues.

5 participants