Use Vite #1179

rhystmills · 2024-11-18T11:42:55Z

What does this change?

This PR moves our build and local development server to Vite from Webpack. I have done this for a few reasons:

We have 7 (when I counted) high priority client-side vulnerabilities related to webpack-dev-server and other webpack packages - I've spent many hours unsuccessfully trying to bump that package while getting hot reloading to work, or even the current setup (page refresh on local change) and had no luck. I want to sort the vulnerabilities out.
I'm not convinced hot reloading via Webpack is working properly. Instead, the page refreshes completely on local changes (perhaps based on the file 'watch' specified in the Webpack config), which is a bit annoying to work with. With Vite working properly, local development feels smoother and slightly faster.

Before	After

Full builds are ~30% faster - taking 12 seconds instead of 18.
We have lots of config for our Webpack setup - which makes major version bumps to Webpack packages difficult. We have three different conf files, and even more configuration in webpack-dev-server.js. I'm fairly sure some of that config has become redundant over various package bumps in the past, and it's now very messy, especially the local dev server where config is split across two files. In this PR, we lose ~350 lines of config and gain only 29.
We end up with far fewer direct dependencies to manage, e.g. for security vulnerabilties, which is a time consuming chore, especially with so many tools to manage. We lose 16 packages and gain only 2.
We've moved from Webpack to Vite in facia-tool and typerighter already and everyone cheered.

What did I change in order to do this?

I did a few things:

