search

openedx / frontend-plugin-framework

An experimental framework for micro-frontend plugins.
GNU Affero General Public License v3.0
6 stars 12 forks source link

feat: update packages to make publish smaller #49

Closed leangseu-edx closed 7 months ago

leangseu-edx commented 7 months ago
codecov[bot] commented 7 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 96.53%. Comparing base (bf2f1bf) to head (c9b8fa3).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #49 +/- ## ======================================= Coverage 96.53% 96.53% ======================================= Files 10 10 Lines 173 173 Branches 35 35 ======================================= Hits 167 167 Misses 5 5 Partials 1 1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

leangseu-edx commented 7 months ago

I just realized that I do not have permission to merge this pr. @jsnwesson can you merge after review it?

jsnwesson commented 7 months ago

Hey @leangseu-edx ! Thanks for looking into this, although I noticed that when I tried running the "host" example app, it wasn't able to render the plugins. Having compared it to the master branch, I can confirm that some dependency in this PR is causing those plugins to go missing. Would you be able to look into it?

Screenshot 2024-03-26 at 8 55 25 AM
leangseu-edx commented 7 months ago

Hey @leangseu-edx ! Thanks for looking into this, although I noticed that when I tried running the "host" example app, it wasn't able to render the plugins. Having compared it to the master branch, I can confirm that some dependency in this PR is causing those plugins to go missing. Would you be able to look into it?

I found the issue. It seems like 

    "@edx/brand": "npm:@openedx/brand-openedx@^1.2.2",
    "@edx/frontend-component-footer": "13.0.3",
    "@edx/frontend-component-header": "5.0.2",

are required at the root of the project. I don't know why it seems necessary.

Additional file changed are just replacing @edx/frontend-plugin-framework with @openedx/frontend-plugin-framework.

leangseu-edx commented 7 months ago

Sorry to add more stuff to this pr. I just feel like it would help with future development. I updated the Using the Example Apps section on README. I believe the last pr should help people develop the application better with hot reload on all subproject and the main project library.

jsnwesson commented 7 months ago

@leangseu-edx no apologies needed, this is all greatly appreciated given the time you've put into making the experience cleaner! One thing to note is that there looks to be a mix up of port numbers in the changes you made so far, but there's also other parts of the example files that likely need to be synced with the correct port number, as I wasn't able to see any of the iFrame-based plugins in the host MFE.

To make sure the terms are clear:

Oddly enough, I was able to see the Host MFE by going to localhost:8082, which according to your changes should've rendered the Child MFE instead. Not sure why that is. Additionally, I noticed in the README "Getting Started" section that devs should go to localhost:8080 to see the example app, but perhaps it was meant to point to whatever the port is for Host MFE?

leangseu-edx commented 7 months ago

To make sure the terms are clear:

  • "Host MFE" AKA the example app

    • Original PORT was 8080 according to the JS-config values, BASE_URL and PORT
    • Now looks to be 8081 according to webpack.dev.config.js

  • "Child MFE" AKA the example-plugin-app

    • Original PORT was 8081 according to the JS-config values BASE_URL and PORT and the customization in webpack.dev.config.js
    • Now looks to be 8082 according to webpack.dev.config.js

Oddly enough, I was able to see the Host MFE by going to localhost:8082, which according to your changes should've rendered the Child MFE instead. Not sure why that is. Additionally, I noticed in the README "Getting Started" section that devs should go to localhost:8080 to see the example app, but perhaps it was meant to point to whatever the port is for Host MFE?

I was sure I changed that back yesterday. At first I thought src need a port to watch the file change and hot reload. Which was the reason I tried to put it at 8080 and the rest got bump.

Anyway, now it should changed accordingly. src use babel to transpile with watch example run on 8080 example-plugin-app run on 8081

jsnwesson commented 7 months ago

Sweeet thanks for the fix! Approved and greatly appreciated. Thanks again @leangseu-edx !

openedx-semantic-release-bot commented 7 months ago

:tada: This PR is included in version 1.1.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket: