Discussion about bundle size

NicolasCARPi commented 8 months ago

Hello @tnrich and others,

Your ove package is used in eLabFTW, and we're very happy with it from a developer point of view, but also from a user perspective. It's a great tool!

We recently were able to ditch our repackaging repo and use the upstream npm package directly. It's great to see the app being maintained and improved over time (long term maintenance is a huge problem in OSS).

But I have a (small) problem. You see, ove package results in a lot of megabytes of JS and CSS (8.9 Mb) for me. But eLabFTW uses other libraries, too, and this adds up and adds up and you end up with a 18 Mb website... Which is something I'd like to work on.

A few points:

I understand it uses other libraries (blueprintjs, react, redux), which my app doesn't, so this needs to be added by the bundler only for ove. An app already using these won't pay the same penalty. Clearly, the dependencies you have (and that I have not) are the main issue here rather than the ove specific code.
My JS and CSS are served using brotli compression, and cached a very long time in the browser. So in the end only 3 Mb is transferred for 15 Mb of JS. Which means this is not a critical issue! But it still means a lot of code to parse for the browser...

I'd like to discuss with you possible paths for working towards improving the bundle size.

For instance, do you think it would be possible to remove the requirement for Ubuntu font? The woff files end up base64'd in the style.css which is 2.1 Mb. Removing the Ubuntu font from that file reduces it to 1.2 Mb. Which is still a lot of styles but it is a quick win with big payout. You could declare it as optional dependency.

Hope you have other leads. I'm not going to suggest switching from blueprint/react to vanilla JS, that would be silly :stuck_out_tongue: But maybe there are other low hanging fruits out there! ;)

BTW, I would be happy to sponsor this through Deltablot.

With best regards, ~Nicolas

tnrich commented 8 months ago

Hey @NicolasCARPi thanks for reaching out. Yeah the bundle size is something that has been on my radar for a long time but hasn't been prioritized given that our app is quite enterprise-y and a slightly larger payload is ok.

I think removing the ubuntu font seems like a good idea, I didn't realize that it added so much weight. I'll remove that now. I'm not sure about other low hanging fruits... here's a quick visualization of the project using

cd packages/ove;
npx vite-bundle-visualizer

Interactive page download available here - Vite Bundle Visualizer.html.zip

There are some things that are probably duplicate functionality. I think the bigger win would be to restructure the code in a way that makes it more tree shakeable somehow.

NicolasCARPi commented 8 months ago

I'll remove that now

Great!

makes it more tree shakeable somehow

Yes, that's definitely something to look at. I'm guessing vite does three shaking by default.

I found something: for lodash, if you can manage to remove all the import _ from lodash (only in 2 places!), then I think tree shaking will be better, because the rest of the code does specific imports, and lodash is super modular! Using _ imports the whole lib AFAIK.

If you make sure to "import someSpecificModule from ..." everytime and avoid importing a whole lib, it should work in the good direction.

Cheers, ~Nico

tnrich commented 8 months ago

Hey @NicolasCARPi ,

I published new versions of ui/ove that remove any import _ from "lodash" in favor of specific imports. Let me know if you find any more.

Yes, that's definitely something to look at. I'm guessing vite does three shaking by default.

Unfortunately those changes don't appear to make the lodash import any smaller according to npx vite-bundle-visualizer. Not sure why that would be.. maybe vite isn't tree shaking by default?

NicolasCARPi commented 8 months ago

Great, I can see the change in CSS file size which is huge, so that's already a positive result.

Indeed it looks like the lodash stuff isn't enough. It might be worth exploring using instead lodash-es: https://stackoverflow.com/questions/67084600/how-to-config-lodash-tree-shaking-in-vite

The README of lodash contains:

Looking for Lodash modules written in ES6 or smaller bundle sizes? Check out lodash-es.

And we are looking for smaller bundle sizes! :D

NicolasCARPi commented 8 months ago

Hello @tnrich ,

Did you have the chance to look into using lodash-es? From what I can tell, it would be a drop-in replacement with minimal code changes (but huge benefits!). s/from "lodash"/from "lodash-es" ;)

Best, ~Nico

tnrich commented 8 months ago

Hey @NicolasCARPi I tried to swap out lodash for lodash-es and ran into this error.

yarn nx run ove:start 

yarn run v1.22.19
$ /Users/tnrich/Sites/tg-oss/node_modules/.bin/nx run ove:start

> nx run ove:start

warning package.json: No license field
$ /Users/tnrich/Sites/tg-oss/node_modules/.bin/vite
✘ [ERROR] "lodash-es" resolved to an ESM file. ESM file cannot be loaded by `require`. See http://vitejs.dev/guide/troubleshooting.html#this-package-is-esm-only for more details. [plugin externalize-deps]
    ../../node_modules/vite/node_modules/esbuild/lib/main.js:1373:27:
      1373 │         let result = await callback({
           ╵                            ^
    at file:///Users/tnrich/Sites/tg-oss/node_modules/vite/dist/node/chunks/dep-df561101.js:66190:35
    at requestCallbacks.on-resolve (/Users/tnrich/Sites/tg-oss/node_modules/vite/node_modules/esbuild/lib/main.js:1373:28)
    at handleRequest (/Users/tnrich/Sites/tg-oss/node_modules/vite/node_modules/esbuild/lib/main.js:729:19)
    at handleIncomingPacket (/Users/tnrich/Sites/tg-oss/node_modules/vite/node_modules/esbuild/lib/main.js:755:7)
    at Socket.readFromStdout (/Users/tnrich/Sites/tg-oss/node_modules/vite/node_modules/esbuild/lib/main.js:679:7)
    at Socket.emit (node:events:390:28)
    at addChunk (node:internal/streams/readable:315:12)
    at readableAddChunk (node:internal/streams/readable:289:9)
    at Socket.Readable.push (node:internal/streams/readable:228:10)
    at Pipe.onStreamRead (node:internal/stream_base_commons:199:23)
  This error came from the "onResolve" callback registered here:
    ../../node_modules/vite/node_modules/esbuild/lib/main.js:1292:20:
      1292 │       let promise = setup({
           ╵                     ^
    at setup (file:///Users/tnrich/Sites/tg-oss/node_modules/vite/dist/node/chunks/dep-df561101.js:66158:27)
    at handlePlugins (/Users/tnrich/Sites/tg-oss/node_modules/vite/node_modules/esbuild/lib/main.js:1292:21)
    at buildOrContextImpl (/Users/tnrich/Sites/tg-oss/node_modules/vite/node_modules/esbuild/lib/main.js:978:5)
    at Object.buildOrContext (/Users/tnrich/Sites/tg-oss/node_modules/vite/node_modules/esbuild/lib/main.js:786:5)
    at /Users/tnrich/Sites/tg-oss/node_modules/vite/node_modules/esbuild/lib/main.js:2177:15
    at new Promise (<anonymous>)
    at Object.build (/Users/tnrich/Sites/tg-oss/node_modules/vite/node_modules/esbuild/lib/main.js:2176:25)
    at build (/Users/tnrich/Sites/tg-oss/node_modules/vite/node_modules/esbuild/lib/main.js:2025:51)
    at bundleConfigFile (file:///Users/tnrich/Sites/tg-oss/node_modules/vite/dist/node/chunks/dep-df561101.js:66109:26)
  The plugin "externalize-deps" was triggered by this import
    ../../vite.react.config.ts:9:26:
      9 │ import { camelCase } from "lodash-es";
        ╵                           ~~~~~~~~~~~
failed to load config from /Users/tnrich/Sites/tg-oss/packages/ove/vite.config.ts
error when starting dev server:
Error: Build failed with 1 error:
../../node_modules/vite/node_modules/esbuild/lib/main.js:1373:27: ERROR: [plugin: externalize-deps] "lodash-es" resolved to an ESM file. ESM file cannot be loaded by `require`. See http://vitejs.dev/guide/troubleshooting.html#this-package-is-esm-only for more details.
    at failureErrorWithLog (/Users/tnrich/Sites/tg-oss/node_modules/vite/node_modules/esbuild/lib/main.js:1649:15)
    at /Users/tnrich/Sites/tg-oss/node_modules/vite/node_modules/esbuild/lib/main.js:1058:25
    at runOnEndCallbacks (/Users/tnrich/Sites/tg-oss/node_modules/vite/node_modules/esbuild/lib/main.js:1484:45)
    at buildResponseToResult (/Users/tnrich/Sites/tg-oss/node_modules/vite/node_modules/esbuild/lib/main.js:1056:7)
    at /Users/tnrich/Sites/tg-oss/node_modules/vite/node_modules/esbuild/lib/main.js:1085:16
    at responseCallbacks.<computed> (/Users/tnrich/Sites/tg-oss/node_modules/vite/node_modules/esbuild/lib/main.js:703:9)
    at handleIncomingPacket (/Users/tnrich/Sites/tg-oss/node_modules/vite/node_modules/esbuild/lib/main.js:762:9)
    at Socket.readFromStdout (/Users/tnrich/Sites/tg-oss/node_modules/vite/node_modules/esbuild/lib/main.js:679:7)
    at Socket.emit (node:events:390:28)
    at addChunk (node:internal/streams/readable:315:12)
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
Warning: run-commands command "cd packages/ove && yarn vite" exited with non-zero status code

 ——————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————

 >  NX   Running target start for project ove failed

   Failed tasks:

   - ove:start

   Hint: run the command with --verbose for more details.

   View structured, searchable error logs at https://nx.app/runs/nWAOBE8W8K

This isn't very high priority for me right now so am not going to spend a lot of time trying to get it to work. Happy to take a PR if you get it working.

Thanks!

NicolasCARPi commented 8 months ago

Ok thanks for trying. It might indeed require more time to figure it out properly. I understand that this is not a priority for you and will come back with a PR if I manage to work it out.

Cheers, ~Nico

tnrich commented 7 months ago

Hi @NicolasCARPi unfortunately we've noticed regressions caused by the removal of the Ubuntu Mono typeface. I'll need to revert that change to how it was. I'm not sure why that typeface would increase the bundle size as much as it does.. Seems like it ought to be smaller. I can continue to investigate how to make that smaller but the editor label sizing logic was built around the size of the ubuntu mono font so changing it out for another is causing issues:

New Breaking Font (no ubuntu mono):

Old version (with ubuntu mono):

NicolasCARPi commented 7 months ago

Hello,

I see. Reverting to using Ubuntu is an understandable solution to an immediate problem. It's a pity for bundle size, but c'est la vie !

After, there will be a need to look into the actual issue at play here: improper text flow management. I'd argue that an UI should not be dependant on typeface or text size or anything like that. Of course, it's easy for me to say this, and I fully understand technical hurdles leading to decisions such as sticking to a particular font. Still, it would very much be the better fix in the long run.

Thank you for your consideration and have a nice week-end!

tnrich commented 7 months ago

Hi @NicolasCARPi agreed! Unfortunately there might be quite a bit of digging in the weeds necessary to get that ideal solution to work. Have a good weekend yourself. Thanks for being understanding!

TeselaGen / tg-oss

Discussion about bundle size #46