Leverage vite dev proxy to improve the web app
Needs ReviewPublic
Actions

Authored by Jan Thoma (jan_thoma) on Sep 13 2022, 5:05 PM.

Details

Reviewers

Sybren A. Stüvel (sybren)

Summary

This patch solves issues when running flamenco behind a reverse proxy.

Avoids the need to set the backend port explicitly by proxying request to the backend trough vite's dev server.
Uses the window.location.href protocol instead of setting it hardcoded as ws making the websocket work with https
Make use of .env files to configure aspects of the dev server

Diff Detail

Repository: rF Flamenco

Event Timeline

Jan Thoma (jan_thoma) requested review of this revision.Sep 13 2022, 5:05 PM

Jan Thoma (jan_thoma) created this revision.

Jan Thoma (jan_thoma) added a reviewer: Sybren A. Stüvel (sybren).Sep 13 2022, 5:24 PM

Sybren A. Stüvel (sybren) added inline comments.Sep 13 2022, 7:20 PM

web/app/src/urls.js
9	This changes the `ws:` (from the original code) to `http:` or `https:` (in the new code). Does this still work when you run things without the Vite dev server?
web/app/vite.config.js
22	`is_https` would be a better constant name. It'll make it easier to understand when you (like me) criss-cross through the code, without carefully reading the assignment.

updates according to review comments

This changes the ws: (from the original code) to http: or https: (in the new code). Does this still work when you run things without the Vite dev server?

I think you missed this question. This is a functional change that I'd rather have confirmed is correct, before landing this patch.

Also you made some stylistic changes, basically from this:

export function ws() {
  return something;
}

to this:

export const ws = () => something;

These changes are fine, but shouldn't be intermixed with actual changes in behaviour. Please change only those things that are necessary for the change in behaviour. In a different patch you can do cleanups/simplifications. Separating these changes in behaviour (aka "functional" changes) from changes in coding style (aka "non-functional" changes) really helps.

web/app/vite.config.js
10	I think the `??` here (and below) should be `\|\|`. In other words, when the environment variables exist but are empty, I think it would be better if they'd be ignored. Let me know if you don't agree.

This revision now requires changes to proceed.Oct 6 2022, 3:04 PM

Made the change to urls.js as simple as possible to achieve the change in behaviour.
Changed ?? to || in vite.config.js

I also built the manager an deployed it behind a reverse proxy. It's working properly. A running instance can be visited here: https://flamenco.sg.t-k-f.ch/app/jobs

Since there's no reply to the chamges i made on 3. October 2022. I would like to ask if everything is correct with the changes i made?

web/app/src/urls.js
9	Acording to the socket.io it doesn't matter https://socket.io/docs/v3/client-initialization/
9	The manager shows me a working websocket, after building it
web/app/vite.config.js
22	good point, i agree