-
Notifications
You must be signed in to change notification settings - Fork 1
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: slot component for dynamic plugins #5
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #5 +/- ##
==========================================
+ Coverage 83.06% 83.31% +0.24%
==========================================
Files 46 53 +7
Lines 685 773 +88
Branches 132 147 +15
==========================================
+ Hits 569 644 +75
- Misses 116 129 +13 ☔ View full report in Codecov by Sentry. |
7d84303
to
d6b09c5
Compare
f5e7cd6
to
2f41a23
Compare
Thanks guys for the review @AuraAlba @01001110J @dcoa , now |
08a3097
to
70948cd
Compare
70948cd
to
ee2dffa
Compare
import messages from './messages'; | ||
|
||
const BodyForm = () => { | ||
const intl = useIntl(); |
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.
Why not directly destructure here:
const intl = useIntl(); | |
const { formatMessage } = useIntl(); |
Or is a standar for this kind of projects?
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.
It's not exactly a standard for all MFE (depending on the maintainer), but it's the common way to do 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.
Most of the MFE's use this structure principal because they use injectIntl to pass intl so I wanted to follow that way
5528a12
to
ed8a010
Compare
Hi there! thanks a lot for the help here. Those comments were really valuable I will continue with the PR that is pointing to the community which is this one I will close this one, thank you again! @AuraAlba @dcoa @01001110J |
Description
Following this POC pluggable approach to importing plugins, we've created a reusable component that will dynamically import a module wherever it is called. The main idea is to be able to change the plugins with this mechanism, which means that instead of modifying the micro-frontend beforehand, we could change the plugin in isolation.
Using pluggable component with
BulkEmailForm
componentThe alert is a plugin as well
How does it work?
Any component can be wrapped with the pluggable component. For instance:
The prop
as
indicates where is the plugin allocated. In this case, the component will be pointing to this route:node_modules/@openedx-plugins/communications-app-card
.If the plugin was installed as a dependency, it will render the component that you have pointing to that one.
If the plugin isn't installed, it will return its children. In this case, it's this:
Why are we doing this?
This means that the plugin doesn't have to be installed if I'm making a feature. It's something that we can improve in the future. That way, another developer can replace that component and make more changes. This approach allows us to wrap as many components as we can, making most of the features pluggable and adaptable to various use cases. The component can be rendered without children as well, which means it won't return anything if the plugin is not installed.
How to test it
Check it out this branch and follow these steps:
Then you can run the mfe with
npm start
If you want to create another plugin you can do it in the folder plugins/communications-app/ with the structure of the other
plugins