311-data icon indicating copy to clipboard operation
311-data copied to clipboard

migrate build tooling from webpack to Vite

Open aqandrew opened this issue 1 year ago • 9 comments

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

  • [x] install Vite package
  • [x] get MVP Vite dev server running
    • [x] rename .js files to .jsx if they contain JSX
      • [ ] write docs explaining the need for this convention moving forward
  • [ ] share instructions for renaming the MAPBOX_TOKEN env var to VITE_MAPBOX_TOKEN
  • [ ] port other webpack.config.js options to vite.config.js
  • [ ] uninstall webpack/Babel-related packages
  • [x] verify that vite build + vite serve builds and serves the site locally
  • [x] verify that the Vite-built site runs on GitHub Pages

Resources/Instructions

  • https://vitejs.dev/

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 page

https://github.com/hackforla/311-data/assets/9038965/f2f2b6af-7e74-423c-a1fe-8dcce11d8680

aqandrew avatar Jun 21 '24 03:06 aqandrew

Assembling some notes on initial review of this ticket. ETA on notes: within the hour

ryanfchase avatar Jun 22 '24 22:06 ryanfchase

@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

  • In what ways does Vite perform better than Webpack
    • you mentioned easier to set up tests
    • if overall build times / HMR times are better, perhaps a side-by-side comparison (either video, or timed metrics)
  • verify that the Vite-built site runs on GitHub Pages
    • this should be the 1st step moving forward to ensure we don't hit a dead end
    • consider creating a fork and getting a GitHub Pages site running as proof of concept

Splitting up work on this ticket

  • I'd say we should split up action items on this ticket
    • all action items related to just getting the demo and proof of concept can stay
    • everything related to docs / instructions for the team will go in follow up ticket(s)
  • we can have this ticket and all follow up tickets belong to the testing epic: https://github.com/hackforla/311-data/issues/1768

Action Items for Ticket Author @aqandrew

If you're ok with the plan I've proposed...

  • [ ] modify the ticket's action items as outlined above
  • [ ] create tickets for the action items that are slated to come after we've demo'd this migration to the team
    • Labels will be draft, Role: Frontend, Size: Missing, Feature: Code Health
    • Add to 311 Data Project Board (it will automatically go to New Issue Approval)
    • Milestone: X - Technical Debt

Action for PM

Similar to above, we'll do the following once we've agreed on the plan:

  • [ ] Add this ticket and all follow-up ticket(s) to the Testing and Code Coverage epic: https://github.com/hackforla/311-data/issues/1768

ryanfchase avatar Jun 22 '24 22:06 ryanfchase

Follow up on above comment

I'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 ..., { devServer: { ..., mode: hot } }, which technically should enable HMR according to Webpack's docs.

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.

if overall build times / HMR times are better, perhaps a side-by-side comparison (either video, or timed metrics)

Seeing as you got HMR working so quickly with Vite, I can consider this question answered 😄

ryanfchase avatar Jun 23 '24 21:06 ryanfchase

I did also see that our webpack.dev.js appears to have HMR enabled:

https://github.com/hackforla/311-data/blob/0d080875ca707509882cde10903c21c2143f16a5/webpack.dev.js#L13

But my local dev server does not behave like HMR.

Timed metrics

Here 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.

https://github.com/user-attachments/assets/e3cf3997-07ab-466a-aa14-48d7da88211e

Comparing 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.

aqandrew avatar Aug 15 '24 01:08 aqandrew

Hi @aqandrew,

Please leave a comment with the following items:

  • updated ETA
  • progress from the last week
  • availability for communications during the week

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.

ryanfchase avatar Aug 20 '24 07:08 ryanfchase

(My bad, I accidentally moved this to the Icebox--meant to move the testing ticket instead.)

  • updated ETA: 9/5/24
  • progress from last week: None since the timed metrics comment above. My next step is to confirm that the Vite-built site runs on GitHub Pages. Afterward I'll create a ticket(s) for writing new docs/instructions.
  • availability: 9am-5pm

aqandrew avatar Aug 22 '24 23:08 aqandrew

I've successfully deployed the Vite-built site on GitHub Pages! 😄 Note the console.log at the bottom of this screenshot:

image

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:

  1. main.yml
  2. repository secrets, in Settings > Secrets and variables > Actions > Secrets > Repository secrets image

aqandrew avatar Aug 28 '24 20:08 aqandrew

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".

ryanfchase avatar Sep 01 '24 21:09 ryanfchase

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.

aqandrew avatar Sep 04 '24 21:09 aqandrew

@ryanfchase PR is ready for review! I'll merge latest main into my branch + create the follow-up tickets tomorrow

aqandrew avatar Sep 10 '24 05:09 aqandrew

Moving this to In Review since the PR is now available

ryanfchase avatar Sep 12 '24 00:09 ryanfchase

Technically there is a dependency, #1824, but I will leave this In Review since only the merging is blocked by this topic.

