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: A leaner example app that is built using Vite #54

Closed

Conversation

bradenmacdonald
Copy link

@bradenmacdonald bradenmacdonald commented Apr 2, 2024

I wanted to run the example app today, but it seems like it depends on devstack, which I don't use. So this PR introduces a demo app that uses Vite instead of frontend-build and which tries to bypass as much of frontend-platform as possible.

Current example app

Takes ~7,000 ms ⚠️ to initialize, then shows this error if you don't have devstack running:

Screenshot 2024-04-02 at 12 39 34 PM

ERR_CONNECTION_REFUSED http://localhost:18000/login_refresh

New example-vite app

Takes ~110 ms 😎 to initialize, then works because it doesn't depend on devstack at all:

Screenshot of the demo app, working

(the iframe plugin will be missing for a few seconds while the slow example-plugin-app is built using webpack, but it will hot-reload to display it once it's ready).

Note that the Vite version still includes Paragon's SCSS since the demo uses it, transpiles TypeScript and JSX, and supports hot reloading and whatever else you might want.

Thoughts

  • If you don't have the other server running, the iframe plugins simply disappear. It's probably better to show an error in that case.
  • Console warning needs fixing: 'Encountered two children with the same key, inserted_direct_plugin'
  • Console warning needs fixing: 'Invalid prop data-testid supplied to React.Fragment. React.Fragment can only have key and children props.'
  • All the example code relevant to this repo (plugins) is completely unchanged and is identical between both versions, except that the Vite version doesn't need the unused React imports that are present in the other one.
  • A frontend plugin framework shouldn't require an LMS to be running ;)
  • I think this shows a major problem with frontend-platform: this plugin framework only depends on getConfig() and friends, but to run a demo of it you have to initialize all the other services like Auth, Logging, Analytics, i18n, Redux, etc. This project (frontend-plugin-framework) should be modified so that the configuration service is an injectable dependency, and frontend-platform should be modified so you can initialize the config without unnecessary services.
  • IMHO it's time to think seriously about ditching webpack and babel and dist folders and all that legacy stuff. Vite+ESM+TS is much nicer, and it provides all the same functionality.
  • frontend-plugin-framework doesn't have type definitions yet. I'd be happy to add them or convert it to TypeScript if people are open to that.

@openedx-webhooks
Copy link

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:

  • supporting documentation
  • Open edX discussion forum threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

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.

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Apr 2, 2024
Copy link

codecov bot commented Apr 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.53%. Comparing base (9be2cd4) to head (084ed05).

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

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

@mphilbrick211 mphilbrick211 added the waiting for eng review PR is ready for review. Review and merge it, or suggest changes. label Apr 3, 2024
@@ -39,7 +39,7 @@ Using the Example Apps

2. Run ``npm run start`` in the root directory.

3. Open another terminal and run ``npm run start:example`` to start the example app. You can visit http://localhost:8080 to see the example app.
3. Open another terminal and run ``npm run start:example-vite`` to start the example app. You can visit http://localhost:8080 to see the example app.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

quick clarification on this: wouldn't changing the instruction here thus make the script npm run start:example irrelevant, and thus the Hostexample app irrelevant? If there's a benefit to keeping both, then I think this instruction could flesh out the reasoning between using one over the other.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, if we decided we were OK with maintaining the Vite version (see also https://discuss.openedx.org/t/experiment-build-an-mfe-with-vite-instead-of-frontend-build-webpack/12702 ) then we could just straight-up replace the example app. I just thought some people might prefer to have one based on frontend-build for consistency.

@jsnwesson
Copy link
Contributor

Super huge thanks for this PR, @bradenmacdonald ! @MaxFrank13 is on call this week, so he'll be the one to triage this.

On your thoughts, I think that there's some that can immediately be made into issue tickets (ie. address console logs, iFrame rendering fallback, type definitions), and I can write those up this week.

The ones that would need further discussion (ie. switch to Vite from Webpack for FPF example apps, remove LMS requirement, etc.) could either be issue tickets that live here as well or moved to wherever is more appropriate -- like extracting simple configuration needs from frontend-platform. Would you be able to get those issues/discussions created?

@MaxFrank13
Copy link
Member

MaxFrank13 commented Apr 17, 2024

I agree with Jason that it would be nice to get some of these into GitHub issues:

"this plugin framework only depends on getConfig() and friends, but to run a demo of it you have to initialize all the other services like Auth, Logging, Analytics, i18n, Redux, etc"

This is a good point. I think an issue for setting up FPF to have an injectable config belongs in this repo but the frontend-platform changes probably need their own issue in that repo.

frontend-plugin-framework doesn't have type definitions yet. I'd be happy to add them or convert it to TypeScript if people are open to that.

I think this would be very helpful. What do you imagine the lift for that work would be? We are happy to assist as well — our team even has a ticket on the backlog for this

If you don't have the other server running, the iframe plugins simply disappear. It's probably better to show an error in that case.
Console warning needs fixing: 'Encountered two children with the same key, inserted_direct_plugin'
Console warning needs fixing: 'Invalid prop data-testid supplied to React.Fragment. React.Fragment can only have key and children props.'

All good call outs. We have tickets for some of this work (again, on the backlog) but we are more than willing to be assisted here. If any of these changes seem trivial, feel free to work them in as you have time — we will do the same 🙂

FWIW, with regards to the Vite conversation — I'm in favor of making the switch. I will defer to the discourse thread though

@bradenmacdonald
Copy link
Author

bradenmacdonald commented Apr 17, 2024

I think this would be very helpful. What do you imagine the lift for that work would be? We are happy to assist as well — our team even has a ticket on the backlog for this

@MaxFrank13 I actually made a lot of progress toward that: openedx/frontend-platform#619 However it was more complex than I had expected. But I think I tried to do too much in one PR, so it got bogged down. I'd like to try again with smaller, targeted PRs. If you'd like my help on this, what I really need is someone to help test, review, and merge PRs without such a long response cycle. If I have a reviewer available, then I could get the smaller improvement PRs through at a regular pace I think, and make good progress. Also happy to be the reviewer and tester if you want to take it on, though I don't have merge rights.

@bradenmacdonald
Copy link
Author

@MaxFrank13 Whoops, got my wires crossed on the above comment... I was talking about a different repo. I think adding types to this repo wouldn't be a big lift at all!

@jsnwesson
Copy link
Contributor

hey @bradenmacdonald , @MaxFrank13 and I got a chance to look at this PR again and we think it's a great solution to make the example apps accessible to the community. We also think the original "host" MFE isn't necessary to keep for the sake of having a build that uses frontend-build, so would you be interested in removing that example app in this PR or having this PR be a stepping stone to further work to clean up the library?

@bradenmacdonald
Copy link
Author

@jsnwesson Sure, I'm happy to make that change in this PR - I think it's a nice simplification :)

I'll let you know when I've gotten to it.

@davidjoy
Copy link

Just chiming in here -

To break the dependency on a running development environment in "example" MFEs, you can do the following:

initialize({
  messages,
  handlers: {
    auth: () => {}, // This turns off auth so it can run independently of edx-platform.
  },
});

Overriding the auth handler means it no longer makes requests to edx-platform, and can just run on its own.

@bradenmacdonald - I'd hesitate to bring in a separate build tool and throw out dependencies on frontend-platform to solve this... in spirit I'd hope example apps act like normal MFEs as much as possible. I'm all for Vite experimentation, but I think we should be really measured about how and when we choose to add it to the platform. We don't even know if it's a viable alternative yet, as much as I'd like it to be.

@bradenmacdonald
Copy link
Author

@davidjoy The app in question isn't really an MFE and it's never going to be deployed in the same way the "real" MFEs are - it's just a tech demo / teaching tool. So that's why I thought it was fine to use simpler/faster tooling even though it's different than our MFE standards. If you feel strongly about it though, we could just make the fix above that you're suggesting and live with the slower builds etc.

@davidjoy
Copy link

@bradenmacdonald For what it's worth, we have pretty consistent example apps in most of our libraries that all resemble stripped down frontend-template-application apps, use frontend-build/frontend-platform, etc. They all run the same and have the same stack, (though I'm guessing many have this same unfortunate edx-platform dependency, which we should fix). I just feel like we should be intentional about diverging from that, or we're increasing cognitive load of developers going from repo to repo in yet another way, and we're trying so hard to find ways to make things more consistent. :)

I ran the dev build myself just now and you're right, it seems to take about 7 seconds to build (more on first startup, from the look of it). Rebuilds for me are taking between 350-450ms, which is fast enough not to be noticeable. IMHO I'd rather have a few extra seconds cold start time and keep consistent tooling... at least while we strategize an approach to update our tooling to be faster. 😄

And, just thinking ahead... if for OEP-65 we want to merge FPF into frontend-platform and have a tighter integration between the two, I expect we'll want to keep the example app "MFE like".

Funnily enough - the example-plugin-app already does the auth: () => {} thing! If this example app follows suit we solve the original problem with 3 lines without biting off the philosophical debate I'd rather find a way to be on your side of. 😉

@bradenmacdonald
Copy link
Author

I'm totally fine closing this PR if we can fix the example app in the way you're suggesting, so there's no need to have a running LMS.

I am still very much interested in letting people try out Vite though, and see how it can both simplify and improve our workflows.

@openedx-webhooks
Copy link

@bradenmacdonald Even though your pull request wasn’t merged, please take a moment to answer a two question survey so we can improve your experience in the future.

@itsjeyd itsjeyd removed the waiting for eng review PR is ready for review. Review and merge it, or suggest changes. label May 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
open-source-contribution PR author is not from Axim or 2U
Projects
Status: Done
Archived in project
Development

Successfully merging this pull request may close these issues.

7 participants