-
-
Notifications
You must be signed in to change notification settings - Fork 63
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
migrate build tooling from webpack to Vite #1778
Comments
Assembling some notes on initial review of this ticket. ETA on notes: within the hour |
@aqandrew @Skydodle I'd like this ticket to move forward as a demo for the team. The deliverable will be a branch that members can check out and run locally themselves Questions / Follow up on Action Items
Splitting up work on this ticket
Action Items for Ticket Author @aqandrewIf you're ok with the plan I've proposed...
Action for PMSimilar to above, we'll do the following once we've agreed on the plan:
|
Follow up on above commentI've looked into Webpack's HMR capabilities, it seems like our webpack dev server behaves more like hot reloading rather than HMR. If we look at our dev server configurations, we are using I also had a look at a different plugin, React Hot Loader, but it may take some more time before I understand how to get HMR working as intended.
Seeing as you got HMR working so quickly with Vite, I can consider this question answered 😄 |
I did also see that our webpack.dev.js appears to have HMR enabled: Line 13 in 0d08087
But my local dev server does not behave like HMR. Timed metricsHere is a video showing current reload times with webpack. The entire page must be refreshed, which triggers a request for data every time a JS module is changed. I.e., whenever a JS/JSX file is saved. I've measured the time in this video between hitting Cmd+S and seeing the updated text at 7.88 seconds, 7.81 seconds, and 7.91 seconds. Let's say the time it takes to see changes reflected in the browser is the median value, 7.88 seconds. Screen.Recording.2024-08-14.at.4.59.39.PM.trimmed.720p.movComparing this to the video in the original comment showing Vite: the slowest refresh occurs between updating "buddy" to "pal". I measured this at 0.58 seconds, 0.58 seconds, and 0.51 seconds. Let's also say that the time it takes to see changes reflected in the browser is the median value, 0.58 seconds. It's important to note that with HMR, app state is preserved because the page does not have to refresh. By this rough benchmark, we can consider Vite's HMR time to be 7.88 / 0.58 = 13.59 times faster than webpack's refresh time. |
Hi @aqandrew, Please leave a comment with the following items:
Thanks! Edit: I moved this to in-progress since it seems you are actively researching / gathering metrics. If you're not currently working on it, you can move it back to Prioritized Backlog with a comment. |
(My bad, I accidentally moved this to the Icebox--meant to move the testing ticket instead.)
|
I've successfully deployed the Vite-built site on GitHub Pages! 😄 Note the My fork is at https://github.com/aqandrew/311-data, and hosted at https://aqandrew.github.io/311-data/. In this process I found that we need to update references to Mapbox environment variable in 2 more places:
|
Thank you @aqandrew, I'm very interested to check this out. Please let me know if there is any further work to be done before we can officially move this to "In Review". |
Next is to redefine meta tags, which are currently handled with HtmlWebpackPlugin. Since we're not using a framework like Next.js, I'll go with react-helmet since it's still a reliable/popular package for managing meta tags (I think?). Will let you know when that's completed. |
@ryanfchase PR is ready for review! I'll merge latest main into my branch + create the follow-up tickets tomorrow |
Moving this to In Review since the PR is now available |
Technically there is a dependency, #1824, but I will leave this In Review since only the merging is blocked by this topic. |
Moving to Icebox, since the review has been completed, and now the work listed in the dependency must be worked on. |
Moved to In Review since I've been re-requested on the PR |
@aqandrew question about getting this line while building the project:
Have you encountered this warning? Is there something in my build environment that I need to change for this? |
Updates:
|
@ryanfchase Good catch! I've just pushed a commit to import all JS files as ESM instead of CJS: e25aa13 I used the fix that you referenced from the Vite docs. |
@aqandrew @traycn I think we're about ready to hit merge! I wanted to reference some info that the VRMS team has passed along to us. They recently encountered a build issue that totally blocked their team. @aqandrew here's a brief report, please have a look and see if any of it applies to us. I don't believe there is (I think ultimately it was a docker + npm version issue), but it will be good to check: |
@ryanfchase I'd say you're right, VRMS' issue shouldn't apply to us since we aren't using Docker. Thanks again for recommending running a proof of concept on a GitHub Pages fork! |
Running a local github pages fork as well. I've added the VITE_MAPBOX_TOKEN to the repo secrets, but none of the others. Running a gh page from my fork to see exactly what needs to change, will report back on my findings. |
@aqandrew just an update: I've used a personal fork of the 311-data repo, I did the following actions:
After triggering a github pages deployment, this is what my page looks like (see dropdown). More interestingly, this is the network tab on the dev tools. So, I don't think this is necessarily an environment key or repo secrets problem. I think it's JUST an issue with automated deployment. Let's pause on merging until this is resolved. Resources
|
@ryanfchase I hit the same error while experimenting with my own fork. ProblemNote that the requests that are 404ing are being requested from Your fork's deployment URL is https://ryanfchase.github.io/311-local-data/, which is based on the name of your repo. So With your latest deployment, visiting that URL does return the requested resource: SolutionYou need to update vite.config.js to specify This will configure Vite's build so that bundled assets are requested from |
Thank you @aqandrew! I'll take a stab at this and verify your solution works for me. I assume no code changes are required for your PR, since the current |
@ryanfchase Thanks for that reminder! I had only made that change in my fork, but I just cherry-picked the commit from my fork onto my PR's branch. We should be good to deploy to https://hackforla.github.io/311-data/ now. ^That's our staging URL though, correct? If I understand correctly, https://311-data.org/ is the prod URL. In that case that |
|
@ryanfchase For sure! This week I can meet either Wednesday or Thursday at 7pm to help with the merge. Does that work for you/@traycn? |
Thank you, @aqandrew! Let's plan on merging in Vite on Wednesday (7p) during breakouts. |
Overview
We'd like to propose an alternative build tool in order to make it easier to implement tests for our frontend testing framework
More Info (optional)
Vite is a popular open-source frontend build tool that offers HMR (hot module reloading), which means that saving changes to files during development can update the dev server almost instantly (< 50 milliseconds, source).
Hey team, I've made progress on the testing ticket #1334 (and got some unit tests working), but I've been having some difficulty configuring Babel/Jest to run further tests.
In my experience, Vite/Vitest are much simpler to set up than webpack/Jest. I got curious and wanted to see if I could replace webpack in our repo with Vite. I managed to get HMR working in about half an hour (see screen recording below!) and figured this would improve everyone's developer experience a lot.
@ryanfchase I'd like to propose deprioritizing tests in favor of upgrading our build tooling. This would be a huge win for a lot less effort, and as a bonus it will become easier to implement tests.
Action Items
vite build
+vite serve
builds and serves the site locallyResources/Instructions
hot reloading demo
⌘S
is shown in the lower left corner of my screen on save; you can see that changes to AcknowledgeModal.jsx are reflected in the browser in less than a second, without triggering a reload of the entire pageScreen.Recording.2024-06-20.at.8.06.02.PM.mov
Dependency
The text was updated successfully, but these errors were encountered: