-
Notifications
You must be signed in to change notification settings - Fork 104
feat: support frontend plugins via env.config.jsx #240
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
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 |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| - [Improvement] Adds support for frontend plugin slot configuration via env.config.jsx. (by @arbrandes) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,7 +13,7 @@ | |
| from tutor.types import Config, get_typed | ||
|
|
||
| from .__about__ import __version__ | ||
| from .hooks import MFE_APPS, MFE_ATTRS_TYPE | ||
| from .hooks import MFE_APPS, MFE_ATTRS_TYPE, PLUGIN_SLOTS | ||
|
|
||
| # Handle version suffix in main mode, just like tutor core | ||
| if __version_suffix__: | ||
|
|
@@ -82,20 +82,20 @@ def _add_core_mfe_apps(apps: dict[str, MFE_ATTRS_TYPE]) -> dict[str, MFE_ATTRS_T | |
| return apps | ||
|
|
||
|
|
||
| @functools.lru_cache(maxsize=None) | ||
| @tutor_hooks.lru_cache | ||
| def get_mfes() -> dict[str, MFE_ATTRS_TYPE]: | ||
| """ | ||
| This function is cached for performance. | ||
| """ | ||
| return MFE_APPS.apply({}) | ||
|
|
||
|
|
||
| @tutor_hooks.Actions.PLUGIN_LOADED.add() | ||
| def _clear_get_mfes_cache(_name: str) -> None: | ||
| @tutor_hooks.lru_cache | ||
| def get_plugin_slots(mfe_name: str) -> list[tuple[str, str]]: | ||
| """ | ||
| Don't forget to clear cache, or we'll have some strange surprises... | ||
| This function is cached for performance. | ||
| """ | ||
| get_mfes.cache_clear() | ||
| return [i[-2:] for i in PLUGIN_SLOTS.iterate() if i[0] == mfe_name] | ||
|
Contributor
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.
Collaborator
Author
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. Nice catch! This solved the linting cascade. |
||
|
|
||
|
|
||
| def iter_mfes() -> t.Iterable[tuple[str, MFE_ATTRS_TYPE]]: | ||
|
|
@@ -107,11 +107,20 @@ def iter_mfes() -> t.Iterable[tuple[str, MFE_ATTRS_TYPE]]: | |
| yield from get_mfes().items() | ||
|
|
||
|
|
||
| def iter_plugin_slots(mfe_name: str) -> t.Iterable[tuple[str, str]]: | ||
| """ | ||
| Yield: | ||
|
|
||
| (slot_name, plugin_config) | ||
| """ | ||
| yield from get_plugin_slots(mfe_name) | ||
|
|
||
|
|
||
| def is_mfe_enabled(mfe_name: str) -> bool: | ||
| return mfe_name in get_mfes() | ||
|
|
||
|
|
||
| def get_mfe(mfe_name: str) -> MFE_ATTRS_TYPE: | ||
| def get_mfe(mfe_name: str) -> t.Union[MFE_ATTRS_TYPE, t.Any]: | ||
|
Collaborator
Author
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, your previous comment got me on the right track, thanks - but I didn't want to change running code just for the sake of a type hint, so this is what I did. It seems there's significant precedent for
Contributor
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 issue is caused by the lru_cache decorator, which is not correctly typed. You can bypass this issue without |
||
| return get_mfes().get(mfe_name, {}) | ||
|
|
||
|
|
||
|
|
@@ -120,6 +129,7 @@ def get_mfe(mfe_name: str) -> MFE_ATTRS_TYPE: | |
| [ | ||
| ("get_mfe", get_mfe), | ||
| ("iter_mfes", iter_mfes), | ||
| ("iter_plugin_slots", iter_plugin_slots), | ||
| ("is_mfe_enabled", is_mfe_enabled), | ||
| ] | ||
| ) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,51 @@ | ||
| {{- patch("mfe-env-config-buildtime-imports") }} | ||
|
|
||
| function addPlugins(config, slot_name, plugins) { | ||
| if (slot_name in config.pluginSlots === false) { | ||
|
Contributor
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. nitpick: Is this idiomatic? Shouldn't it be
Contributor
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 only
Collaborator
Author
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. On syntax: when testing for whether properties exist or not, Javascript is a minefield. You have to make sure you're not testing the value of the property if it exists. This is one of the reasons why the And truthiness is also a minefield in Javascript. Either way, we do have to use the negative case, here. It avoids duplicating code. |
||
| config.pluginSlots[slot_name] = { | ||
| keepDefault: true, | ||
| plugins: [] | ||
| }; | ||
| } | ||
|
|
||
| config.pluginSlots[slot_name].plugins.push(...plugins); | ||
| } | ||
|
|
||
| {{- patch("mfe-env-config-buildtime-definitions") }} | ||
|
|
||
| async function setConfig () { | ||
| let config = { | ||
| pluginSlots: {} | ||
| }; | ||
|
|
||
| try { | ||
| /* We can't assume FPF exists, as it's not declared as a dependency in all | ||
| * MFEs, so we import it dynamically. In addition, for dynamic imports to | ||
| * work with Webpack all of the code that actually uses the imported module | ||
| * needs to be inside the `try{}` block. | ||
| */ | ||
| const { DIRECT_PLUGIN, PLUGIN_OPERATIONS } = await import('@openedx/frontend-plugin-framework'); | ||
|
|
||
| {{- patch("mfe-env-config-runtime-definitions") }} | ||
|
|
||
| {%- for slot_name, plugin_config in iter_plugin_slots("all") %} | ||
| addPlugins(config, '{{ slot_name }}', [{{ plugin_config }}]); | ||
|
Contributor
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. Is there any particular reason for adding plugin_config object one by one? Could we pass array for one plugin_slot (keeping in mind multiple plugins can have same slots)? Like this:
Contributor
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. Apologies, I just realized that objects are already passing in array. I need to pass without array brackets: PLUGIN_SLOTS.add_items([
(
mfe,
"footer_slot",
"""
{
op: PLUGIN_OPERATIONS.Hide,
widgetId: 'default_contents',
},
{
op: PLUGIN_OPERATIONS.Insert,
widget: {
id: 'default_contents',
type: DIRECT_PLUGIN,
priority: 1,
RenderWidget: <Footer />,
},
},
{
op: PLUGIN_OPERATIONS.Insert,
widget: {
id: 'read_theme_cookie',
type: DIRECT_PLUGIN,
priority: 2,
RenderWidget: AddDarkTheme,
},
},
"""
),
])
Collaborator
Author
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. There are different ways you can load the configuration in. @regisb suggested we document it one-by-one to avoid issues with trailing commas. Though in this latest iteration of Otherwise, yes you can totally use a single |
||
| {%- endfor %} | ||
|
|
||
| {%- for app_name, _ in iter_mfes() %} | ||
| if (process.env.APP_ID == '{{ app_name }}') { | ||
| {{- patch("mfe-env-config-runtime-definitions-{}".format(app_name)) }} | ||
|
|
||
| {%- for slot_name, plugin_config in iter_plugin_slots(app_name) %} | ||
| addPlugins(config, '{{ slot_name }}', [{{ plugin_config }}]); | ||
| {%- endfor %} | ||
| } | ||
| {%- endfor %} | ||
|
|
||
| {{- patch("mfe-env-config-runtime-final") }} | ||
| } catch { } | ||
|
|
||
| return config; | ||
| } | ||
|
|
||
| export default setConfig; | ||

Uh oh!
There was an error while loading. Please reload this page.