tiny-pilot / tinypilot

Use your Raspberry Pi as a browser-based KVM.
https://tinypilotkvm.com
MIT License
2.99k stars 249 forks source link

Setup bundling for frontend assets (JS, CSS) #538

Open mtlynch opened 3 years ago

mtlynch commented 3 years ago

We're currently storing different assets in different files, which is starting to add latency to TinyPilot's initial page load. This is a consequence of the fact that we're not using any modern JS framework that would bundle everything together for us.

We should look at options for bundling JS and maybe CSS to reduce initial page load times and allow us to better use JS modules, which will allow us to do unit testing more easily.

jotaen commented 3 years ago

I think we can (and should) approach this step-wise, I’d suggest the following plan:

Todos

mtlynch commented 3 years ago

I'm coming around to this much more, as we're now up to 137 requests to load the dashboard, with the vast majority of them being re-requests of the same CSS and JS files.

image

jotaen4tinypilot commented 3 years ago

I’ve done some investigation and wanted to share my findings and some thoughts before I start working on this. I think it’d be worth to take a step back first and see where we are currently and what we are trying to achieve.

Duplicate fetching

The duplicate fetching is strange actually, because Firefox and Chrome behave differently here. Your screenshot looks like it was taken in Chrome, and there I can confirm that I see this behaviour too. In Firefox it’s different, however, where it only transfers every unique asset once.

The cuprit are the @import and <script src=""> statements in the webcomponents. Firefox seems to collect all requestable files first (like in a Set) before issuing unique requests, whereas Chrome seems to just naively trigger one request per import encountered.

Individual fetching

Assuming that duplicate fetching wasn’t an issue, the number of unique CSS/JS assets is currently:

Community Pro
CSS 4 6
JS 14 17
Total 18 23

Unit testing

We can’t setup unit tests with NodeJS at the moment, because Node doesn’t allow mixing script style with import style, so we can’t say import poll from 'js/util/poll.js'; to test that function in a NodeJS runtime on the CLI.

Discussion

The duplicate CSS requests wouldn’t go away with bundling. We’d still have one CSS @import 'bundle.css' in every webcomponent, since the shadow root CSS is encapsulated from the global scope and doesn’t inherit global styles. That’s different from JavaScript, btw., where all global definitions are accessible. So while we’d reduce the number of fetches with bundling, the problem would still remain – albeit smaller of course, i.e. x redundant fetches from x web components.

I think there is no way around turning on caching for assets. (Which is currently turned off.) I’ve tried turning the cache on and that immediately solved the duplicate fetching on Chrome. The only case I can imagine caching to be a problem is when someone updates their device and the browser thinks “ah cool, I still have all the JS/CSS files, so I don’t bother fetching them again”, which results in undefined behaviour. There are multiple solutions to this: either it could be mitigated by a sufficiently short caching duration (e.g. 10s) compared to the relatively long update progress time. Otherwise there are conceptually more “elegant” alternatives, like etags or fingerprinting (via file name or query string).

Regarding the number of assets that we have: I think it’s currently still somewhat okay. Also, the number of assets hasn’t grown by that much throughout the last months. An alternative to bundling as a build-step, we could also consider dynamic bundling in Flask. Either via a library like Flask-Asset or naively by concatenating the internal files on the fly. The latter is not proper bundling of course, but on the other hand our dependency structure is not super complex anyway. Most of the CSS/JS is global anyway. Downside is that we’d need to move the delivery of assets from nginx to Flask. There might still be options to cache via nginx, though.

For the unit-testing we can convert the JavaScript code in our web components to <script type="module"> regardless of “bundler or not bundler”. type="module" allows us to use ES6 import/export statements, and thus we can setup unit tests (run in Node) for all standalone JS in the js/util/ folder. That’d enable us to put logic there and have it tested.

Proposal

I’d start by doing the following

I’d argue that we need the first anyway, the second would be quite nice-to-have. We could discuss the remaining approach in the meantime, e.g. also on Monday.

mtlynch commented 3 years ago

Really glad you took a step back here! I was assuming that bundling would just kind of magically solve all of our problems and it was the only solution, but you're definitely right to think harder about why we're hitting these issues in the first place.

Yeah, I turned off caching early on because it was resulting in bugs when users performed updates. I figured that the total static assets were small, so it was fine to just bluntly turn off caching globally.

I agree with you that we should fix nginx caching before we address bundling.

mtlynch commented 3 years ago

I've split this off into two separate bugs:

I'm punting the bundling part to a future milestone.