Skip to content
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

build: migrate to Vite for fast and smaller build #159

Draft
wants to merge 36 commits into
base: main
Choose a base branch
from

Conversation

MaikoTan
Copy link
Collaborator

@MaikoTan MaikoTan commented May 31, 2024

Since we have already switched to chromium v95 for a long time, which has native support for ESM and modern JS syntaxes, and webpack is somehow discouraged these days as its slow-y and biggy code generation.
I did some research and then found that Vite (that uses rollup underhood) might be a great solution for us to switch to.

This preview PR shows how the Vite is faster and smaller comparing Webpack in the building.

$ git checkout main
Switched to branch 'main'
Your branch is up to date with 'origin/main'.

$ time npm run build

> [email protected] build
> webpack --progress --config webpack/webpack.prod.ts

webpack 5.76.0 compiled with 2 warnings in 25090 ms

________________________________________________________
Executed in   31.46 secs    fish           external
   usr time   54.81 secs    1.23 millis   54.81 secs
   sys time    4.52 secs    0.02 millis    4.52 secs

$ du -sh ./dist
25M     ./dist

$ git checkout vite
Switched to branch 'vite'

$ time npm run build

> [email protected] build
> vite build --config vite/vite.config.ts

✓ built in 6.79s

________________________________________________________
Executed in    7.25 secs    fish           external
   usr time    8.67 secs  387.00 micros    8.66 secs
   sys time    0.60 secs   94.00 micros    0.60 secs


$ du -sh ./dist
8.2M    ./dist

Since Vite and Webpack are totally different tools, it is not able to copy the behaviours side by side. So what I am aiming is to create a result that functions as same as the user could expect.

Feel free to comment here if you have any thoughts

@cactbotbot
Copy link
Collaborator

cactbotbot commented May 31, 2024

@MaikoTan Thanks for your contribution! 🌵🚀

@github-actions github-actions bot added raidboss /ui/raidboss module config ui/config, ui/[module]/config util /util oopsy /ui/oopsyraidsy module test /test, /ui/test jobs /ui/jobs module radar /ui/radar module dps /ui/dps module pullcounter /ui/pullcounter module labels May 31, 2024
@MaikoTan MaikoTan changed the title build: migrate to use vite for building fast and modern output build: migrate to Vite for fast and smaller build May 31, 2024
@github-actions github-actions bot added the raidemulator /ui/raidemulator module label May 31, 2024
Copy link
Collaborator

@valarnin valarnin left a comment

Choose a reason for hiding this comment

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

Overall I like the direction of this PR, and I'll do a more thorough review once you're done with changes and I can actually test it.

Things to test/verify:

  1. Changes to raidboss/oopsy data files (both timelines [.txt] and triggers [.ts]) properly trigger a reload when running with npm run start
  2. Build artifact matches the same layout as the original build artifact (e.g. paths to HTML files are identical)
  3. Overlays work properly in both OverlayPlugin and normal browsers (edit:) both from local webserver and from disk
  4. raidemulator web worker for file import still functions properly

.vscode/settings.json Outdated Show resolved Hide resolved
vite/vite.config.prod.ts Outdated Show resolved Hide resolved
@MaikoTan
Copy link
Collaborator Author

MaikoTan commented May 31, 2024

$ npm run tsc-no-emit

> [email protected] tsc-no-emit
> tsc --noEmit

Error: node_modules/vite/dist/node/index.d.ts(5,41): error TS2307: Cannot find module 'rollup/parseAst' or its corresponding type declarations.
Error: Process completed with exit code 2.

To solve this problem, we'd have to set the moduleResolution option to nodenext / node16 or bundler (only valid at TypeScript v5.0 or higher).
nodenext and node16 require an additional .js suffix in the import statement and cannot be omitted by plugins like we did before, so the only solution might be upgrading the typescript to v5.0. I think it can be done at another PR though.

@MaikoTan
Copy link
Collaborator Author

MaikoTan commented Jun 3, 2024

  1. Changes to raidboss/oopsy data files (both timelines [.txt] and triggers [.ts]) properly trigger a reload when running with npm run start

I found that adding a simple import.meta.hot.accept() call would immediately apply HMR onto the manifest file and its dependencies (i.e, timelines and triggers). Other issues are still investigating but Vite is so great!

@MaikoTan
Copy link
Collaborator Author

MaikoTan commented Dec 5, 2024

Due to TS type resolution mechanism, I found that it is hard to maintain the original *_manifest.txt mechanism in vite configurations.
Since we are now not using the manifest.txt file at all, I changed the import statements to something like import raidbossData from 'raidboss.manifest to make it easier for providing TS type definitions and also be handled by Vite plugins to resolve as a virtual module for further processing.

By the way, I found that the raidboss_data.bundle.js (which includes all timelines and triggers) is now a massive 7.2MiB size (or 21.4MiB if includes source-map, although the webpack produces 9.2MiB and 27.6MiB...). Theoretically we could split them by patches and use dynamic imports for a faster FCP, but should we?

Build for GitHub Pages output
computing gzip size (33)...[vite-plugin-static-copy] Copied 203 items.
dist/ui/raidboss/raidboss_silent.html              0.07 kB │ gzip:     0.09 kB
dist/ui/raidboss/raidboss_timeline_only.html       0.31 kB │ gzip:     0.24 kB
dist/ui/raidboss/raidboss_alerts_only.html         0.31 kB │ gzip:     0.24 kB
dist/util/logtools/splitter.html                   0.76 kB │ gzip:     0.31 kB
dist/ui/jobs/jobs.html                             1.37 kB │ gzip:     0.33 kB
dist/ui/config/config.html                         1.43 kB │ gzip:     0.35 kB
dist/ui/oopsyraidsy/oopsy_summary.html             1.56 kB │ gzip:     0.42 kB
dist/ui/oopsyraidsy/oopsy_viewer.html              1.59 kB │ gzip:     0.43 kB
dist/ui/pullcounter/pullcounter.html               1.60 kB │ gzip:     0.38 kB
dist/ui/eureka/eureka.html                         1.63 kB │ gzip:     0.39 kB
dist/ui/dps/rdmty/dps.html                         1.70 kB │ gzip:     0.42 kB
dist/util/coverage/coverage.html                   1.78 kB │ gzip:     0.42 kB
dist/ui/radar/radar.html                           1.79 kB │ gzip:     0.48 kB
dist/ui/raidboss/raidboss.html                     2.36 kB │ gzip:     0.49 kB
dist/ui/dps/xephero/xephero-cactbot.html           2.41 kB │ gzip:     0.59 kB
dist/ui/test/test.html                             2.51 kB │ gzip:     0.71 kB
dist/ui/oopsyraidsy/oopsyraidsy.html               9.43 kB │ gzip:     1.32 kB
dist/ui/raidboss/raidemulator.html                15.13 kB │ gzip:     2.47 kB
dist/modulepreload-polyfill.bundle.js              0.93 kB │ gzip:     0.58 kB │ map:      0.11 kB
dist/ui/eureka/eureka.bundle.js                   16.16 kB │ gzip:     6.22 kB │ map:      8.41 kB
dist/ui/test/test.bundle.js                       27.58 kB │ gzip:     7.20 kB │ map:     15.92 kB
dist/ui/dps/xephero/xephero.bundle.js             33.01 kB │ gzip:    11.61 kB │ map:     20.75 kB
dist/util/coverage/coverage.bundle.js             35.25 kB │ gzip:    13.64 kB │ map:     21.55 kB
dist/ui/pullcounter/pullcounter.bundle.js         36.30 kB │ gzip:    11.74 kB │ map:     21.63 kB
dist/ui/radar/radar.bundle.js                     52.17 kB │ gzip:    18.47 kB │ map:     31.46 kB
dist/ui/dps/rdmty/dps.bundle.js                   61.00 kB │ gzip:    16.03 kB │ map:     37.31 kB
dist/ui/config/config.bundle.js                   90.85 kB │ gzip:    28.65 kB │ map:     55.75 kB
dist/ui/oopsyraidsy/oopsyraidsy.bundle.js        364.00 kB │ gzip:   105.19 kB │ map:    221.78 kB
dist/ui/raidboss/raidboss.bundle.js              720.35 kB │ gzip:   205.62 kB │ map:    420.18 kB
dist/ui/raidboss/raidemulator.bundle.js        1,202.09 kB │ gzip:   321.66 kB │ map:    722.46 kB
dist/ui/common/oopsyraidsy_data.bundle.js      1,712.65 kB │ gzip:   390.42 kB │ map:  1,038.28 kB
dist/ui/jobs/jobs.bundle.js                    1,788.70 kB │ gzip:   426.43 kB │ map:  1,174.84 kB
dist/ui/common/vendor.bundle.js                2,054.52 kB │ gzip:   491.37 kB │ map:  1,164.36 kB
dist/ui/common/raidboss_data.bundle.js        20,872.86 kB │ gzip: 4,901.80 kB │ map: 10,124.57 kB

