perf: build-time precompression + startup metadata cache for static serving

[NathanDrake2406]

This PR implements a static file serving method that is better than Next.js 16.2.

Bugs fixed

1. Event loop blocking — existsSync + statSync ran on every static file request, blocking SSR responses behind synchronous filesystem calls. Now zero FS calls per request (metadata cached at startup).
2. No 304 Not Modified — No ETag, no If-None-Match support. Every repeat visit re-downloaded every asset in full. Now returns 304 (200 bytes) when the browser already has the asset.
3. HEAD returned full body — HEAD requests streamed the entire file body. Now sends headers only, per HTTP spec.

Optimizations

1. Build-time precompression (opt-in) — .br (brotli q5), .gz (gzip l8), .zst (zstd l8) generated at build time. Zero compression CPU per request. Enable via precompress: true or --precompress CLI flag.
2. Startup metadata cache — Pre-computed response headers per variant. Zero object allocation in the hot path.
3. In-memory buffer serving — Small precompressed files (< 64KB) served via res.end(buffer) instead of createReadStream().pipe(), eliminating file descriptor overhead.
4. Zstandard support — First Node.js framework to serve .zst assets. 3-5x faster client-side decompression than brotli (Chrome 123+, Firefox 126+).
5. Async filesystem fallback — Non-cached files use fsp.stat() instead of blocking statSync.
6. Filename-hash ETags — Hashed assets use the content hash from the filename for ETags (stable across deploys, Docker builds, CI). Non-hashed files fall back to mtime.

Real-world impact

• SSR doesn't stall anymore. Before: 10 concurrent static file requests block the event loop with sync stat calls while SSR waits. Now: static serving is non-blocking with zero syscalls.
• Repeat visits transfer almost nothing. Before: full asset re-download on every revisit. Now: 304 response (200 bytes) instead of the full bundle.
• First visits transfer 70-80% less data. Before: raw uncompressed JS/CSS. Now: build-time brotli q5 or zstd. A 200KB bundle becomes ~50KB. On mobile, that's seconds saved.
• Server CPU drops to near zero for static assets. Before: brotli compression on every request. Now: pre-compressed at build time, served from memory.
• Build cost is negligible. fumadocs-docs-template: +30ms (1.2%) with precompression off, compression adds ~32ms for 100 assets when opted in.

Benchmark (100 assets, median of 5 runs)

  Precompression Benchmark
  Apple M4 · 10 cores
  5000 requests, 10 concurrent connections

  ┌─────────────────────────────────────────────────────────────────┐
  │  Build-time: compression speed (old vs new levels)             │
  └─────────────────────────────────────────────────────────────────┘

  Config                                Median               Range            Brotli              Gzip              Zstd
  ────────────────────────────────────────────────────────────────────────────────────────────────────────────
  Old (br:5, gz:9, zstd:22)             8027ms         7087-8905ms  262.1 KB (90.8%)  297.2 KB (89.6%)  225.3 KB (92.1%)
  New (br:5, gz:8, zstd:8)                32ms             27-33ms  262.1 KB (90.8%)  297.1 KB (89.6%)  241.5 KB (91.5%)

  Result: 253.6x faster (median of 5 runs)

  ┌─────────────────────────────────────────────────────────────────┐
  │  Serving throughput: vinext vs sirv vs send vs on-the-fly       │
  └─────────────────────────────────────────────────────────────────┘

  Server                       req/s       p50       p99   Transferred   vs send
  ──────────────────────────────────────────────────────────────────────────
  vinext (precompressed)       21032     0.4ms     0.8ms       11.8 MB      1.8x
  sirv (SvelteKit)             14076     0.6ms     2.0ms       12.8 MB      1.2x
  send (Next.js)               11611     0.8ms     2.0ms      139.0 MB      1.0x
  vinext (main)                 7904     1.1ms     4.1ms       13.2 MB      0.7x

  ┌─────────────────────────────────────────────────────────────────┐
  │  304 Not Modified (median of 5 runs)                            │
  └─────────────────────────────────────────────────────────────────┘

  Server                      Median req/s                 Range
  ──────────────────────────────────────────────────────────────
  vinext (precompressed)             25799           22989-26226
  sirv (SvelteKit)                   25535           25300-25784

fumadocs-docs-template build time

main (baseline):   2.530s ± 0.019s
branch:            2.560s ± 0.031s
delta:             +30ms (1.2%), ranges overlap

Feature comparison

	vinext (this PR)	SvelteKit / sirv	Next.js / send	vinext (before)
Per-request FS calls	0	0	1 (stat)	2 (existsSync + statSync)
Event loop blocking	No	No	No	Yes (sync stat)
Precompression	zstd + brotli + gzip	brotli + gzip	none	none
Compression quality	build-time (br:5, gz:8, zstd:8)	max (build-time)	N/A	fast (per-request)
Small file buffering	Yes (< 64KB in memory)	No (always streams)	No (always streams)	No
304 Not Modified	Yes	Yes	Yes	No
HEAD optimization	Yes	Yes	Yes	No
Zstandard	Yes	No	No	No
Content-Length	Yes (all responses)	Yes	Yes	No (compressed)
ETag	Yes (filename hash)	Yes (mtime)	Yes	No

Architecture

Build time:    precompressAssets()         → .br + .gz + .zst files on disk (opt-in)
Server boot:   StaticFileCache.create()    → scan dirs, cache metadata + buffers
Per request:   Map.get() → ETag check → res.writeHead(precomputed) → res.end(buf)

[pkg-pr-new]

Open in StackBlitz

npm i https://pkg.pr.new/vinext@641

commit: 43d0d4d

[james-elicx]

Nice idea! Thanks for the PR!

I wonder if we should gate this behind an option instead of being on-by-default, considering that Next.js doesn't do this... I wonder if compression level is something we should let users control as well.

It would be good to get a progress bar like we have in pre-rendering too if there isn't one already.

[james-elicx]

/bigbonk review

[ask-bonk]

Review: Build-time precompression + startup metadata cache

Strong PR overall. The architecture (build-time precompression → startup cache → zero-allocation hot path) is sound, the test coverage is thorough (52 new tests), and the performance wins are real. The code quality is high — clear separation of concerns between precompress.ts, static-file-cache.ts, and the refactored tryServeStatic.

I have a few issues to flag, ranging from a correctness bug to design questions.

Key issues

1. Missing Vary: Accept-Encoding on original variant when compressed variants exist — When a file has .br/.gz/.zst variants but the client doesn't accept any compression, the original variant is served without a Vary header. This means caches between the server and client could cache the uncompressed response and serve it to clients that do accept compression. The compressed variant headers correctly include Vary: Accept-Encoding, but the original variant needs it too when sibling compressed variants exist.

2. Mtime-based ETag is fragile for containerized/CI deployments — W/"${size}-${mtimeMs}" uses the file modification time. In Docker builds, CI artifact uploads, or cp -r without --preserve, mtime changes between deploys even when file content is identical. This causes unnecessary cache invalidation for returning users. Hashed assets (which are the primary target) have content hashes in their filenames so this is less impactful in practice, but for non-hashed public directory files it matters. A content-hash-based ETag (or at least using the filename hash when available) would be more robust.

3. Maintainer feedback on opt-in — @james-elicx raised making this opt-in/configurable. The precompression step adds meaningful build time (brotli q11 is slow), and since Next.js doesn't do this, it could surprise users. Worth addressing before merge.

See inline comments for specifics.

[ask-bonk]

Bug: Missing Vary: Accept-Encoding on the original variant when compressed siblings exist.

When a file has .br/.gz/.zst precompressed variants, the original variant's headers omit Vary: Accept-Encoding. If a CDN or shared cache sits between the server and client, it could cache this uncompressed response and serve it to clients that accept compression (or vice versa).

The fix: when compressed variants exist for a file, add Vary: Accept-Encoding to the original variant's headers too.

Suggested change

	};
	const original: FileVariant = {
	path: fileInfo.fullPath,
	headers: { ...baseHeaders, "Content-Length": String(fileInfo.size) },
	};

After the compressed variant detection block (after line ~180), add:

if (entry.br || entry.gz || entry.zst) {
  original.headers["Vary"] = "Accept-Encoding";
}

[ask-bonk]

Mtime-based ETag is fragile in container/CI deployments.

W/"${size}-${mtimeMs}" changes whenever the file is copied without preserving timestamps (common in Docker multi-stage builds, CI artifact caching, rsync without -t, etc.). For hashed assets this is mitigated by the immutable cache-control + filename hash, but for non-hashed public directory files (favicon.ico, robots.txt, etc.) this means unnecessary 200s instead of 304s across deploys.

Consider:

1. For files under assets/ (already content-hashed): extract the hash from the filename for the ETag, or just use the size (since the filename changes on content change anyway).
2. For other files: a fast hash of the file content (e.g., the first 8 bytes of a SHA-256, hex-encoded) would be more stable.

Not a blocker since sirv uses the same strategy, but worth noting as a follow-up improvement.

[ask-bonk]

Unnecessary sequential stat() — could batch for faster startup.

readdir({ withFileTypes: true }) doesn't provide size/mtimeMs, so the stat() call is indeed necessary. However, since each stat() is awaited sequentially inside the for loop, a directory with 500 files would make 500 sequential syscalls during startup.

Consider collecting the file paths from readdir and batching the stat() calls with Promise.all (perhaps in chunks of 50-100 to avoid fd exhaustion). This would meaningfully reduce startup latency for large dist/client/ directories.

Non-blocking — the current approach is correct, just slower than it needs to be.

[ask-bonk]

Nit: Comment is incorrect. Accept-Encoding is NOT guaranteed to be lowercase in Node.js IncomingMessage. Node.js lowercases header names (keys), but header values preserve their original casing. So req.headers["accept-encoding"] gives you the value as-sent by the client.

In practice this is fine because .includes("zstd") etc. will match the standard lowercase tokens that browsers send. But the comment is misleading.

Suggested change

	// Accept-Encoding is already lowercase in Node.js IncomingMessage.
	// Accept-Encoding header values from browsers use lowercase tokens.

[ask-bonk]

No error handling on createReadStream().pipe().

If the file is deleted between startup (when it was cached) and request time, createReadStream will emit an error that crashes the connection without a proper HTTP response. The slow path at line 490 uses pipeline() which handles errors; this fast path should too.

Suggested change

	fs.createReadStream(variant.path).pipe(res);
	pipeline(fs.createReadStream(variant.path), res, () => {});

(Or alternatively, wrap in a try/catch and send a 500 if the stream errors.)

[ask-bonk]

Node.js version concern: zlib.zstdCompress is experimental before Node 22.15.

The repo requires engines.node >= 22, but zlib.zstdCompress was experimental until Node 22.15.0 (May 2025). Users on Node 22.0–22.14 will get ExperimentalWarning runtime warnings during builds.

Options:

1. Bump engines.node to >=22.15 (aligns with stable zstd)
2. Wrap in try/catch and gracefully skip .zst generation if the API isn't available or is experimental
3. Accept the warnings (since the API does work)

Worth documenting the decision either way.

[ask-bonk]

Unbounded concurrency for large asset directories.

All compression work is pushed to compressionWork and then awaited with Promise.all. For a large app with hundreds of assets, this means hundreds of concurrent brotli q11 compressions (CPU-intensive) running simultaneously, which could spike memory usage and starve the event loop.

Consider limiting concurrency (e.g., process 8-16 files at a time). A simple approach:

const CONCURRENCY = Math.min(os.cpus().length, 16);

with a semaphore or chunked processing.

[ask-bonk]

No opt-out mechanism. As @james-elicx noted, this runs unconditionally on every build. For users who deploy behind a CDN that handles compression (e.g., Cloudflare), the build-time compression adds overhead with no benefit. Brotli q11 is particularly slow — a large app with many JS bundles could add 10+ seconds to build time.

Suggestions:

1. Add a CLI flag: vinext build --no-precompress
2. Add a plugin option: vinext({ precompress: false }) in vite.config.ts
3. Default to enabled but respect a config option to disable

The Cloudflare Workers deployment target is particularly relevant here — Cloudflare's CDN handles compression at the edge, so precompressing for the Node.js prod server doesn't help Workers users at all.

[ask-bonk]

304 response includes headers that should arguably be excluded.

The notModifiedHeaders include Content-Type, which RFC 9110 says SHOULD NOT be sent on a 304. In practice browsers handle this fine, but it's worth being aware of. The test at line 362 of serve-static.test.ts correctly verifies that Content-Length is absent, which is good.

Non-blocking, but for strict RFC compliance, notModifiedHeaders should only include ETag, Cache-Control, Vary, and Date.

[ask-bonk]

"Legacy accessors" smell — consider removing if this is new code.

These fields (resolvedPath, size, contentType, cacheControl, brPath, brSize, gzPath, gzSize, zstPath, zstSize) duplicate information already available through the original/br/gz/zst variant objects. The comment says they're for "backwards compatibility with tests," but since the tests in this PR are all new, there is no backwards compatibility requirement.

Removing them would simplify the interface and eliminate the dual source of truth. The tests in static-file-cache.test.ts could be updated to access entry.original.headers["Content-Length"] instead of entry.size, etc.

[ask-bonk]

Review posted on PR #641. Here's a summary of what I flagged:

Correctness issues:

1. Missing Vary: Accept-Encoding on the original variant when compressed siblings exist — can cause incorrect shared cache behavior
2. createReadStream().pipe() without error handling in the fast path (line 447) — if a file is deleted between startup and request time, the connection crashes without a proper HTTP response

Design/robustness concerns:
3. Mtime-based ETag fragility in containerized/CI deployments where file mtimes change across deploys
4. Unbounded compression concurrency in precompressAssets — hundreds of concurrent brotli q11 operations could spike memory
5. No opt-out mechanism for precompression — runs unconditionally, but Cloudflare Workers users don't benefit (CDN handles compression at the edge) and brotli q11 is slow
6. Node.js 22.0–22.14 experimental warnings for zlib.zstdCompress

Code quality:
7. Misleading comment about Accept-Encoding being lowercase in IncomingMessage (header names are lowercased, values are not)
8. Legacy accessor fields on StaticFileEntry that duplicate information from the variant objects — unnecessary since both the interface and tests are new
9. 304 response includes Content-Type which RFC 9110 says SHOULD NOT be sent (non-blocking, browsers handle it fine)
10. Sequential stat() calls in walkFilesWithStats could be batched for faster server startup

github run

[NathanDrake2406]

Nice idea! Thanks for the PR!

I wonder if we should gate this behind an option instead of being on-by-default, considering that Next.js doesn't do this... I wonder if compression level is something we should let users control as well.

It would be good to get a progress bar like we have in pre-rendering too if there isn't one already.

I added a CLI option for it. Tbh I was just in the mood to optimise and half way through I realised it doesn't matter for Workers anyway lmao. It's okay tho, someday when a dev who isn't on Workers will benefit form it :copium:

[james-elicx]

/bigbonk review

[ask-bonk]

Review: Build-time precompression + startup metadata cache

Strong contribution. The architecture is well-designed — build-time compression, startup cache, zero-allocation hot path — and the previous review's main correctness issue (missing Vary on original variant, batched stat() calls) has been addressed. Test coverage is thorough (52 new tests). I have a few remaining issues, one of which is a correctness bug.

Issues

1. Doc comment / constant mismatch on compression levels (misleading, multiple locations)
The file header comment in precompress.ts says "brotli q5" and "zstd l22", and the CLI comment says "brotli q5 + gzip l9 + zstd l22", but the PR title and description claim "brotli q11" and "zstd l19". The actual code uses brotli q5 and zstd l22. The PR description benchmarks cite "brotli q11" results. Either the code or the description is wrong — this needs to be reconciled so users know what quality level they're getting.

2. createZstdCompress called unconditionally in createCompressor — crashes on Node < 22.15
The build-time code in precompress.ts correctly feature-detects zlib.zstdCompress before using it. But the runtime createCompressor function in prod-server.ts calls zlib.createZstdCompress() unconditionally in the "zstd" case. If negotiateEncoding returns "zstd" on a Node version where zstd isn't available (or is experimental), this will throw at runtime. The negotiation should check zstd availability before advertising support.

3. pipeline error callback silently swallows errors on the fast path
At prod-server.ts:447, pipeline(fs.createReadStream(variant.path), res, () => {}) silently ignores all errors. If the file is deleted between cache creation and request time, createReadStream emits an error that never gets handled — the client gets a broken connection with no HTTP error response. The slow path and the compressed-response path both use the same pattern, but those at least have compressor in the pipeline which triggers a res close. The fast path (buffer miss, large file) should handle errors — at minimum log, ideally send 500 if headers haven't been sent.

4. Slow path doesn't serve HEAD correctly
The fast path (with cache) correctly returns headers-only for HEAD requests. The slow path (no cache, lines 452-500) has no HEAD check — it always streams the full body. This means if StaticFileCache.create() ever returns an empty cache (race, error), HEAD requests regress to streaming the full file.

5. resolvedClient traversal check is skipped on the fast path
The fast path trusts the cache completely — if a pathname is in the cache, it's served. But the cache is keyed by "/{relativePath}" where relativePath comes from path.relative(base, fullPath). If walkFilesWithStats somehow yields a path outside clientDir (e.g., symlink), the traversal guard is bypassed. Low risk since readdir + isFile() won't follow cross-device symlinks by default, but worth noting — the slow path has an explicit startsWith(resolvedClient + path.sep) check.

[ask-bonk]

Doc/code mismatch on compression levels. The header says "brotli q5" and "zstd l22", but the PR title and description benchmarks claim "brotli q11" and "zstd l19". The actual code uses BROTLI_PARAM_QUALITY: 5 (line 117) and ZSTD_c_compressionLevel: 22 (line 124).

The PR description's benchmark results cite "brotli q11, max quality" — if those numbers were measured with q11, they don't reflect what this code actually produces. Either:

1. Update the code to use q11/l19 to match the PR description, or
2. Update the PR description to accurately reflect q5/l22

q5 is a reasonable default (much faster than q11), but the discrepancy between docs and code will confuse anyone reading the PR description alongside the source.

[ask-bonk]

Bug: negotiateEncoding advertises zstd without checking runtime availability.

This returns "zstd" if the client sends it in Accept-Encoding, but createCompressor then calls zlib.createZstdCompress() unconditionally. On Node versions where zstd isn't available (or is experimental and disabled), this will throw.

The build-time code in precompress.ts correctly feature-detects zstdCompress before using it. The same pattern should apply here:

Suggested change

	if (lower.includes("zstd")) return "zstd";
	if (lower.includes("zstd") && typeof zlib.createZstdCompress === "function") return "zstd";

Alternatively, hoist a const HAS_ZSTD = typeof zlib.createZstdCompress === "function" constant and use it in both negotiateEncoding and createCompressor.

[ask-bonk]

Error swallowed silently — broken connections on file-not-found.

If the cached file is deleted between startup and request time (e.g., a deploy that replaces dist/ while the server is running), createReadStream emits an error. The empty callback () => {} silently discards it, and the client gets a broken/hung connection with no HTTP response.

Consider:

Suggested change

	pipeline(fs.createReadStream(variant.path), res, () => {});
	pipeline(fs.createReadStream(variant.path), res, (err) => {
	if (err && !res.headersSent) {
	res.writeHead(500);
	res.end("Internal Server Error");
	}
	});

Note: res.headersSent will be true here since writeHead was called above. So the realistic fix is to just not crash silently — at minimum a console.error so operators can diagnose the issue. The same pattern exists on line 490 and 499 in the slow path.

[ask-bonk]

Slow path missing HEAD request handling.

The fast path (cache hit) correctly returns headers-only for HEAD requests at line 437-440. The slow path here always streams the full body regardless of req.method. If the cache is empty or unavailable, HEAD requests will stream the entire file.

Suggested change

	res.writeHead(200, {
	res.writeHead(200, {
	...baseHeaders,
	"Content-Length": String(resolved.size),
	});
	if (req.method === "HEAD") {
	res.end();
	return true;
	}
	pipeline(fs.createReadStream(resolved.path), res, () => {});
	return true;

[ask-bonk]

Nit: the comment says "Accept-Encoding header values from browsers use lowercase tokens" which is accurate in practice but slightly misleading — the HTTP spec says encoding tokens are case-insensitive, and browsers happen to send lowercase. The code handles this correctly by using .includes("zstd") etc. on the raw value (which browsers always send lowercase). Just noting for accuracy since the earlier review flagged the same comment.

[ask-bonk]

Slow path also missing HEAD handling for compressed responses.

When the slow path serves a compressed response, it unconditionally pipes the compressed stream. HEAD requests should get headers only.

Suggested change

	const compressor = createCompressor(encoding);
	const compressor = createCompressor(encoding);
	res.writeHead(200, {
	...baseHeaders,
	"Content-Encoding": encoding,
	Vary: "Accept-Encoding",
	});
	if (req.method === "HEAD") {
	res.end();
	return true;
	}
	pipeline(fs.createReadStream(resolved.path), compressor, res, () => {});

[ask-bonk]

Same doc/code mismatch: comment says "brotli q5 + gzip l9 + zstd l22" but the PR description benchmarks claim "brotli q11". Should match whatever the actual values are in precompress.ts.

[ask-bonk]

Minor: all file contents are eagerly read into memory before compression begins. For a project with many large compressible assets, this could spike memory. Consider reading and compressing files within the chunked loop instead of pre-reading all of them. Currently:

1. Read ALL files into files[] array (potentially hundreds of MB)
2. Then compress in chunks

If you move the readFile into the chunk processing loop, peak memory drops from O(total_assets) to O(CONCURRENCY * max_file_size).

[ask-bonk]

Previous review flagged mtime-based ETag fragility in containerized deployments. This is still using W/"${size}-${mtimeMs}". Since this PR already addressed other review feedback (Vary on original, batched stats), noting this as a known follow-up — it's not a blocker since sirv uses the same approach, but worth tracking.

[ask-bonk]

Good coverage overall. Two test gaps to consider adding:

1. Slow path test (no cache): All tests pass a StaticFileCache to tryServeStatic. There are no tests for the cache-less fallback path (lines 452-500 of prod-server.ts), which has the HEAD and error handling issues I flagged. At minimum, one test calling tryServeStatic(req, res, clientDir, pathname, compress) without a cache argument would exercise that path.

2. Encoded pathname test: The fast path has special handling for %-encoded pathnames (lines 391-403), including a decodeURIComponent that can throw. A test with a malformed percent-encoded pathname (e.g., "/%E0%A4%A") would verify the catch returns false gracefully.

[ask-bonk]

Review posted. Here's a summary of what I flagged:

Key issues

1. Compression level doc/code mismatch — The PR description claims brotli q11 and zstd l19, but the actual code uses brotli q5 and zstd l22. The benchmarks may not reflect what this code actually produces.

2. Runtime zstd crash on older Node (bug) — negotiateEncoding returns "zstd" and createCompressor calls zlib.createZstdCompress() unconditionally, but there's no feature detection like the build-time code has. Will throw on Node versions without stable zstd.

3. Silent error swallowing in pipeline — The fast-path pipeline(createReadStream, res, () => {}) silently discards errors if a cached file is deleted between startup and request time.

4. Slow path missing HEAD request handling — The fast path (with cache) correctly skips the body for HEAD requests, but the slow path (no cache) always streams the full file body regardless of method.

5. Slow path also missing HEAD for compressed responses — Same issue in the compressed branch of the slow path.

Positive notes

• Previous review's Vary: Accept-Encoding on original variant issue was fixed
• Batched stat() calls were implemented as suggested
• 52 new tests with good coverage of the fast path
• Clean separation between precompress.ts, static-file-cache.ts, and tryServeStatic
• The --no-precompress CLI flag addresses the maintainer feedback about opt-out

github run

[NathanDrake2406]

@james-elicx should I use 11 or 5?

[james-elicx]

Higher level of compression would be good at built-time but 11 can also be a bit slow, so 5 is probably a good starting point.

[james-elicx]

Can the CLI flag be opt-in instead of opt-out as well?

[NathanDrake2406]

Can the CLI flag be opt-in instead of opt-out as well?

I added an auto detect edge for workers and nitro instead. It seems more "it just works" philosophy than opting in which is kinda nerdy

[james-elicx]

I added an auto detect edge for workers and nitro instead. It seems more "it just works" philosophy than opting in which is kinda nerdy

The reason I asked is that one of the goals is fast build-time, and not everyone may need this out-of-the-box - there's always the option to re-evaluate turning on by default in the future as well.

I ran this against the fumadocs example, which has 12 assets to pre-compress, on an M4 Max.

On the main branch I get the following:

➜ vinext (main) ✗ hyperfine --runs 5 "vp exec --filter fumadocs-docs-template vinext build"
Benchmark 1: vp exec --filter fumadocs-docs-template vinext build
  Time (mean ± σ):      2.790 s ±  0.117 s    [User: 3.834 s, System: 1.794 s]
  Range (min … max):    2.710 s …  2.997 s    5 runs

And on this branch I get the following:

➜ vinext (perf/precompressed-static-serving) ✗ hyperfine --runs 5 "vp exec --filter fumadocs-docs-template vinext build"
Benchmark 1: vp exec --filter fumadocs-docs-template vinext build
  Time (mean ± σ):      3.468 s ±  0.073 s    [User: 4.215 s, System: 2.506 s]
  Range (min … max):    3.368 s …  3.574 s    5 runs

That was almost a 25% increase in build time for me. For apps that have lots of static assets, that could be a meaningful amount of time and impact to DX, likewise with less performant machines.

[NathanDrake2406]

I added an auto detect edge for workers and nitro instead. It seems more "it just works" philosophy than opting in which is kinda nerdy

The reason I asked is that one of the goals is fast build-time, and not everyone may need this out-of-the-box - there's always the option to re-evaluate turning on by default in the future as well.

I ran this against the fumadocs example, which has 12 assets to pre-compress, on an M4 Max.

On the main branch I get the following:

➜ vinext (main) ✗ hyperfine --runs 5 "vp exec --filter fumadocs-docs-template vinext build"
Benchmark 1: vp exec --filter fumadocs-docs-template vinext build
  Time (mean ± σ):      2.790 s ±  0.117 s    [User: 3.834 s, System: 1.794 s]
  Range (min … max):    2.710 s …  2.997 s    5 runs

And on this branch I get the following:

➜ vinext (perf/precompressed-static-serving) ✗ hyperfine --runs 5 "vp exec --filter fumadocs-docs-template vinext build"
Benchmark 1: vp exec --filter fumadocs-docs-template vinext build
  Time (mean ± σ):      3.468 s ±  0.073 s    [User: 4.215 s, System: 2.506 s]
  Range (min … max):    3.368 s …  3.574 s    5 runs

That was almost a 25% increase in build time for me. For apps that have lots of static assets, that could be a meaningful amount of time and impact to DX, likewise with less performant machines.

I did some more testing and found out that the compression level itself wasn't optimised. So I tuned the compression level and got these results. It's only 34ms extra at build time compared to main and not 25% if using these compression levels.

I tuned the compression levels — the old zstd:22 was the cause of the 25% regression, not precompression itself. Here are the numbers with the new levels (br:5, gz:8, zstd:8) on 100 assets:

Build-time cost: old vs new

Config	Time	Brotli	Gzip	Zstd
Old (br:5, gz:9, zstd:22)	12,002ms	262 KB (90.8%)	297 KB (89.6%)	225 KB (92.1%)
New (br:5, gz:8, zstd:8)	34ms	262 KB (90.8%)	297 KB (89.6%)	242 KB (91.5%)

353x faster. Identical brotli/gzip output, zstd is +7% larger but still 91.5% compression.

Serving throughput (5000 reqs, 10 concurrent)

Server	req/s	p50	p99	Transferred	vs send
vinext (this PR)	30,064	0.3ms	0.7ms	11.8 MB	2.0x
sirv (SvelteKit)	17,759	0.5ms	1.9ms	12.8 MB	1.2x
send (Next.js)	14,930	0.6ms	2.1ms	139.0 MB	1.0x
vinext (main)	9,650	0.8ms	3.6ms	13.2 MB	0.6x

304 Not Modified

Server	req/s
vinext (this PR)	40,409
sirv (SvelteKit)	42,685

34ms build cost buys 3.1x faster serving vs main, 2x vs Next.js, 1.7x vs SvelteKit for 100 static assets. Could you change the levels to 5-8-8 and run the test again?

[NathanDrake2406]

I also did the chunking optimisation that Bonk suggested

[james-elicx]

baseline

➜ vinext (main) ✗ hyperfine --warmup 3 --runs 5 "vp exec --filter fumadocs-docs-template vinext build"
Benchmark 1: vp exec --filter fumadocs-docs-template vinext build
  Time (mean ± σ):      2.401 s ±  0.014 s    [User: 3.880 s, System: 1.579 s]
  Range (min … max):    2.386 s …  2.423 s    5 runs

branch

➜ witty-engine (perf/precompressed-static-serving) ✔ hyperfine --warmup 3 --runs 5 "vp exec --filter fumadocs-docs-template vinext build"
Benchmark 1: vp exec --filter fumadocs-docs-template vinext build
  Time (mean ± σ):      2.727 s ±  0.018 s    [User: 4.245 s, System: 2.274 s]
  Range (min … max):    2.703 s …  2.750 s    5 runs

so it's about a 13.5% increase in build-time with the latest changes on my M4 Pro with a warm file-system cache (which is faster for fs ops than my M4 Max due to different security software).

I still would like this to be behind an opt-in precompress assets flag initially, and we can later re-visit turning it on by default after it's stabilised.

[james-elicx]

/bigbonk review

[james-elicx]

22 is different to the number you referenced in your reply earlier - was that change reverted?

[ask-bonk]

Review: Build-time precompression + static file cache

Good work overall — the architecture is sound (build-time compression → startup cache → zero-allocation hot path), test coverage is thorough (52+ tests), and the separation between precompress.ts, static-file-cache.ts, and the refactored tryServeStatic is clean. Previous review feedback has been addressed well (Vary on original, batched stats, pipeline error handling, HEAD on slow path, zstd feature detection).

Two items need resolution before merge:

1. Maintainer asked for opt-in, code is still opt-out (blocker)

@james-elicx explicitly asked (twice) for this to be opt-in rather than the current 'auto' default that enables precompression for all non-edge builds. The latest benchmark shows a 13.5% build-time regression on fumadocs even with the tuned compression levels. The current --no-precompress flag is opt-out. This needs to be flipped so precompression is off by default and users enable it with --precompress or precompress: true.

2. Pipeline error handler is unreachable on fast path (bug)

The pipeline error callback at line 450-454 checks !res.headersSent, but res.writeHead(200, ...) was already called at line 435/437. So res.headersSent is always true when the callback fires, making the 500 response dead code. If createReadStream fails (file deleted between startup and request), the error is silently swallowed and the client gets a broken/hung connection. At minimum, destroy the response socket or log the error.

See inline comments for specifics and two smaller items.

[ask-bonk]

Blocker: precompression is still on-by-default for Node.js targets.

@james-elicx asked twice for this to be opt-in, most recently on Mar 28 after benchmarking a 13.5% build-time regression on fumadocs. The current logic enables precompression by default when there's no Cloudflare/Nitro plugin detected.

To make it opt-in, flip the 'auto' default to disabled:

Suggested change

	if (opt === "auto" && (hasCloudflarePlugin || hasNitroPlugin)) return;
	if (opt === "auto") return;

Then users who want it pass precompress: true in their vite config or --precompress on the CLI. You'd also need to add --precompress as the positive flag in cli.ts (the current --no-precompress only makes sense for opt-out semantics).

[ask-bonk]

Bug: error handler is unreachable. res.writeHead(200, ...) at line 435/437 sets res.headersSent = true. So when the pipeline callback fires with an error, the !res.headersSent guard is always false — the 500 response is dead code. The client gets a broken/hung connection.

The same pattern exists in the slow path at lines 502-506 and 520-524 — those also call writeHead before pipeline.

Since headers are already sent, you can't send a 500. But you should at least destroy the socket so the client gets a clean connection reset instead of hanging:

Suggested change

	if (extraHeaders) {
	res.writeHead(200, { ...variant.headers, ...extraHeaders });
	} else {
	res.writeHead(200, variant.headers);
	}
	if (req.method === "HEAD") {
	res.end();
	return true;
	}
	// Small files: serve from in-memory buffer (no fd open/close overhead).
	// Large files: stream from disk to avoid holding them in the heap.
	if (variant.buffer) {
	res.end(variant.buffer);
	} else {
	pipeline(fs.createReadStream(variant.path), res, (err) => {
	if (err && !res.headersSent) {
	res.writeHead(500);
	res.end();
	}
	});
	if (variant.buffer) {
	res.end(variant.buffer);
	} else {
	pipeline(fs.createReadStream(variant.path), res, (err) => {
	if (err) {
	res.destroy();
	}
	});
	}

Apply the same fix to the slow-path pipeline calls.

[ask-bonk]

Memory: all file contents are eagerly read into memory before compression begins.

The collect-then-compress pattern means peak memory is O(total_compressible_asset_size) — for a large app with 50MB of JS/CSS in assets/, all 50MB is held in files[] simultaneously, even though files are compressed in chunks of CONCURRENCY.

If you move the readFile into the chunk processing loop, peak memory drops to O(CONCURRENCY * max_file_size). The walkFiles generator already yields lazily, so you'd just need to collect paths (not contents) in the first pass, then read + compress within each chunk.

Not blocking, but worth addressing before this hits projects with large asset directories.

[ask-bonk]

Nit: PR description says "brotli q11" and "zstd l19" but code uses q5 and l22. The header comment here is accurate (q5, l22), but the PR title and description benchmarks still claim q11/l19. The PR description should be updated to match the actual compression levels so reviewers and future readers aren't confused about what was measured.

(The maintainer agreed q5 is a good default, so the code is fine — just the PR description is stale.)

[ask-bonk]

Review submitted with REQUEST_CHANGES. Here's a summary of what I flagged:

Blockers (2):

1. Precompression is still on-by-default — @james-elicx asked twice for this to be opt-in. The current 'auto' default enables it for all non-edge Node.js builds, but the latest benchmark still shows a 13.5% build-time regression on fumadocs. Needs to be flipped so users explicitly enable it with --precompress or precompress: true.

2. Pipeline error handler is unreachable — In prod-server.ts:450-454, the !res.headersSent guard is dead code because writeHead(200) was already called at line 435/437. If createReadStream fails after headers are sent, the error is silently swallowed and the client hangs. Same pattern in the slow path. Fix: call res.destroy() on error instead of trying to send a 500.

Non-blocking (2):

3. Eager readFile in precompress — All file contents are loaded into memory before compression starts. Peak memory is O(total_compressible_size) instead of O(CONCURRENCY * max_file_size). Moving readFile into the chunk loop would fix this.

4. Stale PR description — PR title/description still claim "brotli q11" and "zstd l19" but the actual code uses q5 and l22. The code is fine per maintainer agreement — just the description needs updating.

github run

[james-elicx]

shouldn't this track all three instead of just brotli?

[james-elicx]

in the worst case scenario this could be somewhat slow - potential to cache knowledge of existence of the files, or have the server be aware of the static assets that exist that it can lookup against instead of doing fs ops?

[james-elicx]

would the recursive option help here potentially?

[james-elicx]

vite config option could support custom compression levels as well as opting in

[ask-bonk]

Review submitted on PR #641. Here's a summary of what I flagged:

Key findings:

1. Slow path missing ETag/304 support (correctness) — The fast path (with cache) correctly handles conditional requests via pre-computed ETags and returns 304. The slow path (no cache) omits ETags entirely, so every response is a full 200 regardless of If-None-Match. Not a blocker since the production server always has a populated cache, but it's a parity gap worth either fixing or documenting.

2. Double stat() per file in walkFilesWithStats (robustness) — readdir({ withFileTypes: true }) already does kernel-level type detection, then stat() is called again for size/mtime. Suggested readdir({ recursive: true }) on Node 22.12+ as a simplification for a follow-up.

3. Precompression error silently swallowed if SSR closeBundle doesn't fire — The .catch() handler stores the error but doesn't log it. If the SSR environment never fires closeBundle, the error is invisible. Suggested adding console.error in the catch block.

4. Minor nits on wasted readFile for tiny files and the variant selection expression readability.

What's in good shape: All previous blockers addressed (opt-in, Vary, pipeline errors, HEAD, zstd detection, batched stats, multi-value If-None-Match, filename-hash ETag with base64url). Clean architecture, thorough test coverage (100+ tests across 3 files), well-tuned compression levels (br:5, gz:8, zstd:8), and good progress bar UX.

github run

[james-elicx]

/bigbonk review

[ask-bonk]

Review

This is a well-executed PR with strong architecture — the three-layer design (build-time precompression → startup metadata cache → zero-allocation hot path) is clean, and the iterative review process has addressed every major blocker from previous rounds (opt-in semantics, pipeline error handling, HEAD on all paths, zstd feature detection, Vary on originals, batched stats, multi-value If-None-Match, filename-hash ETag validation, progress bar).

Test coverage is excellent (100+ tests across 3 files covering fast path, slow path, HEAD, 304, malformed pathnames, multi-ETag, and encoding negotiation). The benchmarks are compelling and the 30ms build overhead for precompression is negligible.

I found one correctness issue worth fixing and a few minor items.

Severity	Count
Correctness (should fix)	1
Code quality	2
Nit	3

[ask-bonk]

Slow-path 304 missing Vary: Accept-Encoding — When the fast path returns a 304 for a file that has compressed variants, the notModifiedHeaders correctly include Vary: Accept-Encoding. But the slow-path 304 here only sends ETag and Cache-Control.

If a shared cache (CDN, reverse proxy) sees this 304 without Vary, it could collapse the cached representation — serving a compressed 200 response to a client that later gets a 304 validating it, or vice versa.

In practice this is unlikely to cause real issues (the slow path only fires when the cache is empty, which shouldn't happen in normal production), but it's a correctness gap:

Suggested change

	res.writeHead(304, { ETag: etag, "Cache-Control": cacheControl, ...extraHeaders });
	res.writeHead(304, { ETag: etag, "Cache-Control": cacheControl, Vary: "Accept-Encoding", ...extraHeaders });

Note: this should only be added when compress is true and the content type is compressible (same condition as line 515). Without that guard, files like images would incorrectly get Vary: Accept-Encoding. Alternatively, leave as-is and add a comment noting the gap.

[ask-bonk]

Progress reports after-the-fact for the last chunk. The processed += chunk.length and onProgress call happen after Promise.all completes for each chunk. This means the progress bar jumps in steps of CONCURRENCY rather than updating per-file. This is actually fine for deterministic progress (avoids the ordering issue within a chunk that earlier reviews flagged), but it means the progress bar sits at 0% until the first chunk finishes, then jumps to e.g. 12%.

Not blocking — the current approach is the right tradeoff over per-file reporting within a Promise.all chunk. Just noting the visual behavior.

[ask-bonk]

Nit: Math.floor is the more conventional choice over Math.trunc for positive-only values and matches what send (Express/Next.js) does. I see Math.floor is already used here — good.

[ask-bonk]

Nit: The entries.values() iterator here will visit duplicate entries for HTML aliases (since the same StaticFileEntry is registered under multiple keys). The bufferedPaths dedup set prevents double-reading, so this is correct. Just noting that for very large sites with many HTML pages, the outer loop iterates more entries than unique files. Not worth changing — the dedup handles it.

[ask-bonk]

This fs.existsSync call is synchronous and runs during the build. The writeBundle hook is async-capable, so this could use fsp.access() or fsp.stat() instead. However, since this runs once during build (not per-request), the sync call is acceptable. Non-blocking.

[ask-bonk]

Nice variant selection pattern. Worth noting for future readers: this intentionally does NOT check HAS_ZSTD (unlike negotiateEncoding) because it's serving a pre-existing .zst file from disk — the Node.js runtime zstd API isn't needed. The HAS_ZSTD check only matters for the slow-path's on-the-fly compression via createZstdCompress().

[ask-bonk]

Review submitted on PR #641. Here's a summary:

Overall assessment: This PR is in good shape after multiple review iterations. The three-layer architecture (build-time precompression → startup cache → zero-allocation hot path) is clean and well-tested with 100+ tests across 3 files. All previous blockers have been addressed.

Key finding (1 correctness issue):

1. Slow-path 304 missing Vary: Accept-Encoding (prod-server.ts:502) — The fast path correctly includes Vary in 304 responses via notModifiedHeaders, but the slow path's 304 only sends ETag and Cache-Control. A shared cache could mishandle this. Low risk since the slow path rarely fires in production (cache is always populated), but it's a correctness gap worth either fixing or documenting.

Minor items (2 code quality, 3 nits):

• Progress bar jumps in chunk-sized steps rather than per-file (correct tradeoff, just noting the visual behavior)
• fs.existsSync in build hook is sync but acceptable since it runs once at build time
• Confirmed the HAS_ZSTD check is intentionally omitted in the fast-path variant selection (serving pre-existing .zst files doesn't need the runtime zstd API)
• HTML alias dedup in buffer pass is correct via bufferedPaths set
• Math.floor for mtime truncation matches send/Express convention

github run

[james-elicx]

/bigbonk review

[ask-bonk]

Review

Strong PR. The three-layer architecture (build-time precompression → startup cache → zero-allocation hot path) is well-designed, the code is clean, and the test coverage is thorough (100+ tests across 3 files). The iterative review process has resolved all major blockers — opt-in semantics, pipeline error handling, HEAD on all paths, zstd feature detection, Vary on originals, batched stats, multi-value If-None-Match, filename-hash ETag with base64url validation.

I found a few items worth addressing — one correctness issue and some minor polish.

Severity	Count
Correctness (should fix)	1
Robustness	2
Code quality / nit	3

[ask-bonk]

Correctness: slow-path 304 is missing Vary: Accept-Encoding for compressible content types.

The fast path correctly includes Vary in notModifiedHeaders when compressed variants exist. The slow path conditionally includes it here via isCompressible, which is correct — but only when compress is true AND the content type is compressible.

However, there's a subtle issue: the spread ...(isCompressible ? { Vary: "Accept-Encoding" } : undefined) works, but spreading undefined in an object literal is a no-op only because JS ignores it. This is an established pattern but can confuse readers. More importantly, if compress is false (like for image optimization), compressible types like SVGs won't get Vary even if they could be served compressed in other contexts. This matches the fast-path behavior (where compress=false also skips variants), so it's consistent — just worth a brief comment noting this is intentional.

Non-blocking — the behavior is consistent between fast and slow paths.

[ask-bonk]

Robustness: slow-path ETags use a separate stat result from resolveStaticFile, but the stat was already done there. This is fine — the mtime and size come from the same stat() call. Just noting that if resolveStaticFile did HTML fallback resolution, the ETag corresponds to the resolved file (e.g., about.html), not the requested path (/about). This is correct behavior — the ETag should represent the content — but worth knowing.

[ask-bonk]

Robustness: readFile is wasted for files below MIN_SIZE (1KB).

Every file gets fully read into memory before the size check. For a project with many tiny assets (e.g., icon SVGs under 1KB), this does unnecessary I/O.

A micro-optimization would be to stat() first and skip the read entirely for small files. However, the extra syscall per file may not be worth it — readFile for sub-1KB files is nearly instantaneous, and stat() + readFile is two syscalls vs one. The current approach is likely cheaper in aggregate.

No change needed — just documenting the tradeoff for future readers.

[ask-bonk]

Nit: Math.floor here is the right call — matches send (Express/Next.js) convention and handles the truncation correctly for positive timestamps. Good.

[ask-bonk]

Code quality: the comment at line 172-173 about alias registration ordering is a nice touch. This was flagged in earlier reviews and the ordering dependency is now clearly documented. The pattern of mutating original.headers before registering aliases is correct and the comment makes the constraint visible.

[ask-bonk]

Nit: error handling is now complete. The .catch() block at line 3405 logs immediately via console.error, and closeBundle at line 3416 rethrows if the SSR environment fires. This addresses the earlier concern about errors being invisible if closeBundle doesn't run. Good.

[ask-bonk]

Code quality: the boolean short-circuit pattern for variant selection is clever but dense.

The expression (ae.includes("zstd") && entry.zst) || (ae.includes("br") && entry.br) || ... relies on the fact that false || undefined evaluates to undefined (falsy), so it falls through to the next || operand. This is correct and performant.

The surrounding comments (lines 432-441) explain the intent well, including the deliberate omission of HAS_ZSTD and the compress=false semantics. No change needed — the comments make this readable despite the terse expression.

[ask-bonk]

Good defensive regex for filename-hash ETags. The base64url character class check (/^[A-Za-z0-9_-]+$/) correctly rejects version-like suffixes (e.g., v2.0.0 in my-library-v2.0.0.js — the . fails the regex). The 6-12 length range is reasonable for Vite's 8-char hashes plus other bundlers.

One minor observation: if Rolldown ever changes its hash character set or length, this would need updating. The comment at line 249-250 already documents this assumption, which is good.

[ask-bonk]

Robustness: progress reporting is chunk-granular, not per-file. This is the right tradeoff — it avoids the non-deterministic ordering issue within Promise.all that earlier reviews flagged (where smaller files resolve before larger ones, causing the progress bar to jump backward). The visual effect is that the progress bar advances in steps of CONCURRENCY rather than per-file, but since CONCURRENCY is capped at 8, the steps are small enough.

The comment at lines 146-149 clearly explains both the decision and the semantic difference between processed and filesCompressed. Good.

[ask-bonk]

All three pipeline error handlers are consistent. Lines 467-474 (fast path), 542-548 (slow path compressed), and 562-568 (slow path uncompressed) all use the same pattern: console.warn + res.destroy(err). Good — this was the main blocker from earlier reviews and all three instances are now in sync.

Minor note: console.warn is a reasonable choice over console.error since this is an operational warning (file disappeared between startup and request), not an application-level error. Consistent with typical server framework logging patterns.

[ask-bonk]

Review submitted successfully. Here's a summary of what I flagged:

Overall assessment: This PR is in solid shape after multiple review iterations. The architecture is clean, the code is well-separated, and the test coverage is thorough.

Key findings:

1. Correctness (1): Slow-path 304 response's Vary: Accept-Encoding handling works correctly but has a subtle interaction with the compress flag — noted as consistent with fast-path behavior, non-blocking.

2. Robustness (2):

   • Slow-path ETag correctly corresponds to the resolved file (after HTML fallback), not the requested path
   • readFile for sub-MIN_SIZE files is wasted I/O but the alternative (stat first) would likely be slower in aggregate

3. Code quality/nit (3): Confirmed that error handling in closeBundle/.catch() is now complete, pipeline error handlers are consistent across all three locations, and the variant selection pattern + filename-hash ETag regex are well-documented.

No blockers. All previous review blockers (opt-in semantics, pipeline errors, HEAD on all paths, zstd feature detection, Vary on originals, batched stats, multi-value If-None-Match, progress bar) have been addressed.

github run

[james-elicx]

/bigbonk review

[ask-bonk]

Review

This is a well-architected PR. The three-layer design (build-time precompression → startup cache → zero-allocation hot path) is clean, the separation of concerns across precompress.ts, static-file-cache.ts, and the refactored tryServeStatic is solid, and the test coverage is thorough (100+ tests across 3 files). The previous review iterations have resolved all major issues — opt-in semantics, pipeline error handling, HEAD on all paths, zstd feature detection, Vary on originals, batched stats, multi-value If-None-Match, filename-hash ETag with base64url validation.

I have a few remaining items — one correctness concern worth fixing before merge, and some smaller things that could be follow-ups.

Severity	Count
Should fix	1
Non-blocking	4
Nit	2

[ask-bonk]

Should fix: readFile is unconditional but the size check happens after.

Every file in the chunk gets fully read into memory before the content.length < MIN_SIZE check. For files below 1KB, the read is pure waste. More importantly, the early return here means the progress counter for this file is only tracked at the chunk level (line 150), not at the file level — so onProgress can't report which specific file was skipped.

This is fine for the current use case (most hashed assets are well above 1KB), but if you want to tighten it up later, you could stat() first and skip the read for tiny files. The tradeoff is one extra syscall per file vs one wasted read per tiny file — probably not worth changing now, but worth a brief comment:

Suggested change

	if (content.length < MIN_SIZE) return;
	if (content.length < MIN_SIZE) return; // Below threshold — skip (readFile already done; stat() would add a syscall)

[ask-bonk]

Should fix: slow-path mtime ETag is computed from resolved.mtimeMs but resolved came from resolveStaticFile which may have followed an HTML fallback.

This is actually correct — the ETag represents the resolved file (e.g. about.html, not /about), which is the right behavior. But there's a subtle issue: resolveStaticFile calls statIfFile up to 3 times (exact path, .html fallback, index.html fallback). Each stat() is a separate syscall. When the slow path then uses resolved.mtimeMs for the ETag, the stat result is already available — good.

However, the slow path has no protection against TOCTOU races: the file could be modified between the stat() in resolveStaticFile and the createReadStream below. The ETag would reflect the old mtime while the body contains the new content. This is inherent to mtime-based ETags and matches what send (Express/Next.js) does, so not a blocker — just documenting the known limitation.

[ask-bonk]

Non-blocking: slow-path compressed response creates compressor even for HEAD requests that skip it.

The HEAD check at line 540 correctly returns before piping, but the negotiateEncoding(req) call at line 531 and the res.writeHead at line 535 both run. The createCompressor at line 544 only runs after the HEAD check, so this is actually fine — the compressor is not created for HEAD. Good.

(I initially misread this as creating the compressor before the HEAD check. The ordering is correct.)

[ask-bonk]

Non-blocking: buffer threshold check uses variant.size (the numeric field on FileVariant).

Previous bonk reviews flagged a parseInt round-trip concern, but looking at the actual code, variant.size is already a number on the FileVariant type (line 52). The comparison variant.size <= BUFFER_THRESHOLD is a clean numeric check with no string parsing. Good — this was a false positive from earlier reviews.

[ask-bonk]

Nit: Math.floor is used here which is correct. Previous reviews debated Math.trunc vs Math.floor — Math.floor is the right choice and matches send (Express/Next.js). For positive timestamps they're equivalent, but Math.floor is the conventional choice. No change needed.

[ask-bonk]

Non-blocking: the opt-in gate correctly checks both options.precompress and process.env.VINEXT_PRECOMPRESS.

This addresses the maintainer's request. One thing to note: the env var approach means VINEXT_PRECOMPRESS=1 set externally (not via CLI) also enables precompression. This is probably fine (gives CI pipelines a way to enable it without touching vite config), but worth documenting in the JSDoc on the precompress option:

Suggested change

	if (!options.precompress && process.env.VINEXT_PRECOMPRESS !== "1") return;
	if (!options.precompress && process.env.VINEXT_PRECOMPRESS !== "1") return;

(No code change — just noting the env var should be mentioned in the VinextOptions.precompress JSDoc above.)

[ask-bonk]

Non-blocking: the base64url regex and length range are reasonable but the comment should mention the Rolldown migration.

The comment says "Vite emits 8-char base64url hashes; allow 6-12 for other bundlers. If Rolldown changes its hash length, update this range." This is good forward-looking documentation. Since vinext is on Vite 8 / Rolldown already, it would be useful to verify what hash length Rolldown actually emits and narrow the range if possible. Not a blocker — the 6-12 range is safe.

[ask-bonk]

Review submitted successfully. Here's a summary of what I flagged on PR #641:

Overall assessment: The PR is in solid shape after multiple review iterations. The three-layer architecture (build-time precompression → startup cache → zero-allocation hot path) is clean, well-separated, and thoroughly tested with 100+ tests across 3 files. All previous blockers have been addressed (opt-in semantics, pipeline error handling, HEAD on all paths, zstd feature detection, Vary on originals, batched stats, multi-value If-None-Match, filename-hash ETag validation).

Items flagged (7 inline comments):

Severity	Item
Should fix (1)	Minor: readFile runs unconditionally before the MIN_SIZE check — wasted I/O for tiny files. Comment suggestion for documentation.
Non-blocking (4)	TOCTOU race in slow-path mtime ETag (matches send/Next.js behavior); opt-in gate env var should be documented in JSDoc; buffer threshold uses clean numeric check (false positive from earlier reviews); Rolldown hash length verification
Nit (2)	Math.floor usage is correct and conventional; slow-path compressor creation ordering is correct

No blockers found. The PR is ready for merge pending maintainer approval.

github run

[james-elicx]

/bigbonk review

[ask-bonk]

Review

This is a well-architected PR. The three-layer design (build-time precompression → startup cache → zero-allocation hot path) is clean and the code quality is high. The iterative review process with bonk has addressed all the major issues that were raised, and the benchmarks are compelling. I read through all 8 changed files and have a few remaining observations — one correctness concern, one code health item, and some minor things.

Summary

Severity	Count
Correctness (should fix)	1
Code health	1
Non-blocking / nit	3

What's in good shape

• Opt-in semantics per maintainer request — precompression is off by default
• Pipeline error handling with console.warn + res.destroy(err) across all three locations
• HEAD on both fast and slow paths
• zstd feature detection via HAS_ZSTD for on-the-fly compression, intentionally omitted for serving precompressed files
• Vary: Accept-Encoding on original variants when compressed siblings exist
• Multi-value If-None-Match via matchesIfNoneMatchHeader
• Filename-hash ETag with base64url character class validation
• Bounded concurrency for both compression and buffer reads
• Progress bar with chunk-granular reporting (avoids the non-deterministic ordering issue)
• Thorough test coverage (100+ tests across 3 files, including slow path, HEAD, 304, malformed pathnames, encoding negotiation)

[ask-bonk]

Correctness: slow-path ETag uses mtime even for hashed assets.

The fast path (cache) uses etagFromFilenameHash() to extract content hashes from Vite filenames like app-DqZc3R4n.js, producing stable ETags that survive deploys. The slow path always uses the mtime-based ETag W/"${size}-${mtime}", even for files under /assets/. This means:

1. If the slow path ever fires for a hashed asset (cache miss, race at startup), returning visitors who got a filename-hash ETag from the fast path will never match the mtime ETag from the slow path — they'll re-download the file unnecessarily.
2. The mtime ETag changes across deploys (Docker, CI) even though the file content is identical.

In practice the slow path almost never fires for production traffic (the cache is populated at startup), so this is low-risk. But for consistency it would be straightforward to apply the same etagFromFilenameHash logic here:

Suggested change

	const etag = `W/"${resolved.size}-${Math.floor(resolved.mtimeMs / 1000)}"`;
	const etag = (isHashed && etagFromFilenameHash(pathname)) || `W/"${resolved.size}-${Math.floor(resolved.mtimeMs / 1000)}"`;

This would require exporting etagFromFilenameHash from static-file-cache.ts (or extracting it to a shared util) and adjusting the signature since the slow path has pathname not relativePath. Not blocking merge, but worth tracking as a follow-up if not done here.

[ask-bonk]

Code health: the buffer pass iterates entries.values() which yields duplicates for HTML aliases.

Since HTML files get registered under both /about.html and /about (sharing the same StaticFileEntry by reference), the outer loop visits the same entry twice. The bufferedPaths dedup set prevents double-reading, which is correct.

However, the toBuffer array itself can contain duplicate FileVariant objects (from duplicate StaticFileEntry references pointing to the same variant). For example, if about.html is under 64KB, its original variant gets pushed to toBuffer once (second push is skipped by bufferedPaths), but the iteration overhead grows linearly with the number of HTML aliases.

This is fine for current scale, but if you wanted to tighten it, you could deduplicate at the entry level rather than the variant level:

const seenEntries = new Set<StaticFileEntry>();
for (const entry of entries.values()) {
  if (seenEntries.has(entry)) continue;
  seenEntries.add(entry);
  for (const variant of [entry.original, entry.br, entry.gz, entry.zst]) {
    if (!variant || variant.size > BUFFER_THRESHOLD) continue;
    toBuffer.push(variant);
  }
}

Non-blocking — the current approach is correct.

[ask-bonk]

Nit: this pathname === "/" early return is duplicated inside cache.lookup() (line 225 of static-file-cache.ts) and also before the slow path is reached (since the callers in startAppRouterServer and startPagesRouterServer already skip /). Three layers of the same check. Not wrong — belt-and-suspenders — but worth being aware it's defense-in-depth, not because any single layer is unreliable.

[ask-bonk]

Nit: path.basename(chunk[chunk.length - 1]) in the progress callback reports the last file in the chunk, not the last file to finish processing. Since Promise.all resolves when all items complete, the progress message always shows the last filename alphabetically (or by insertion order), regardless of which file took longest. This is fine — it's cosmetic — but could briefly show a different filename than what's actually compressing if someone reads the progress bar closely.

[ask-bonk]

Nit: writeBundle.handler is not marked async — it starts precompression as a fire-and-forget promise. This is intentional (the comment at line 3378-3381 explains the overlapping strategy), but it means errors from the async IIFE are caught by .catch() at line 3407 rather than propagating through Vite's plugin pipeline. The closeBundle handler picks up the error and rethrows it, which is correct.

One subtle edge: if the client writeBundle hook fires but the build is aborted before the SSR closeBundle fires (e.g., process.exit() in another plugin), the .catch() handler logs the error but the process may exit before the log is flushed. This is a rare edge case and the current approach is pragmatic — just documenting the limitation.

[ask-bonk]

Review posted successfully. Here's a summary of what I flagged on PR #641:

Overall assessment: The PR is in solid shape after extensive iterative review. The three-layer architecture (build-time precompression → startup cache → zero-allocation hot path) is well-designed, and all previous blockers have been addressed.

Key findings (5 inline comments):

Severity	Item
Correctness (should fix)	Slow-path ETag uses mtime even for hashed assets under /assets/, while the fast path uses filename-hash ETags — returning visitors who got a filename-hash ETag from the cache path won't match the mtime ETag from the slow path
Code health	Buffer pass iterates entries.values() which yields duplicate entries for HTML aliases; the bufferedPaths dedup handles it correctly but could be tightened
Nit (3)	Triple-layered pathname === "/" defense-in-depth; progress bar shows last filename in chunk not last to finish; writeBundle handler's fire-and-forget pattern has a subtle edge case on early process exit

github run

[james-elicx]

/bigbonk review

[ask-bonk]

Review

This is a well-architected PR. The three-layer design (build-time precompression → startup metadata cache → zero-allocation hot path) is clean, the code quality is high, and the test coverage is thorough. The iterative review process has addressed all major issues from prior rounds — opt-in semantics, pipeline error handling, HEAD on all paths, zstd feature detection, Vary on original variants, batched stat() calls, multi-value If-None-Match, and filename-hash ETag validation.

I reviewed the code with fresh eyes and have a few items to flag. One is a correctness issue that could cause confusion, the rest are minor.

Severity	Count
Correctness (should fix)	1
Robustness	1
Code quality / nit	3

[ask-bonk]

Correctness: etagFromFilenameHash receives pathname but ext from the resolved path — these can diverge after HTML fallback.

If a request for /assets/widget-abc123 resolves to assets/widget-abc123.html via the HTML fallback, then:

• pathname = /assets/widget-abc123 (no extension)
• ext = .html (from resolved.path)
• etagFromFilenameHash("/assets/widget-abc123", ".html") → basename("/assets/widget-abc123", ".html") → "widget-abc123" → extracts "abc123" → W/"abc123"

But the actual content being served is the .html file, not the JS file the filename hash was generated for. The ETag would be semantically wrong — it identifies content from a different file.

In practice this is unlikely for hashed assets (they always have extensions), but it's a latent bug that would be hard to diagnose. Consider using resolved.path instead of pathname for the ETag computation, or at least using the same path for both ext and the filename-hash extraction:

Suggested change

	(isHashed && etagFromFilenameHash(pathname, ext)) ||
	const etag =
	(isHashed && etagFromFilenameHash(resolved.path, ext)) ||
	`W/"${resolved.size}-${Math.floor(resolved.mtimeMs / 1000)}"`;

This way both arguments come from the same resolved file.

[ask-bonk]

Robustness: walkFiles collects .br/.gz/.zst files, then the COMPRESSIBLE_EXTENSIONS filter lets them through to get re-compressed on re-runs.

The JSDoc on line 78 says "never compresses .br, .gz, or .zst files themselves" — which is true because .br etc. aren't in COMPRESSIBLE_EXTENSIONS. But this depends on an implicit invariant rather than an explicit guard. If someone later adds .br to COMPRESSIBLE_EXTENSIONS (unlikely but not impossible), the safe-to-rerun claim breaks.

The current approach works, just noting it's worth a brief inline comment on the filter to make the invariant explicit:

// .br/.gz/.zst are not in COMPRESSIBLE_EXTENSIONS, so precompressed
// variants are never re-compressed on re-runs.

[ask-bonk]

Nit: Math.floor for positive timestamps is correct and matches send/Express. Worth noting that if two files in the same directory have different content but identical size and their mtimeMs values fall within the same second (e.g. during a fast automated copy), they'll share an ETag. Astronomically unlikely for non-hashed public files in practice — just documenting the theoretical collision.

[ask-bonk]

Nit: the boolean short-circuit pattern for variant selection is dense but correct. The surrounding comments (lines 432-441) do a good job explaining the intent, including the deliberate omission of HAS_ZSTD for pre-existing files and the compress=false semantics.

One edge case worth documenting (maybe inline or in the module header): if a future caller passes compress: false for a request where precompressed variants exist (e.g. serving an SVG from /assets/), they'll silently get the uncompressed original. The NOTE comment on line 435-438 covers this well — just flagging that it's the kind of semantic coupling that benefits from a test case to lock down the behavior.

[ask-bonk]

Nit: path.basename(chunk[chunk.length - 1]) always reports the last file in the array, not the last file to finish compressing. Within a Promise.all chunk, a 200KB JS file might still be compressing while a 2KB CSS file finished long ago. The progress message will show the JS filename regardless. This is fine — the progress bar is informational and converges to 100% either way.

[ask-bonk]

Review submitted successfully. Here's a summary of what I flagged:

Correctness (1 item):

• etagFromFilenameHash in the slow path receives pathname (original request) but ext from the resolved file path — these can diverge after HTML fallback resolution, producing a semantically incorrect ETag. Suggested using resolved.path for both.

Robustness (1 item):

• The invariant that precompressed files (.br/.gz/.zst) are never re-compressed depends on them not being in COMPRESSIBLE_EXTENSIONS. This works but is implicit — suggested adding a clarifying comment.

Nits (3 items):

• Mtime ETag theoretical collision for same-size files within the same second (documented, not actionable)
• The compress: false dual meaning (skips both on-the-fly and precompressed variants) is well-documented in comments but would benefit from a test case
• Progress bar reports the last filename in the chunk array, not the last file to finish — cosmetic only

github run

[james-elicx]

/bigbonk review

[ask-bonk]

Review

This is a well-architected PR. The three-layer design (build-time precompression → startup metadata cache → zero-allocation hot path) is clean, the code is well-separated across modules, and the test coverage is thorough. The iterative review process with bonk and the maintainer has resolved all the major blockers — opt-in semantics, pipeline error handling, HEAD on all paths, zstd feature detection, Vary on original variants, batched stat(), multi-value If-None-Match, filename-hash ETag with base64url validation.

I've reviewed all 8 changed files and have a few remaining items to flag — one correctness gap that should be addressed before merge, one minor bug, and a few code-quality observations.

Severity	Count
Should fix	2
Non-blocking	3
Nit	2

[ask-bonk]

Should fix: slow-path ETag for hashed assets diverges from fast-path ETag.

The fast path (cache) uses etagFromFilenameHash() to extract content hashes from Vite filenames, producing ETags like W/"DqZc3R4n" that are stable across deploys. The slow path always falls through to the mtime-based ETag W/"${size}-${mtime}" because etagFromFilenameHash receives resolved.path (an absolute filesystem path like /home/user/dist/client/assets/app-DqZc3R4n.js), but path.basename(relativePath, ext) inside etagFromFilenameHash will correctly extract the hash from that absolute path since path.basename strips the directory.

Wait — actually re-reading the code, etagFromFilenameHash(resolved.path, ext) does call path.basename(resolved.path, ext) which would yield app-DqZc3R4n, so the hash extraction should work correctly. The slow path and fast path should produce the same ETag for hashed assets. This is correct — ignore my initial concern.

However, the slow path has no test coverage for 304 responses. Adding a test like the fast-path 304 tests would confirm this parity and prevent regressions:

it("slow path returns 304 for matching ETag", async () => {
  await writeFile(clientDir, "assets/etag-slow-xxx.js", "content");
  const req = mockReq(undefined, { "if-none-match": '<expected etag>' });
  // ...
});

[ask-bonk]

Should fix: slow-path compressed response emits Vary: Accept-Encoding on 200 but the 304 at line 522 only conditionally includes it via isCompressible.

This is actually consistent — the 304 includes Vary when isCompressible is true (line 522), and the 200 compressed branch also requires isCompressible to be true (line 537). So the Vary header is consistent between 200 and 304 on the slow path. Good.

The real gap is the missing Content-Length comment you already have on line 540-541. Worth noting: this means the slow path's compressed 200 can't be used for resumable downloads or progress indicators, but that's an inherent limitation of on-the-fly compression and is fine.

[ask-bonk]

Should fix (minor): onProgress is called with processed counting all files including skipped ones, but the final log message at index.ts:3398 says "Precompressed N assets" using filesCompressed — these two counters can diverge.

If a project has 100 compressible files but 20 are below MIN_SIZE, the progress bar reaches 100/100, but the summary says "Precompressed 80 assets". This is slightly confusing but not a bug. The comment on lines 151-154 documents this well.

The actual minor bug: if all files in a chunk are below MIN_SIZE, the path.basename(chunk[chunk.length - 1]) in the progress callback reports the name of a file that was skipped, not one that was compressed. Again, cosmetic — but worth noting.

[ask-bonk]

Non-blocking: double stat() per file.

readdir({ withFileTypes: true }) already calls fstatat (or equivalent) for type detection, then fsp.stat() here calls it again for size/mtime. This is a known limitation since Dirent doesn't expose size/mtimeMs.

On Node 22+, fsp.readdir with { recursive: true } could flatten the directory walk into a single call, but it still wouldn't give you size/mtime. The current batched approach is a reasonable tradeoff.

One potential follow-up: since readdir with withFileTypes: true already identifies files vs directories, you could skip the isFile() check inside stat() and just call fsp.stat() directly — the Dirent.isFile() check at line 306 already filtered non-files. This would save the redundant stat.isFile() check, though the savings are negligible.

[ask-bonk]

Nit: triple pathname === "/" guard.

This check appears here (line 395), inside cache.lookup() (line 228 of static-file-cache.ts), and callers in startAppRouterServer/startPagesRouterServer also skip / before calling tryServeStatic. Three layers of the same guard. Belt-and-suspenders is fine for a security-relevant path, but worth knowing it's intentional defense-in-depth.

[ask-bonk]

Non-blocking: Math.floor is the right choice here. Matches what send (Express/Next.js) does and is correct for positive timestamps. For the record: Math.floor and Math.trunc differ for negative numbers (pre-1970 mtimes), but those are astronomically unlikely. Good as-is.

[ask-bonk]

Non-blocking: fire-and-forget pattern has a subtle edge case on process.exit().

The writeBundle handler starts precompression as an untracked promise (the pendingPrecompress reference is the only way to await it). If the build process is killed between writeBundle completing and closeBundle firing (e.g., process.exit() in another plugin, or SIGTERM), the precompression work is abandoned mid-write. This could leave partial .br/.gz/.zst files on disk.

Since precompressAssets overwrites existing files on re-run, this is self-healing on the next build. Not a blocker — just documenting the edge case.

[ask-bonk]

Nit: well-documented variant selection pattern.

The boolean short-circuit pattern is dense ((ae.includes("zstd") && entry.zst) || ...) but the surrounding comments (lines 432-441) do an excellent job explaining the intent, including the deliberate omission of HAS_ZSTD for pre-existing files and the compress=false dual semantics. This is one of the better-documented hot paths I've seen.

[ask-bonk]

Review submitted successfully. Here's a summary of what I flagged:

Overall assessment: The PR is in solid shape after extensive iterative review. The three-layer architecture (build-time precompression → startup cache → zero-allocation hot path) is clean, well-separated across modules, and thoroughly tested with 100+ tests across 3 files. All previous blockers have been addressed (opt-in semantics, pipeline error handling, HEAD on all paths, zstd feature detection, Vary on originals, batched stats, multi-value If-None-Match, filename-hash ETag validation).

Items flagged (7 inline comments):

Severity	Item
Should fix (1)	Missing test coverage for slow-path 304 responses — the slow path has ETag support but no test confirms it works, unlike the fast path which has dedicated 304 tests
Should fix (1)	Minor progress bar confusion — processed counts all files including skipped ones below MIN_SIZE, while the summary log uses filesCompressed (well-documented, but the progress callback can report a skipped file's name)
Non-blocking (3)	Double stat() per file (inherent Dirent limitation), fire-and-forget edge case on process.exit(), Math.floor correctness confirmation
Nit (2)	Triple pathname === "/" defense-in-depth, praise for well-documented variant selection pattern

github run

[james-elicx]

/bigbonk review

[ask-bonk]

Review: Build-time precompression + startup metadata cache

This is a well-executed PR. The three-layer architecture (build-time precompression → startup cache → zero-allocation hot path) is clean, the code is well-separated across modules, and the iterative review process with the maintainer has resolved all the major issues. CI is green across all checks.

What's done well:

• Clean separation: precompress.ts (build), static-file-cache.ts (startup), refactored tryServeStatic (serving)
• Opt-in semantics per maintainer request — --precompress flag / precompress: true
• All sync FS calls replaced with async equivalents (or eliminated via cache)
• Pipeline error handling with console.warn + res.destroy(err) consistently across all three locations
• HEAD on both fast and slow paths
• zstd feature detection via HAS_ZSTD for on-the-fly compression, intentionally omitted for serving pre-existing .zst files
• Vary: Accept-Encoding on original variants when compressed siblings exist
• Multi-value If-None-Match via matchesIfNoneMatchHeader
• Filename-hash ETag with base64url character class validation
• Bounded concurrency for compression (CONCURRENCY cap at 8) and buffer reads (chunked at 64)
• Progress bar with chunk-granular reporting
• Thorough test coverage (100+ tests across 3 files, covering fast path, slow path, HEAD, 304, malformed pathnames, encoding negotiation, slow-path 304)
• Compression levels (br:5, gz:8, zstd:8) well-tuned per benchmarks — 30ms build overhead for 100 assets

I have two small non-blocking suggestions and a couple of nits below. Nothing that should block merge.

[ask-bonk]

Nit: Worth a one-line comment for future readers since the fast path always has Content-Length. Something like:

Suggested change

	res.writeHead(200, {
	// Content-Length omitted intentionally: compressed size isn't known
	// ahead of time, so Node.js uses chunked transfer encoding.
	// (The fast path avoids this by pre-computing sizes from .br/.gz/.zst files.)
	res.writeHead(200, {

[ask-bonk]

The notModifiedHeaders intentionally exclude Content-Type (matching the test at serve-static.test.ts:387), which is good for RFC 9110 compliance. Just confirming this was a deliberate design choice — it's the right call.

[ask-bonk]

Minor: os.availableParallelism() was added in Node 19.4 / backported to 18.14. Since the repo requires engines.node >= 22, this is fine — just noting for awareness. If vinext ever widens engine support, this would need a fallback to os.cpus().length.

[ask-bonk]

The boolean short-circuit variant selection is dense but correct, and the comments above (lines 432-441) do a great job explaining the design decisions — particularly the intentional omission of HAS_ZSTD for pre-existing files and the compress=false dual semantics.

One thought for a potential follow-up: if this pattern ever needs a fourth encoding, it might be cleaner to extract it into a small helper function. For now this is fine.

[ask-bonk]

The env var VINEXT_PRECOMPRESS is documented in the JSDoc on the precompress option (line 832), which is good. One subtle behavior worth noting: this means VINEXT_PRECOMPRESS=1 vinext build enables precompression even without --precompress on the CLI or precompress: true in config. This is a useful escape hatch for CI pipelines — just make sure it's mentioned in user-facing docs when those are written.

[ask-bonk]

The base64url regex + 6-12 length range is a good defensive check. The test at static-file-cache.test.ts:136 verifying my-library-v2.0.0.js falls back to mtime confirms this works correctly.

Since vinext is on Vite 8 / Rolldown, it would be worth verifying the actual hash length Rolldown emits in a follow-up and potentially narrowing the range if it's always 8 chars. The comment here is good forward-looking documentation.

[ask-bonk]

Progress is chunk-granular rather than per-file, which avoids the non-deterministic ordering issue within Promise.all. Good tradeoff — with CONCURRENCY capped at 8, the visual steps are small enough.

One cosmetic note: path.basename(chunk[chunk.length - 1]) always shows the last filename in the array, not the last to finish compressing. This is fine since it converges to 100% either way.

[ask-bonk]

Review submitted successfully. Here's a summary of what I reviewed and flagged:

Overall assessment: APPROVED — This PR is in solid shape after extensive iterative review with the maintainer.

The PR implements a three-layer static file serving architecture:

1. Build-time (precompress.ts): opt-in .br/.gz/.zst generation at compression levels br:5, gz:8, zstd:8
2. Startup (static-file-cache.ts): pre-computed response headers + in-memory buffers for small files
3. Per-request (refactored tryServeStatic): zero FS calls, Map.get() → ETag check → pipe

All previous review blockers have been addressed (opt-in semantics, pipeline error handling, HEAD on all paths, zstd feature detection, Vary on originals, batched stats, multi-value If-None-Match, filename-hash ETag validation). CI is fully green across all 20 checks.

I left 7 inline comments — all non-blocking nits and observations, no blockers.

github run