ryanfchase avatar Sep 12 '24 01:09 ryanfchase

Moving to Icebox, since the review has been completed, and now the work listed in the dependency must be worked on.

ryanfchase avatar Sep 25 '24 21:09 ryanfchase

Moved to In Review since I've been re-requested on the PR

ryanfchase avatar Oct 02 '24 21:10 ryanfchase

@aqandrew question about getting this line while building the project:

The CJS build of Vite's Node API is deprecated. See https://vitejs.dev/guide/troubleshooting.html#vite-cjs-node-api-deprecated for more details.

Have you encountered this warning? Is there something in my build environment that I need to change for this?

Note from Vite docs

Link to doc.

image

ryanfchase avatar Oct 02 '24 21:10 ryanfchase

I've successfully deployed the Vite-built site on GitHub Pages! 😄 Note the console.log at the bottom of this screenshot:

(image)

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:

  1. main.yml
  2. repository secrets, in Settings > Secrets and variables > Actions > Secrets > Repository secrets

(image)

Updates:

  • I've created a ticket based on updating Repository Secrets: https://github.com/hackforla/311-data/issues/1839
  • I've also added it to the Epic: https://github.com/hackforla/311-data/issues/1768

ryanfchase avatar Oct 02 '24 21:10 ryanfchase

Have you encountered this [CJS] warning? Is there something in my build environment that I need to change for this?

@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 avatar Oct 03 '24 15:10 aqandrew

@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:

https://github.com/hackforla/VRMS/issues/1767#issuecomment-2411783144

ryanfchase avatar Oct 17 '24 18:10 ryanfchase

@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!

aqandrew avatar Oct 18 '24 20:10 aqandrew

@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.

ryanfchase avatar Oct 19 '24 04:10 ryanfchase

@aqandrew just an update:

I've used a personal fork of the 311-data repo, I did the following actions:

  • I made a branch vite which holds the latest from 1778-migrate-from-webpack-to-vite
    • I made it the main branch (just to be safe I guess)
  • I modified the main.yml workflow (named as "Deploy NRD Frontend") to watch for changes to the vite branch, rather than main
    • probably not necessary if I just pushed directly to main on my fork
  • I set up the Pages settings to deploy from the gh-pages branch, this is identical to how we do this on the 311-Data repo

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

Click to see Vite network error

image

  • My fork's gh-pages branch: https://github.com/ryanfchase/311-local-data/tree/gh-pages

ryanfchase avatar Oct 19 '24 06:10 ryanfchase

@ryanfchase I hit the same error while experimenting with my own fork.

Problem

Note that the requests that are 404ing are being requested from https://ryanfchase.github.io/assets/:

image

Your fork's deployment URL is https://ryanfchase.github.io/311-local-data/, which is based on the name of your repo. So /311-local-data/ needs to be included in the URL path for local assets. Then the correct URL for requesting the JS bundle for instance would be https://ryanfchase.github.io/311-local-data/assets/index-R9iGsBV9.js.

With your latest deployment, visiting that URL does return the requested resource:

image

Solution

You need to update vite.config.js to specify base: '/311-local-data/'. Like I did in my fork here: https://github.com/aqandrew/311-data/commit/16230b709ea8c5ac77cc6916bea26080eacd6688. See Vite's docs for more info on the base config option.

This will configure Vite's build so that bundled assets are requested from https://ryanfchase.github.io/311-local-data/assets/.

aqandrew avatar Oct 22 '24 21:10 aqandrew

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 base URL will be correct for the 311-data repo?

  • PR: https://github.com/hackforla/311-data/pull/1822

ryanfchase avatar Oct 23 '24 22:10 ryanfchase

@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 base setting will have to differ between staging/prod builds, once we release to prod again.

aqandrew avatar Oct 23 '24 23:10 aqandrew

  1. @aqandrew honestly we don't have infrastructure for staging and prod builds. We haven't figured this out since switching to Github Pages. We do have a repo (311-landingpage) that points to https:/311-data.org , but I don't think we planned on using that repo for our prod builds. I'll need to consult with Bonnie on this. I'll add it to the PM agenda for Thursday.
  2. To your point about cherry-picking the commit -- I'm seeing that now, thanks. I'll just verify your work by doing the same change on my branch.
  3. Since we're on the final stretch here for the Vite migration, can we schedule an hour or so with you in the upcoming week to hit "merge" together? This would give us a chance to do some sanity checks, and possibly roll back together if we need to. @traycn and I can coordinate this together if not, but I feel like it would be a big help!
    • Meeting times: https://www.hackforla.org/projects/311-data

ryanfchase avatar Oct 23 '24 23:10 ryanfchase

@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?

aqandrew avatar Oct 24 '24 21:10 aqandrew

Thank you, @aqandrew! Let's plan on merging in Vite on Wednesday (7p) during breakouts.

ryanfchase avatar Oct 27 '24 03:10 ryanfchase