(!) Some chunks are larger than 500 kB after minification. Consider:
- Using dynamic import() to code-split the application
- Use build.rollupOptions.output.manualChunks to improve chunking: https://rollupjs.org/configuration-options/#output-manualchunks
- Adjust chunk size limit for this warning via build.chunkSizeWarningLimit.
✓ built in 17.44s

Is there any suggestion here? Ping @valarnin

@MaikoTan
Copy link
Collaborator Author

MaikoTan commented Dec 5, 2024

image

Wow~

Now the performance blame would come to MSBuild. Lol.

@valarnin
Copy link
Collaborator

valarnin commented Dec 6, 2024

By the way, I found that the raidboss_data.bundle.js (which includes all timelines and triggers) is now a massive 7.2MiB size. Theoretically we could split them by patches and use dynamic imports for a faster FCP, but should we?

I don't think FCP is very important for overlays since the overlay is almost always served up locally and therefore there's not an issue with data transfer speed. Initial JS engine parsing takes a little bit longer for a single large file compared to smaller ~1mb bundles, but the performance hit is negligible.

Using dynamic imports would cause potential delays when changing content and would require a lot of code changes and making stuff run as async that doesn't currently, that seems way out of scope for this PR. It might also cause really hard to diagnose issues if a user is pointing at the github.io hosted bundle and github has a hiccup and doesn't serve up the bundle.

massive 7.2MiB size

The current release zip's raidboss_data.bundle.js is 10,201,534 bytes. gzip compression via webpack dev server reduces this to ~2MB. Apparently github.io's gzip compression is really bad, because it only reduces the file to ~5.85mb.

I'm not sure if the 7.2MiB number you're getting is from the uncompressed file (good) or the compressed file (really, really bad).

@MaikoTan
Copy link
Collaborator Author

MaikoTan commented Dec 6, 2024

I'm not sure if the 7.2MiB number you're getting is from the uncompressed file (good) or the compressed file (really, really bad).

Long story short:

without source map:

  • uncompressed: 7.2MiB
  • gzipped: 4.79MiB

with source map (which is used on github page)

  • uncompressed: 21.4MiB
  • gzipped: 4.95MiB

@MaikoTan
Copy link
Collaborator Author

MaikoTan commented Dec 6, 2024

I don't think FCP is very important for overlays since the overlay is almost always served up locally and therefore there's not an issue with data transfer speed. Initial JS engine parsing takes a little bit longer for a single large file compared to smaller ~1mb bundles, but the performance hit is negligible.

Using dynamic imports would cause potential delays when changing content and would require a lot of code changes and making stuff run as async that doesn't currently, that seems way out of scope for this PR. It might also cause really hard to diagnose issues if a user is pointing at the github.io hosted bundle and github has a hiccup and doesn't serve up the bundle.

In the past days it is not a bid deal as most players are running their overlays locally. But Their is iinact that is using our overlays from ghpages build causes some people sometimes hardly to load the overlay (which I heard mostly from China as they have politic network issues). I know this change is hard to made so it is just a "by-the-way", but you're right, it definitely causes delays if users met network issues no matter it is dynamic-imported or not.

@valarnin
Copy link
Collaborator

valarnin commented Dec 6, 2024

I know this is still WIP but figured I'd give a quick preliminary scan of the built artifact for potential problems.


The generated artifact doesn't add the extra top-level cactbot folder. This will cause issues with the autoupdater and will cause problems with user files.

Archive:  cactbot-0.33.1.zip
  Length      Date    Time    Name
---------  ---------- -----   ----
    83456  2024-11-19 03:30   cactbot/cactbot/CactbotEventSource.dll
...
Archive:  cactbot-f11b96b3.zip
  Length      Date    Time    Name
---------  ---------- -----   ----
    84480  2024-12-06 07:04   cactbot/CactbotEventSource.dll

Running a diff of the list of files in the archive shows a couple points of concern:

165a168
> cactbot/cactbot/ui/common/vendor.bundle.js
183c190
< cactbot/cactbot/ui/oopsyraidsy/oopsy_live.bundle.js
---
> cactbot/cactbot/ui/oopsyraidsy/oopsy_common.css
184a192
> cactbot/cactbot/ui/oopsyraidsy/oopsyraidsy.bundle.js
186d193
< cactbot/cactbot/ui/oopsyraidsy/oopsy_summary.bundle.js
189d195
< cactbot/cactbot/ui/oopsyraidsy/oopsy_viewer.bundle.js
207d212
< cactbot/cactbot/ui/raidboss/raidemulator.worker.js
215d219
< cactbot/cactbot/ui/test/test.css

vendor.bundle.ts seems to contain an assorted collection of stuff that's not actually vendor-related (e.g. player-override.ts contents, timerbar.ts contents).

oopsyraidsy stuff seems to be completely rearranged. I think it's all there but it's hard to be sure without actually running/testing. It looks like all the typescript was rolled into a single oopsyraidsy.bundle.js. That file is also significantly smaller, I think webpack was really, really not bundling oopsy properly and was including the oopsy data bundle in each of the oopsy bundles. I think this change is fine, but should be tested thoroughly to make sure there are no unexpected side effects.

test.css is missing, I'm not sure if this got rolled into another bundle somewhere or is just not being built properly. There might be some webpack automatic magic happening here to add the css reference to the html file at build time or something.

The raidemulator worker bundle seems to be MIA. Maybe it got bundled in with something else, but IIRC I had a lot of issues with getting it to run properly as a separate web worker thread in browsers if it wasn't isolated to its own source file.


None of the overlays work when loaded from a file:/// URI, because they all use absolute references to scripts/css/images. And also because esm modules can't be loaded from a file:/// URI (<script type="module", see: MDN)

  <title>Cactbot Raidboss</title>
  <script type="module" crossorigin src="/modulepreload-polyfill.bundle.js"></script>

This is going to try and load file:///C:/modulepreload-polyfill.bundle.js when loaded from a file URI.

image

This also holds true via an actual OverlayPlugin overlay:

[2024-12-06 12:55:15] Info: asdf: BrowserConsole: Access to script at 'file:///C:/modulepreload-polyfill.bundle.js' from origin 'null' has been blocked by CORS policy: Cross origin requests are only supported for protocol schemes: http, data, chrome, chrome-extension, chrome-untrusted, https. (Source: file:///C:/Users/valarnin/Downloads/cactbot-f11b96b3/cactbot/ui/raidboss/raidboss.html, Line: 0)

Removing the crossorigin property from the <style> tags allows the stylesheets to be loaded, but no amount of wrangling will allow a <script type="module" script to load via file:/// URI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
config ui/config, ui/[module]/config dps /ui/dps module jobs /ui/jobs module oopsy /ui/oopsyraidsy module pullcounter /ui/pullcounter module radar /ui/radar module raidboss /ui/raidboss module raidemulator /ui/raidemulator module test /test, /ui/test util /util
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants