-
Notifications
You must be signed in to change notification settings - Fork 81
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
Convert "Pages & Resources" page to a plugin system #638
Convert "Pages & Resources" page to a plugin system #638
Conversation
Thanks for the pull request, @bradenmacdonald! Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
); | ||
}; | ||
|
||
export default CalculatorSettings; |
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.
Commentary: This file is mostly unchanged (just moved) but since I changed CalculatorSettings
to use useIntl
instead of injectIntl
, the indentation all changed and git shows it in the diff as a new file.
); | ||
}; | ||
|
||
export default NotesSettings; |
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.
Commentary: This file is mostly unchanged (just moved) but since I changed NotesSettings
to use useIntl
instead of injectIntl
, the indentation all changed and git shows it in the diff as a new file. Even though useIntl
is way nicer than injectIntl
, I didn't change the rest of these plugins, in order to make the diff simpler.
"dependencies": { | ||
}, | ||
"devDependencies": {} | ||
} |
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.
In case it's not clear, generally each plugin is just three files:
Settings.jsx
which defines the UI (mostly unchanged from how it was before; just updated import paths)messages.js
which contains the strings (unchanged) (optional)package.json
which defines the plugin as an installable module (new)
<PageRoute path={`${path}/:appId/settings`}> | ||
{ | ||
({ match, history }) => { | ||
const SettingsComponent = React.lazy(async () => { |
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.
Commentary: There was a some kind of bug with this line, where the React.lazy
was actually getting re-evaluated many times, and the useEffect
of the Xpert Unit Summary plugin was getting called over and over, hammering the backend with requests to populate the redux state. This wasn't actually affecting the current MFE because it was hard-coded to load Xpert Unit Summary settings differently (see removed lines above). But when I changed it to be loaded like the other plugins, this bug occurred. The fix was easy, however: I moved it to a component and put it behind a useMemo
hook, so it would only load the state once.
// if we use a template string here: | ||
// TypeError: Cannot read property 'range' of null with using template strings here. | ||
// Ref: https://github.com/babel/babel-eslint/issues/530 | ||
return await import('@openedx-plugins/course-app-' + appId + '/Settings.jsx'); // eslint-disable-line |
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.
Commentary: As you can see, a "Course App Frontend Plugin" is any package that has a name like course-app-______
and includes a Settings.jsx
component.
In this proof of concept, all plugins must be installed into a single NPM organization scope (@openedx-plugins/
). This is because during the build, webpack has to search for all matching plugins in node_modules
. Using an org scope allows it to only have to search that org scope's subfolder (i.e. node_modules/@openedx-plugins/*/
rather than the rootnode_modules
folder and all subfolders (which can be huge, as we all know).
I think using the @openedx-plugins/
scope would be a nice clean, performant approach, but it would mean that plugins can't be hosted on the npm registry unless published by Axim. (They could still be installed by git from private repos though). So it may be preferable to remove the org scope restriction to allow more flexible hosting of plugins on npm, even though it could mean slower builds (I didn't test whether it makes much difference or not).
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.
How much of an impact to build-time would loosening the scope have? Would it be any better to make this a tad less dynamic and have the operator provide a list (more likely, a map) of fully qualified module names at build time?
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.
How much of an impact to build-time would loosening the scope have?
I'm not sure. It's possible it makes hardly any difference at all. If we are interested in adopting something like this, I will measure it.
Would it be any better to make this a tad less dynamic and have the operator provide a list (more likely, a map) of fully qualified module names at build time?
If you put the config into an actual .js
file that doesn't use dynamic imports at all, sure, you can avoid this entirely. It's definitely simpler. The downside is that that sort of configuration is slightly harder to work with when customizing deployments. I did write a blog post where I recommended that approach at first though, to keep it simple :)
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.
How much of an impact to build-time would loosening the scope have?
So I actually measured this now and for this course-authoring MFE, using node_modules/@openedx-plugins/course-app-*/Settings.jsx
as the pattern takes about 103 seconds to build the MFE and using a looser scope pattern of node_modules/openedx-fe-plugin-course-app-*/Settings.jsx
it takes on average 107 seconds. So it actually has an impact of 4 seconds on my machine, which is not trivial.
And without webpack needing to search for those possible plugins to facilitate loading them later (i.e. without the changes in this PR), the build took 97s. So I would say we probably don't want to merge a change that takes the build from 97 to 107 seconds if possible :p.
One way to avoid some of this is to use hard-coded imports instead of dynamic imports, e.g. put something like
const enabledPlugins = {
"progress": () => import("path/to/progress/plugin.jsx"),
}
this build takes about 100s, 3s longer than normal. In this case webpack is spending 3 seconds building the extra bundles for code splitting and dynamic loading, but not spending any time searching for what plugins/modules might be imported, because they're already listed.
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.
Have you looked into an environment or env.config.js variable for this? i.e. an entry called 'plugins' there can list all the plugins to install, and the code here can iterate over entries in that.
In my testing I'd used webpack's module federation for this, in which case you could pre-build such plugins and then the list of plugins to load could even be provided by runtime config from the MFE config API. I'm not sure if that approach is still in consideration.
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.
@xitij2000 Thanks for the idea. I will look into it, but haven't had time yet.
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.
@xitij2000 Thinking more about this - for this particular use case, it's not really necessary because tehre is already a server-side configuration to choose which of these "course app" plugins are enabled. The MFE now just mirrors the server side and assumes that whatever server-side plugins are installed, there is a corresponding frontend plugin installed.
For other cases, I agree that some config-based enabling would be good. Ideally, you could also pass parameters to each plugin. The MFE runtime config API is probably the best option for this, though it seems quite limited in what data types it provides to the MFE (more like env vars with simple string values than JSON objects with rich configuration).
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 don't think the MFE Config API has such a restriction, you can pass any kind of rich object. In fact, the runtime theming configuration relies on it. Even for "environment variables" there is now the 'env.config.js' route that will allow richer configuration.
I agree that the MFE config API is a bit limited. I'd like to see it becoming pluggable and support more than just site configuration as a source. In that case, a backend plugin could inject its own config into it. This is easily doable with hooks and filters.
import messages from './messages'; | ||
import appInfo from '../appInfo'; | ||
import ResetIcon from './ResetIcon'; | ||
|
||
import './SettingsModal.scss'; |
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.
Commentary: This plugin's SCSS is now loaded when the plugin is displayed, rather than always being included in the MFE's overall styles.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #638 +/- ##
==========================================
+ Coverage 89.27% 89.74% +0.46%
==========================================
Files 551 517 -34
Lines 9738 9062 -676
Branches 2099 1907 -192
==========================================
- Hits 8694 8133 -561
+ Misses 996 883 -113
+ Partials 48 46 -2 ☔ View full report in Codecov by Sentry. |
@@ -1,3 +0,0 @@ | |||
// Jest Snapshot v1, https://goo.gl/fbAQLP | |||
|
|||
exports[`SettingsComponent renders LazyLoadedComponent when provided with props 1`] = `[Function]`; |
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 snapshot test was not implemented properly - the snapshot was just [Function]
@arbrandes Can you help me get this reviewed? @xitij2000 FYI - you might want to review this too, if you haven't seen it already. |
@bradenmacdonald Please rebase this branch with the latest master and resolve the conflict. Other than this PR LGTM. |
Thanks @awais-ansari. Done. One note before we consider merging this: I have configured the xpert_unit_summary plugin not to be installed by default (so we have an example of a separate plugin). But that means that we have to either make it installed by default or change it so that it gets installed for edx.org use, before we can merge this. The simplest would be if I just make it installed by default, I suppose, which would be like that status quo. |
8a3add6
to
2ab6b2d
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.
This looks really great @bradenmacdonald! In my local testing everything worked as expected, and the architecture is quite clear!
In the PR description you mention that
Course Apps were specifically designed for this Course Authoring MFE and even though they use a plugin framework on the backend, the frontend is currently hard-coded, and every configurable course app must be built in to frontend-app-course-authoring
This PR does a wonderful job of addressing that!
I noticed one spot where the hardcoded-ness is still present. I don't think that addressing this is a blocker for merging this PR, but I'd love to hear any thoughts on how to best address this.
I'm also very interested to hear your thoughts on how to provide a intuitive experience for operators installing new course apps via tutor plugins. Would it make sense to include the frontend plugin as part of the course app tutor plugin? If so, it'd be really cool to have an example and/or template course app repository showing how all of that fits together.
I also want to thank you for the documentation in the PR description. I'd love to see some of that find a more permanent home (maybe @feanil has thoughts on where that could live). It might be worth making a "document course app frontend plugins" issue on this repo to discuss/track that.
Thanks again for this PR. I'm really excited to see this all coming together.
Here's to a smooth rebase!
Thanks @brian-smith-tcril ! Sorry I haven't had a chance to reply and rebase yet. Hoping to get that done soon and then will ping you :) |
@brian-smith-tcril OK, I got this updated with |
Hi everyone,
2. Here's the branch for it. First, I have tried this approach on a dummy footer in a codesandbox just to understand it. Here, the approach works fine with react17. For injecting plugin dynamically, I import packagesName from Any insights on this will be appreciated. |
@hinakhadim Can you please open a PR for your changes and ping us on that dedicated PR? The approach you're using is different than what's going on in this PR. |
Here is the link to the PR. The approach is different as this mfe's backend also follows the plugin based approach. I followed this article. |
How're we looking? @brian-smith-tcril, any objections to merging this? |
|
@bradenmacdonald, can we get a last rebase, pretty please? |
@bradenmacdonald 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
@arbrandes it was squashable! |
This is a proof-of-concept with a real-world use case for the Frontend Pluggability Summit.
The "Pages & Resources" page of the Course Authoring MFE displays the settings for any "Course Apps" enabled for a particular course. You can read in this ADR more about Course Apps :
However, even though Course Apps were specifically designed for this Course Authoring MFE and even though they use a plugin framework on the backend, the frontend is currently hard-coded, and every configurable course app must be built in to frontend-app-course-authoring. Including, for example, 2U's (proprietary?) "Xpert Unit Summaries" configuration UI.
What this PR does: is makes every "Course App" frontend an installable plugin as well. Eight plugins are included in the repo and installed by default (
calculator
,edxnotes
,live
,ora_settings
,proctoring
,progress
,teams
,wiki
), one is still built-in (discussions
- because it has a totally different UI than the others), and one is still included in the repo for now but not installed by default (xpert_unit_summary
).A basic plugin with a fairly complex UI:
A plugin showing custom styles (the "reset all units" button has custom SCSS that is part of the plugin):
Technical Details
Plugins will be detected when they are
npm install
ed. For example, to enable the "xpert unit summary" plugin, you have to run the commandnpm install some-other-repo/plugins/xpert_unit_summary/ --no-save
. I put that plugin in a folder calledsome-other-repo
for now to show that it doesn't matter where the plugin is hosted - it can be a totally separate repo/npm-package, and yet it will still install and register with the MFE.Because we have the course apps functionality on the backend, this particular usage of plugins does not have any need to "list all installed plugins" so you won't find any code for that. Instead, the backend determines which course apps are enabled, and then the MFE just assumes that the corresponding plugin is available and will load it on demand when the user clicks on the app card.
If you did want to be able to list all the installed frontend plugins from the MFE code, it's actually quite easy to do though:
How to test it out
If you have the MFE installed and running in development mode already, just check out this branch. You'll need to run
npm install
within the MFE's root folder then restart the MFE. If you want to test the Xpert Unit Summaries, you'll need to have the corresponding backend plugin installed, and then runnpm install plugins/course-apps/xpert_unit_summary/ --no-save
to install the frontend plugin into the MFE environment. To make things easier, I used this shim to fake the AI Aside/Xpert plugin and force enable many others.How would I use this in production?
You'd just have to add some additional step to
npm install @openedx-plugins/my-custom-plugin
for any custom plugins you have defined when you build the MFE.