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: slot component for dynamic plugins #5

Closed
wants to merge 16 commits into from

Conversation

johnvente
Copy link

@johnvente johnvente commented Dec 13, 2023

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.

slot-compoentn


Using pluggable component with BulkEmailForm component

image


The alert is a plugin as well
image

How does it work?

Any component can be wrapped with the pluggable component. For instance:

<PluggableComponent id="input-form" as="communications-app-card">
    <h1>Any component as default</h1>
</PluggableComponent>

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:

<h1>Any component as default</h1>

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:

 cd plugins/communications-app/CheckBoxForm && npm link
 cd plugins/communications-app/InputForm && npm link
 nvm use && npm install

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

@codecov-commenter
Copy link

codecov-commenter commented Dec 13, 2023

Codecov Report

Attention: 13 lines in your changes are missing coverage. Please review.

Comparison is base (ff90033) 83.06% compared to head (ed8a010) 83.31%.
Report is 1 commits behind head on master.

Files Patch % Lines
...bulk-email-form/BuildEmailFormExtensible/index.jsx 57.89% 8 Missing ⚠️
src/components/PluggableComponent/index.jsx 90.32% 3 Missing ⚠️
...l-form/BuildEmailFormExtensible/context/reducer.js 85.71% 2 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

@johnvente
Copy link
Author

Thanks guys for the review @AuraAlba @01001110J @dcoa , now BulkEmailForm is full pluggable, I've changed it for BuildEmailFormExtensible

import messages from './messages';

const BodyForm = () => {
const intl = useIntl();
Copy link
Member

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:

Suggested change
const intl = useIntl();
const { formatMessage } = useIntl();

Or is a standar for this kind of projects?

Copy link

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.

Copy link
Author

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

@johnvente
Copy link
Author

johnvente commented Dec 28, 2023

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

@johnvente johnvente closed this Dec 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants