-
Notifications
You must be signed in to change notification settings - Fork 103
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: replace frontend-build with vite #1000
Conversation
Thanks for the pull request, @bradenmacdonald! What's next?Please work through the following steps to get your changes ready for engineering review: 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. 🔘 Let us know that your PR is ready for review:Who will review my changes?This repository is currently maintained by Where can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:
When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
|
||
/* Linting */ | ||
"strict": true, | ||
"noImplicitAny": false, // Work around temporary lack of type data |
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.
^ Other than this noImplicitAny
line, this file is unchanged from the Vite react-ts
template.
@@ -0,0 +1,11 @@ | |||
{ |
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.
This file is unchanged from the Vite react-ts
template.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1000 +/- ##
==========================================
+ Coverage 66.78% 68.20% +1.41%
==========================================
Files 51 45 -6
Lines 849 824 -25
Branches 176 172 -4
==========================================
- Hits 567 562 -5
+ Misses 272 253 -19
+ Partials 10 9 -1 ☔ View full report in Codecov by Sentry. |
@@ -0,0 +1,18 @@ | |||
module.exports = { |
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.
This file is unchanged from the Vite react-ts
template.
Hi @openedx/2u-aperture! Would someone mind taking a look at this? Thanks! |
Note this is being discussed at https://discuss.openedx.org/t/experiment-build-an-mfe-with-vite-instead-of-frontend-build-webpack/12702 |
@@ -15,6 +15,7 @@ | |||
"lint": "eslint . --ext ts,tsx --report-unused-disable-directives --max-warnings 0", | |||
"snapshot": "fedx-scripts jest --updateSnapshot", | |||
"start": "vite --host || echo this echo is here to ignore the --config arg if tutor passes it in", | |||
"start-tutor": "PUBLIC_PATH=/profile/ MFE_CONFIG_API_URL='http://localhost:8000/api/mfe_config/v1' vite --host", |
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.
I'm very interested in getting something like this working before (and if) we migrate to Vite, haha. I'll give it a shot and let you know.
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 works! I use it in course-authoring all the time. It's much easier and faster than running it inside a tutor container.
Run this command on your host (possibly after npm install
since it's sometimes different between host and container versions, especially on ARM), and then you can use it just like the tutor containerized version:
APP_ID=course-authoring PUBLIC_PATH=/course-authoring/ MFE_CONFIG_API_URL='http://localhost:8000/api/mfe_config/v1' npm run start -- --host apps.local.overhang.io
And honestly the MFEs should be fixed so these env vars are the defaults, which would simplify this a lot.
@bradenmacdonald, this is a proof of concept PR, right? Should it be closed? |
@deborahgu Yes, that makes sense. It's not intended to be merged for now. |
@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. |
This is a proof of concept that shows how easily
@openedx/frontend-build
, a custom wrapper around webpack and babel, can be replaced with Vite.As far as I can tell, Vite supports all the functionality we need from
frontend-build
, plus has much better TypeScript integration, is much faster, and most of all requires less maintenance.The various
webpack.*.config
file in frontend-build have something like 39+187+213 = 439 lines of code (+41 loc for babel.config.js), whereas the equivalentvite.config.ts
is only ~48 lines of code - that's 10x less configuration to maintain.Notes:
tutor-mfe
andfrontend-platform
; in fact this PR is the only thing necessary to start using Vite for builds. (Though of course things could be improved even further by modifying tutor andfrontend-platform
accordingly)Metrics
Notes on these metrics:
/usr/bin/time -l
. The "peak memory footprint" reported by that command was even more favorable but I'm not sure which metric is better to use.frontend-component-header
depends on a bunch of legacy stuff that shouldn't be in the build -babel-polyfill
,babel-runtime
, andcore-js
. Fixing that will improve these metrics even further I think.Screenshots
The only difference in the result I could notice is a change to the default avatar because of how Vite currently handles SVG imports. There is probably a way to make it do the same thing that webpack was doing, but if you look at the code here you'll see that this way is actually simpler and more consistent than what was happening before.