-
Notifications
You must be signed in to change notification settings - Fork 96
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 #240
feat: support frontend plugins via env.config.jsx #240
Conversation
@regisb, how about this? |
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.
I think this is a great improvement, and I only have minor comments. Looking forward to merging this.
}; | ||
|
||
{%- for app_name, app in iter_mfes() %} | ||
if (process.env.npm_package_name == '@edx/frontend-app-{{ app_name }}') { |
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.
This seems brittle. Will this work for 3rd-party hosted packages? Is there no other way to have access to the MFE app name? For instance process.env.APP_ID
?
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.
I initially considered APP_ID
but didn't go for it because it's not really used in the upstream MFE repos.
But then again, you're right, if somebody adds an MFE that doesn't follow the frontend-app convention, it wouldn't work here. Plus, APP_ID
is set in the Dockerfile for every MFE anyway. I'll see if it works.
README.rst
Outdated
/* Hide the default footer. */ | ||
op: PLUGIN_OPERATIONS.Hide, | ||
widgetId: 'default_contents', | ||
}, |
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.
This is a bit of a nitpick: I suggest having separate items for every plugin slot. I.e:
PLUGIN_SLOTS.add_items(
("all", "footer_slot",
"""
{
/* Hide the default footer. */
op: PLUGIN_OPERATIONS.Hide,
widgetId: 'default_contents'
}),
("all", "footer_slot", """..."""),
...
)
In particular, with this approach we don't have to be too careful with trailing commas.
This is a minor concern and should not be blocking this PR.
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.
As the env.config.jsx
template stands now, when targetting "all"
MFEs people will always have to add trailing commas, regardless of whether they're in separate filter items or not.
I agree this is less than ideal, though. Plus, this makes me realize the current version of the mfe-specific slot configuration has a bug (because I was still thinking in hard-coded slot names mode). I'll figure something out.
tutormfe/plugin.py
Outdated
@@ -90,6 +89,14 @@ def get_mfes() -> dict[str, MFE_ATTRS_TYPE]: | |||
return MFE_APPS.apply({}) | |||
|
|||
|
|||
@functools.lru_cache(maxsize=None) |
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.
Make sure to use tutor.hooks.lru_cache (and fix get_mfes above)
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.
Got it, will do!
tutormfe/plugin.py
Outdated
""" | ||
This function is cached for performance. | ||
""" | ||
return [i for i in PLUGIN_SLOTS.apply([]) if i[0] == mfe_name] |
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.
PLUGIN_SLOTS.iterate()
is syntactic sugar for PLUGIN_SLOTS.apply([])
.
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.
Nice! I'll make use of it.
tutormfe/plugin.py
Outdated
""" | ||
Yield: | ||
|
||
(mfe_name, slot_name, slot_config) |
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.
We are never using mfe_name
in the templates. Does that mean that we should simply not return 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.
I left it in just in case. But yeah, there's less of an argument for it if the getter function only works when defining an MFE (or "all"). I'll change it.
} | ||
{%- endfor %} | ||
|
||
{{- patch("mfe-env-config-tail") }} |
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.
nitpick: The "-tail" and "-head" suffixes are not used for other patch statements in Tutor. For instance, the patches in openedx/Dockerfile use "-pre", "-post", "-final". I'm not sure which convention is best, but we should probably follow the existing one.
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.
Yeah, I struggled with the names here, clearly. I have a different take that I'll push shortly.
14a46cf
to
6d749fd
Compare
@regisb, ready for a second round. |
6d749fd
to
3bc8bed
Compare
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.
Stellar!
{{- patch("mfe-env-config-buildtime-imports") }} | ||
|
||
function addPlugins(config, slot_name, plugins) { | ||
if (slot_name in config.pluginSlots === false) { |
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.
nitpick: Is this idiomatic? Shouldn't it be if(!(slot_name in config.pluginSlots))
? Or if(config.pluginSlots[slot_name] === null)
?
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.
I think only if (slot_name in config.pluginSlots)
is fine.
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.
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 in
operator came about.
And truthiness is also a minefield in Javascript. === false
is the only way to be absolutely sure. It's why MDN uses it in their example.
Either way, we do have to use the negative case, here. It avoids duplicating code.
|
||
{{- patch("mfe-env-config-buildtime-definitions") }} | ||
|
||
async function getConfig () { |
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.
Could we change this function name? It's conflicting with import { getConfig } from '@edx/frontend-platform';
.
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.
Sure. But I don't know what the behavior is going to be if you call frontend-platform's getConfig while actually setting config.
In any case, I'll just call it setConfig
, then.
{{- patch("mfe-env-config-runtime-definitions") }} | ||
|
||
{%- for slot_name, plugin_config in iter_plugin_slots("all") %} | ||
addPlugins(config, '{{ slot_name }}', [{{ plugin_config }}]); |
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 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:
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,
},
},
]"""
),
])
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.
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,
},
},
"""
),
])
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 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 env.config.jsx
it doesn't really matter if there are trailing commas or not, I guess it keeps it simple.
Otherwise, yes you can totally use a single add_item()
with a string that has multiple ops for one slot.
README.rst
Outdated
.. code-block:: javascript | ||
|
||
const mymodule1 = await import('mymodule1'); | ||
const { mycomponent1, mycomponent2 } = await import('mymodule2'); | ||
|
||
Note that if any module is not available at runtime, ``env.config.jsx`` execution will fail silently. |
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.
Could we update the documentation to specify that if a component is exported as a default export, it should be accessed using Component.default when dynamically imported? This clarification will help ensure proper usage and avoid potential errors during runtime.
I was testing with indigo and here's my file:
// import Footer from '@edly-io/indigo-frontend-component-footer';
...
if (process.env.APP_ID == 'learner-dashboard') {
const Footer = await import('@edly-io/indigo-frontend-component-footer');
...
{
op: PLUGIN_OPERATIONS.Insert,
widget: {
id: 'default_contents',
type: DIRECT_PLUGIN,
priority: 1,
RenderWidget: <Footer />,
},
},
...
}
It is throwing error Element type is invalid: expected a string (for built-in components) or a class/function (for composite components) but got: object.
If I use const { Footer } = await import('@edly-io/indigo-frontend-component-footer');
, it throws this error Element type is invalid: expected a string (for built-in components) or a class/function (for composite components) but got: undefined. You likely forgot to export your component from the file it's defined in, or you might have mixed up default and named imports.
.
The footer is exported as default. export default Footer;
. The errors are generated when I shift to dynamic import await (...)
. Could we add in doc that we have to use Footer(Component).default
in case of dynamic imports.
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.
Sure, I'll point to the MDN documentation. They mention that you have to use the default
key.
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.
They actually document a better way further down in the page, which is what I ended up putting up as an example.
3bc8bed
to
7daaeb5
Compare
I addressed the latest comments: feel free to merge it if it looks good, or let me know if not. :) |
Just a quick heads-up from my side to proceed with merging it. |
@DawoudSheraz can you please merge and rebase the sumac branch on top of nightly? |
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.
I will test this branch locally with indigo nightly. Hina is on leaves this week, she mentioned that this can break indigo. I would verify that before merging.
I tested this locally and indigo's dark theme toggle is not working as expected. It is not appearing consistently across pages (for instance, it does not appear on the learner dashboard). Even when it appears, the toggle does not retain its value when navigating to different pages. Hina mentioned that this could impact indigo. I am ok with merging considering Hina will be back from leaves next week and can look into updating Indigo. |
Let me know if I can help with anything. We should probably try and get this (or something like this) merged this week so it comes out with Sumac. |
@hinakhadim friendly nudge :) |
Yeah, On it. I was stuck into an issue regarding this. I was facing errors in only ora-grading MFE. All MFEs were working fine with the same Errors:
Solution:
I matched the frontend-platform and plugin-framework versions with other MFEs but that didn't helped. Don't know the exact reason of this issue but |
Hi @arbrandes , There's an issue generating with this PR on tutor-indigo that is it's not picking up by any MFE. The image building is successful and when we try to see in browser. The changes are not appearing. Steps:
Reason of Issue: Could you please help in resolving it? I tried to figure it out by calling |
@hinakhadim, lemme try and reproduce it on my end. Is there a PR to tutor-indigo that I can refer to? |
Sure, please. Here's the PR Link overhangio/tutor-indigo#109 It might be Error is in this conversation: #240 (comment) . At that time my env.config.jsx was like: (As you can see, footer in even imported in
After adding |
7daaeb5
to
7e34b50
Compare
@hinakhadim, I have some findings to discuss, but I'll do it as a review to overhangio/tutor-indigo#109. |
@hinakhadim, now that the MFEs have been fixed, let me know if things are good with overhangio/tutor-indigo#109 so we can merge this. |
Things are working good. Just waiting for a heads-up from another member regarding testing before we proceed with the merge. Status: overhangio/tutor-indigo#109 (comment) |
@arbrandes Hi. Please fix the failing tests so that this can be merged. Thanks |
@DawoudSheraz, I'm looking into it, but to be honest, it'll probably take me a while. My expertise with Python type annotations is limited. |
Plus, I'm not currently set up to reproduce the test environment. 😓 |
""" | ||
get_mfes.cache_clear() | ||
i: tuple[str, str, str] | ||
return [i[-2:] for i in PLUGIN_SLOTS.iterate() if i[0] == mfe_name] |
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.
iterate
can only be called on filters that return a list of things. I think that the annotation of PLUGIN_SLOTS is incorrect. It should be:
PLUGIN_SLOTS: Filter[list[tuple[str, str, str]], []]
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.
Nice catch! This solved the linting cascade.
tutormfe/plugin.py
Outdated
""" | ||
get_mfes.cache_clear() | ||
i: tuple[str, str, str] |
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.
This annotation should not be necessary, let's get rid of 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.
Yeah, this was just a dumb attempt to get the linting to stop complaining before I got python 3.9 set up locally. 😅
For the issue at line 124, I'm not entirely sure if this is the most appropriate solution, but it does resolve the problem
|
198e3ea
to
93c21cb
Compare
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]: |
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.
@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 Any
in the codebase, so I expect it to be fine.
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 issue is caused by the lru_cache decorator, which is not correctly typed. You can bypass this issue without Any
by writing:
def get_mfe(mfe_name: str) -> MFE_ATTRS_TYPE:
mfe: MFE_ATTRS_TYPE = get_mfes().get(mfe_name, {})
return mfe
This provides a mechanism to configure frontend plugins using a new PLUGIN_SLOTS filter and a series of patches that modify 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.
93c21cb
to
69f3467
Compare
Ok, finally got pyenv properly set up so I could reproduce the failures. Thanks for the reviews! |
This provides a mechanism to configure frontend plugins using a new PLUGIN_SLOTS filter and a series of patches that modify 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 the third take. Prior: #233, #234)