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: iframe-based experience plugins #235

Closed
wants to merge 46 commits into from
Closed

Conversation

davidjoy
Copy link
Contributor

@davidjoy davidjoy commented Oct 21, 2021

Description:
This PR enables the ability to render plugin MFEs inside a host MFE using Plugin Slots and iFrames.

The additions and changes in this PR include:

  • Plugin setup for both Host (where plugin is rendered) and "Child" MFE (where desired component is retrieved)
  • The example app was also moved inside its own folder so that an example-plugin app could also run concurrently in order to see the plugin framework in use. (31 files)
    • Includes replacing .env files with env.config.js

Recommended Approach to Understanding

  • Run Example Apps
    • Run npm ci in both example app directories before running npm run start for each app.
    • Notable files include
      • Host MFE:
        • example/src/index
        • example/src/PluginsPage
      • "Child" MFE:
        • example-plugin-app/src/index.jsx
        • example-plugin-app/src/PluginOne.jsx (for error fallback)
        • example-plugin-app/src/PluginTwo.jsx
  • Read "edX Micro-frontend Plugins Tech Spec" (Restricted to 2U) to gain a better overview of the framework, considerations that still need to be made, and some diagrams.
  • Checkout the branch and follow the flow for:
    • Unit tests written in Plugin.test.jsx, include both Host and "Child"
    • Host MFE
      • PluginSlot -> PluginContainer -> PluginContainerIFrame
      • includes hooks, constants, and pluginConfigShape
    • "Child" MFE
      • Plugin + PluginErrorBoundary

Post merge:

  • After the build finishes for the merged commit, verify the new release has been pushed to NPM.

@davidjoy davidjoy requested a review from a team October 21, 2021 21:35
@codecov-commenter
Copy link

codecov-commenter commented Oct 21, 2021

Codecov Report

Merging #235 (f18800a) into master (6d5594d) will decrease coverage by 6.27%.
The diff coverage is 44.13%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #235      +/-   ##
==========================================
- Coverage   81.22%   74.95%   -6.28%     
==========================================
  Files          38       48      +10     
  Lines         879     1058     +179     
  Branches      159      182      +23     
==========================================
+ Hits          714      793      +79     
- Misses        156      251      +95     
- Partials        9       14       +5     
Impacted Files Coverage Δ
src/plugins/PluginContainer.jsx 0.00% <0.00%> (ø)
src/plugins/PluginSlot.jsx 0.00% <0.00%> (ø)
src/plugins/data/utils.js 2.63% <2.63%> (ø)
src/plugins/PluginComponent.jsx 28.57% <28.57%> (ø)
src/plugins/data/hooks.js 58.82% <58.82%> (ø)
src/plugins/PluginErrorBoundary.jsx 61.53% <61.53%> (ø)
src/plugins/Plugin.jsx 78.57% <78.57%> (ø)
src/plugins/PluginIframe.jsx 100.00% <100.00%> (ø)
src/plugins/data/constants.js 100.00% <100.00%> (ø)
src/plugins/data/shapes.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6d5594d...f18800a. Read the comment docs.

davidjoy and others added 15 commits May 26, 2023 11:07
By including *.scss in sideEffects, we guarantee that imports of our SCSS files aren't getting tree shaken.  This manifested as a problem in the example app, where the example/index.scss file wasn't showing up in the dev server.  The import was getting tree shook (?) because it wasn't importing any specific export.
1. Adding a port to the .env.development file
2. Improving language on the ExamplePage.
3. Removing some CSS that shouldn't have made it in.
4. Adding an LTR direction to index.html
5. Removing plugin1.html, which was a test and shouldn't have been checked in.
This commit adds a variety of plugin classes for both host applications and plugin apps themselves.
This adds an "example-plugin-app" and also gives "example" its own package.json and build files.  It separates out the test apps from the library in their parent directory, making them more independent.
This was just a scratch file where I was taking notes and shouldn't have been checked in.
Doing conflict resolution in package-lock.json is nearly impossible.  Instead of trying, I ran `npm i` to install dependencies and let it fix package-lock.json itself.  This is that generated file.
They needed to be re-created because of the node 16/18 upgrade.  They were in the old format.
…n files

Because this was an old branch we’re resurrecting, it suffered from some ugly version skew in its package-lock.json files and node_modules.  It was breaking the dev build(s).
This way you can run the example dev servers from the root project directory.
Adjusting dependency arrays and ignoring a ‘forbidden’ prop type warning.
The code removed in this commit existed to load plugins via webpack module federation.  It was never intended to be included in this version of plugins.  We don’t intend to use module federation, and are instead investing in a micro-frontend framework like Piral.
@davidjoy davidjoy force-pushed the djoy/plugin_classes branch from d6299da to b943c18 Compare May 26, 2023 15:07
Each app can have its own env.config.js because they’re independently run.  We no longer need any of the .env files here.  Since we’re no longer using .env as an input into the webpack process, we need to hardcode the port for the plugins app to 8081 so it doesn’t fall back to 8080 and conflict with the main example app.
@codecov
Copy link

codecov bot commented Sep 8, 2023

Codecov Report

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

Comparison is base (8182d21) 83.17% compared to head (1bb5c21) 81.81%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #235      +/-   ##
==========================================
- Coverage   83.17%   81.81%   -1.37%     
==========================================
  Files          40       48       +8     
  Lines        1070     1204     +134     
  Branches      195      215      +20     
==========================================
+ Hits          890      985      +95     
- Misses        168      205      +37     
- Partials       12       14       +2     
Files Coverage Δ
src/plugins/PluginContainer.jsx 100.00% <100.00%> (ø)
src/plugins/PluginContainerIframe.jsx 100.00% <100.00%> (ø)
src/plugins/data/constants.js 100.00% <100.00%> (ø)
src/plugins/data/shapes.js 100.00% <100.00%> (ø)
src/plugins/Plugin.jsx 95.00% <95.00%> (ø)
src/plugins/data/hooks.js 85.41% <85.41%> (ø)
src/plugins/PluginErrorBoundary.jsx 0.00% <0.00%> (ø)
src/plugins/PluginSlot.jsx 0.00% <0.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Mashal-m
Copy link
Contributor

Hey @davidjoy, What is the current status of this PR, is it ready to review and merge?
Could you please resolve conflicts?

@arbrandes
Copy link
Contributor

@Mashal-m, this is now in progress via openedx/frontend-plugin-framework#2. I guess we can close this one.

@arbrandes arbrandes closed this Nov 20, 2023
@jsnwesson
Copy link

@Mashal-m what Adolfo said is correct! The only thing I'd ask is that the branch stays open until I'm done with the migration. I will close the branch once I am finished.

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