Skip to content

perf: phase 1 - compression & image optimization (@abdo-spices) #7014

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.

Sign up for GitHub

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

perf: phase 1 - compression & image optimization (@abdo-spices) #7014

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d48d760
pref(hero images): optimize merch images for improved LCP
abdo-spices Oct 8, 2025
86568ce
pref: enable compression & image optimization
abdo-spices Oct 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions frontend/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,9 @@
"oxlint": "1.15.0",
"postcss": "8.4.31",
"sass": "1.70.0",
"sharp": "0.32.6",
Copy link
Member

@fehmer fehmer Oct 8, 2025

Choose a reason for hiding this comment

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

is there a reason for this two year old version of sharp? the current version is 0.34.4

Copy link
Author

@abdo-spices abdo-spices Oct 8, 2025

Choose a reason for hiding this comment

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

there is bug in modern sharp binary that specific version do not have it so we need 0.32.6

Copy link
Member

@fehmer fehmer Oct 8, 2025

Choose a reason for hiding this comment

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

which bug, please link the source

Copy link
Author

@abdo-spices abdo-spices Oct 8, 2025

Choose a reason for hiding this comment

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

which bug, please link the source

the bug is mainly with build script in sharp i can fix it in my device but well break on other people machines

lovell/sharp#1783

lovell/sharp#400

lovell/sharp#1627 (comment)

there is more cases

it also happen on windows, the it's also known bug in astro since a lot of astro projects uses image component that's powered by sharp

Copy link
Member

@Miodec Miodec Oct 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Youre linking very old issues here. Are you sure this is a problem?

"subset-font": "2.3.0",
"svgo": "4.0.0",
"tsx": "4.16.2",
"typescript": "5.5.4",
"unplugin-inject-preload": "3.0.0",
Expand All @@ -70,6 +72,8 @@
"vite-plugin-checker": "0.7.2",
"vite-plugin-filter-replace": "0.1.14",
"vite-plugin-html-inject": "1.1.2",
"vite-plugin-image-optimizer": "2.0.2",
"vite-plugin-compression2": "2.2.1",
"vite-plugin-inspect": "11.0.0",
"vite-plugin-minify": "2.1.0",
"vite-plugin-oxlint": "1.3.1",
Expand Down
2 changes: 1 addition & 1 deletion frontend/src/ts/elements/merch-banner.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ export function showIfNotClosedBefore(): void {
Notifications.addBanner(
`New merch store now open, including a limited edition metal keycap! <a target="_blank" rel="noopener" href="https://mktp.co/merch">monkeytype.store</a>`,
1,
"./images/merch3.png",
"./images/merch3.avif",
false,
() => {
closed.set(true);
Expand Down
Binary file added frontend/static/images/merch2.avif
View file
Open in desktop
Copy link
Member

@fehmer fehmer Oct 8, 2025

Choose a reason for hiding this comment

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

please delete the replaced .png files

Copy link
Author

@abdo-spices abdo-spices Oct 8, 2025

Choose a reason for hiding this comment

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

I think it's better to keep the original images

Copy link
Member

@fehmer fehmer Oct 8, 2025

Choose a reason for hiding this comment

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

why? Image optimization will pick them up and they are not used.

Copy link
Author

@abdo-spices abdo-spices Oct 8, 2025

Choose a reason for hiding this comment

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

why? Image optimization will pick them up and they are not used.

I can delete them, but it's better to keep them we might switch the image optimizer or convert them to other formats later if needed.

Copy link
Member

@Miodec Miodec Oct 15, 2025

Choose a reason for hiding this comment

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

I think ideally we would keep the originals, but also dont commit the compressed files, only output them to the dist directory when deploying.

Binary file not shown.
Binary file added frontend/static/images/merch3.avif
View file
Open in desktop
Binary file not shown.
Binary file added frontend/static/images/merch4.avif
View file
Open in desktop
Binary file not shown.
2 changes: 2 additions & 0 deletions frontend/vite.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ import DEV_CONFIG from "./vite.config.dev";
import MagicString from "magic-string";
import { Fonts } from "./src/ts/constants/fonts";

global.navigator = undefined; // sass compatibly & imagemin bug https://github.com/vitejs/vite/issues/5815#issuecomment-984041683
Copy link
Member

@Miodec Miodec Oct 15, 2025

Choose a reason for hiding this comment

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

Youre linking a closed issues from 4 years ago, are you sure this is needed?


/** @type {import("vite").UserConfig} */
const BASE_CONFIG = {
plugins: [
Expand Down
6 changes: 6 additions & 0 deletions frontend/vite.config.prod.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ import { readdirSync, readFileSync, statSync } from "node:fs";
import { ViteMinifyPlugin } from "vite-plugin-minify";
import { sentryVitePlugin } from "@sentry/vite-plugin";
import { getFontsConig } from "./vite.config";
import { compression, defineAlgorithm } from "vite-plugin-compression2";
import { ViteImageOptimizer } from "vite-plugin-image-optimizer";

function pad(numbers, maxLength, fillString) {
return numbers.map((number) =>
Expand Down Expand Up @@ -252,6 +254,10 @@ export default {
);
},
},
ViteImageOptimizer(),
compression({
Copy link
Member

@fehmer fehmer Oct 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think this is needed. Compression is already handled by the webserver/cdn

Image

This adds ~70s build time on my machine.

Copy link
Author

@abdo-spices abdo-spices Oct 8, 2025

Choose a reason for hiding this comment

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

Takes about 26s on my machine, and yeah, the Spanish list file (~1MB) is the main bottleneck at ~80% of compression time.

I think pre-compression is worth it though
latency: whether it's the server or CDN doing compression on-the-fly, you're adding 5-50ms per request. pre-compressed files skip that entirely.

server load (if it's not the CDN): If the server is handling compression instead of the CDN, that's CPU being used on every request. Pre-compression moves that work to build time, which happens way less frequently.
26s at build time vs. guaranteed fast response times without any runtime processing seems like a decent trade-off.

Copy link
Member

@fehmer fehmer Oct 8, 2025

Choose a reason for hiding this comment

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

I think pre-compression is worth it though
latency: whether it's the server or CDN doing compression on-the-fly, you're adding 5-50ms per request. pre-compressed files skip that entirely.

any source on that? I'd hope firebase would cache the compressed file and not compress it each time it is requested.

Besided, how is it working? The plugin generates an additional .br file for styles, json etc. How is it used?

Copy link
Author

@abdo-spices abdo-spices Oct 8, 2025

Choose a reason for hiding this comment

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

Besided, how is it working? The plugin generates an additional .br file for styles, json etc. How is it used?

All files can be compressed not just JS or CSS, but also images and JSON. When the browser detects a .br version, it automatically requests that instead of the original file. This significantly reduces bandwidth usage, which is important since Firebase charges based on bandwidth.

Copy link
Member

@Miodec Miodec Oct 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Firebase is already compressing files on upload. When i check a url that bypasses cloudflare everything is already compressed. So im not sure this is needed.

algorithms: [defineAlgorithm("brotliCompress", { level: 9 })],
}),
],
build: {
sourcemap: process.env.SENTRY,
Expand Down
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@
"start": "turbo run start",
"start-be": "turbo run start --filter @monkeytype/backend",
"start-fe": "turbo run start --filter @monkeytype/frontend",
"docker": "cd backend && docker compose up",
"docker": "cd backend && docker compose -f docker/compose.yml up",
"audit-fe": "cd frontend && npm run audit",
"release": "monkeytype-release",
"release-fe": "monkeytype-release --fe",
Expand Down
Loading
Loading