Removed all webpack config, related packages, the local websocket server on 9002 used for (faulty?) hot reloading
Added vite and some minor config, using it for build (via rollup, under the hood) and the local dev server.
Renamed any .js files containing react to end in .jsx to make Vite happy (it's also good practice)
Remove reqwest because it wasn't playing nicely with Vite - and we can safely use the native fetch api these days anyway.

The reqwest change is probably the one most likely to generate problems in this PR, it changes the mechanism used by all the requests in the application, and the arguments expected by reqwest are not the same as those expected by fetch.

I've tested every interaction the client-side application makes with outside services(e.g. Workflow, Targeting and usages) in our CODE environment) and they all seem to be working.

Will there be any follow-up work?

This PR should work by itself, but there will be some tidying as a follow-up. I'd like to:

Rename 'panda-reqwest' because we don't use reqwest any more.
Remove any redundant arguments still passed to 'panda-reqwest' in invocations across the application, e.g. crossOrigin - and do some other tidying of requests.
Make sure .jsx and .tsx files are being linted properly and sort any linting errors.

Also, things that predate this PR which I noticed while working on it:

We send a request to e.g. https://workflow.code.dev-gutools.co.uk/api/atom/undefined on load of a video page - this is obviously unintentional, though it doesn't seem to be causing major problems.
There are lots of 500 error to pages on the domain https://video.local.dev-gutools.co.uk/support/liveCapi locally
Add some more client-side tests, we test almost nothing.
Move more files to TypeScript.

How to test

Run ./script/start.sh locally. Does it start a non-hot reloading server locally? If so, build is working.
Run ./script/client-dev.sh locally. Does it start a hot reloading server locally? If so, hot reloading is working.
Deploy to CODE. Does the app work? If so, artefact building is working.

It's also worth testing requests to external services on CODE, where things are wired up better than in local development. Do the Workflow, Targeting and YouTube integrations still work as expected (i.e. do requests in their respective tabs work properly, and does information seem to propagate to those systems?). I can help out with suggestions on testing these integrations, but mostly you'll want to check that you can create and update related settings in related tabs.

Things I tested (you might also want to):

Furniture:

YouTube

Workflow

Workflow text fields
Workflow dropdowns
Passes through to workflow?

Usages:

Usages tab
Updates usages in real time?

Targeting:

Targeting tab
Targeting rule
Updates targeting?

Other:

How can we measure success?

Build and local development still work.
All requests in the application still work.
No new error messages appear in normal usage of the app.

…cations for v4 - hot module reloading not yet working

… files can't be in 'public' because vite and play are both opinionated about the purpose of the 'public' directory, leading to shenanigans. Currently there is a problem with the javascript file bundled by vite, possibly because reqwest is causing an issue (we could replace it) # Please enter the commit message for your changes. Lines starting

…moved, and so did reqwest, in order to get the app building properly. Panda requests now use 'fetch' instead. The Search page was accidentally removed in an earlier commit, this reinstates it.

…e's http and ws (non-secure) requests locally by updating our content security policy - is there a better way?

… ensuring that ./scripts/start.sh and ./scripts/client-dev.sh still work

…sx or tsx files because there are unresolved linter problems there that will create lots of noise in this already large PR. This should be addressed in a follow-up PR.

…orkflow' from showing - because getStatus was not expecting the error to look like one returned by fetch

rhystmills · 2024-11-26T10:57:25Z

.eslintrc.js

+  },
+  settings: {
+    react: {
+      version: 'detect'


Added this to sort an error when eslint was run.

rhystmills · 2024-11-26T11:26:53Z

nginx/nginx-mappings.yml

@@ -2,6 +2,3 @@ name: video
 mappings:
  - prefix: video
    port: 9001
-  - prefix: video-assets


This was only used for running the webpack dev server locally.

rhystmills · 2024-11-26T11:27:59Z

package.json

-    "lint": "eslint public/video-ui/src/**/*.js",
+    "build": "tsc && vite build",
+    "client-dev": "vite",
+    "lint": "eslint public/video-ui/src/**/** --ext .js,.ts --no-error-on-unmatched-pattern",


We also want to match on .jsx and .tsx files - I'll do this in a follow-up as it will create lots of noise in this PR - there are linting errors in files that were previously not matched by the lint command here.

rhystmills · 2024-11-26T11:39:12Z

public/video-ui/src/services/pandaReqwest.js

-export function pandaReqwest(reqwestBody, timeout = 0) {
-  const payload = Object.assign({ method: 'get' }, reqwestBody);
+export const pandaReqwest = (reqwestBody, timeout = 0) => {
+  const payload = Object.assign({ method: 'get', credentials: 'include' }, reqwestBody);


We could add these credentials only for specific requests - this might be worth doing as a follow-up PR because we'll need to update the application in a lot of places, creating noise in the PR.

public/video-ui/src/services/pandaReqwest.js - doesn't need to be an exact match

rhystmills added 8 commits November 5, 2024 12:01

Bump webpack-dev-server to v4 and update options to match new specifi…

ff5ba7d

…cations for v4 - hot module reloading not yet working

Get vite build working (other than fonts). Webpack deps need to be re…

747e720

…moved, and so did reqwest, in order to get the app building properly. Panda requests now use 'fetch' instead. The Search page was accidentally removed in an earlier commit, this reinstates it.

Gets fonts working in vite-built application

5bbecf0

Support hot reloading dev server via Vite locally. I had to allow vit…

89e0406

…e's http and ws (non-secure) requests locally by updating our content security policy - is there a better way?

Move files back to public after turning off vite's publicDir feature,…

00a5546

… ensuring that ./scripts/start.sh and ./scripts/client-dev.sh still work

Get linting working - though I'm persisting the linter not checking j…

08bb4d4

…sx or tsx files because there are unresolved linter problems there that will create lots of noise in this already large PR. This should be addressed in a follow-up PR.

Tidy some added changes

a546025

rhystmills force-pushed the rm/use-vite branch from b12fed7 to 087d2c1 Compare November 19, 2024 08:29

Restore polling in pandaReqwest, which is used on the usages tab

e4ff8cb

rhystmills force-pushed the rm/use-vite branch from 087d2c1 to e4ff8cb Compare November 19, 2024 08:38

Use correct css file name

bf44a76

rhystmills force-pushed the rm/use-vite branch 5 times, most recently from 15cd1ff to bfccb96 Compare November 21, 2024 14:18

Revert more of panda-reqwest and stop 'Could not get Atom status in W…

980741a

…orkflow' from showing - because getStatus was not expecting the error to look like one returned by fetch

rhystmills force-pushed the rm/use-vite branch from bfccb96 to a9a8b66 Compare November 25, 2024 11:26

Ensure POST requests to workflow still wor

a8d68b0

rhystmills force-pushed the rm/use-vite branch from a9a8b66 to a8d68b0 Compare November 25, 2024 11:51

rhystmills commented Nov 26, 2024

View reviewed changes

.eslintrc.js

},

settings: {

react: {

version: 'detect'

Copy link

Contributor Author

rhystmills Nov 26, 2024

Choose a reason for hiding this comment

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

Added this to sort an error when eslint was run.

rhystmills commented Nov 26, 2024

View reviewed changes

rhystmills added 2 commits November 26, 2024 11:29

Restore linebreak

c0f6549

Update docs

1d1bf14

rhystmills commented Nov 26, 2024

View reviewed changes

rhystmills added 2 commits November 26, 2024 11:57

Remove console log

c3b6235

Don't attempt to parse non-json response

925d261

rhystmills force-pushed the rm/use-vite branch from 74c6f92 to 925d261 Compare November 26, 2024 13:42

rhystmills force-pushed the rm/use-vite branch from 775fa4e to 477b2c7 Compare November 28, 2024 12:19

Assume requests are json if the content-type header includes

6b40a4d

public/video-ui/src/services/pandaReqwest.js - doesn't need to be an exact match

rhystmills force-pushed the rm/use-vite branch from 477b2c7 to 6b40a4d Compare November 28, 2024 12:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use Vite #1179

Use Vite #1179

rhystmills commented Nov 18, 2024 •

edited

Loading

rhystmills Nov 26, 2024

rhystmills Nov 26, 2024

rhystmills Nov 26, 2024

rhystmills Nov 26, 2024

Use Vite #1179

Are you sure you want to change the base?

Use Vite #1179

Conversation

rhystmills commented Nov 18, 2024 • edited Loading

What does this change?

What did I change in order to do this?

Will there be any follow-up work?

How to test

How can we measure success?

rhystmills Nov 26, 2024

Choose a reason for hiding this comment

rhystmills Nov 26, 2024

Choose a reason for hiding this comment

rhystmills Nov 26, 2024

Choose a reason for hiding this comment

rhystmills Nov 26, 2024

Choose a reason for hiding this comment

rhystmills commented Nov 18, 2024 •

edited

Loading