feat: support zstd for compressPublicAssets #3934
Conversation
|
@aaharu is attempting to deploy a commit to the Nitro Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughAdds Zstandard (zstd) support across config, docs, compression tooling, public-asset metadata/serving, and tests so Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/utils/compress.ts (2)
21-33: Hoist invariant computations outside the loop.The configuration parsing (lines 21-32) and
zstdSupportedcheck (line 33) produce the same values for every file but are computed inside the per-file callback. Consider hoisting these beforePromise.allto avoid redundant work.♻️ Suggested refactor
export async function compressPublicAssets(nitro: Nitro) { + const compressPublicAssets = nitro.options.compressPublicAssets; + if (compressPublicAssets === false) { + return; + } + + const { + gzip = false, + brotli = false, + zstd = false, + } = compressPublicAssets === true + ? { gzip: true, brotli: true, zstd: true } + : compressPublicAssets; + const zstdSupported = zlib.zstdCompress !== undefined; + const publicFiles = await glob("**", { cwd: nitro.options.output.publicDir, absolute: false, dot: true, ignore: ["**/*.gz", "**/*.br", "**/*.zst"], }); await Promise.all( publicFiles.map(async (fileName) => { - const compressPublicAssets = nitro.options.compressPublicAssets; - if (compressPublicAssets === false) { - return; - } - - const { - gzip = false, - brotli = false, - zstd = false, - } = compressPublicAssets === true - ? { gzip: true, brotli: true, zstd: true } - : compressPublicAssets; - const zstdSupported = zlib.zstdCompress !== undefined; const filePath = resolve(nitro.options.output.publicDir, fileName);
81-97: Use explicit encoding check for zstd branch.Line 85 uses
else if (zstdSupported)instead ofelse if (encoding === "zstd"). While the encodings array filters out zstd when unsupported, this condition:
- Is less clear about intent than an explicit encoding check
- Rechecks a condition already verified at line 58
- Could theoretically leave the Promise unresolved if
zstdSupportedis false♻️ Suggested fix
if (encoding === "gzip") { zlib.gzip(fileContents, gzipOptions, cb); } else if (encoding === "br") { zlib.brotliCompress(fileContents, brotliOptions, cb); - } else if (zstdSupported) { + } else { + // encoding === "zstd" (zstd support verified in encodings filter) zlib.zstdCompress( fileContents, {
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
docs/3.config/0.index.mdsrc/build/virtual/public-assets.tssrc/runtime/internal/static.tssrc/types/config.tssrc/utils/compress.tstest/presets/cloudflare-pages.test.ts
🧰 Additional context used
📓 Path-based instructions (5)
src/**/*.{ts,js,tsx,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.{ts,js,tsx,jsx}: Usepathefor cross-platform path operations instead of Node.jsnode:path
Use ESM and modern JavaScript
Do not add comments explaining what the line does unless prompted
Files:
src/runtime/internal/static.tssrc/build/virtual/public-assets.tssrc/types/config.tssrc/utils/compress.ts
src/**/*.{ts,js}
📄 CodeRabbit inference engine (AGENTS.md)
Use
unstoragefor storage abstraction
Files:
src/runtime/internal/static.tssrc/build/virtual/public-assets.tssrc/types/config.tssrc/utils/compress.ts
src/runtime/**/*.{ts,js}
📄 CodeRabbit inference engine (AGENTS.md)
src/runtime/**/*.{ts,js}: Runtime code insrc/runtime/must be runtime-agnostic: do not use Node.js-specific APIs unless behind runtime checks
In runtime code, prefer Web APIs (fetch, Request, Response, URL, etc.) over Node.js APIs
In runtime code, use onlyconsolefor logging (noconsola)
Keep runtime code minimal and side-effect free to reduce bundle size
Files:
src/runtime/internal/static.ts
src/{build,dev,runner,cli}/**/*.{ts,js}
📄 CodeRabbit inference engine (AGENTS.md)
Use
consolafor logging in build/dev code; usenitro.loggerwhen available
Files:
src/build/virtual/public-assets.ts
src/build/**/*.{ts,js}
📄 CodeRabbit inference engine (AGENTS.md)
Virtual modules must be registered in
src/build/virtual.ts
Files:
src/build/virtual/public-assets.ts
🧠 Learnings (1)
📚 Learning: 2025-12-24T11:45:17.435Z
Learnt from: CR
Repo: nitrojs/nitro PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-24T11:45:17.435Z
Learning: Applies to src/runtime/**/*.{ts,js} : Keep runtime code minimal and side-effect free to reduce bundle size
Applied to files:
src/runtime/internal/static.ts
🧬 Code graph analysis (1)
src/utils/compress.ts (1)
src/types/nitro.ts (1)
Nitro(16-37)
🔇 Additional comments (14)
src/types/config.ts (1)
350-354: LGTM! Type definition correctly extends compression options.The
zstdoption follows the same pattern asgzipandbrotli, maintaining consistency in the API design.src/build/virtual/public-assets.ts (2)
39-39: LGTM! MIME type detection correctly handles zstd extension.The regex now strips
.zstalongside.gzand.brto determine the correct MIME type of compressed assets.
61-63: LGTM! Encoding detection follows established pattern.The zstd encoding assignment correctly mirrors the existing logic for gzip and brotli compressed assets.
test/presets/cloudflare-pages.test.ts (1)
47-47: LGTM! Test snapshot correctly updated for zstd variants.The exclusion list now includes
.zstcompressed variants alongside existing.gzand.brfiles, ensuring Cloudflare Pages routes are correctly configured for all compression formats.Also applies to: 58-58, 61-61, 66-66
docs/3.config/0.index.md (2)
176-180: LGTM! Documentation accurately reflects zstd support.The default configuration correctly shows
zstd: falsefor backward compatibility, and the description clearly explains that zstd is now supported alongside gzip and brotli for pre-compression of assets larger than 1024 bytes.
181-231: LGTM! Comprehensive list of compressible MIME types.The documented MIME type list provides clear guidance on which asset types will be compressed. This is helpful for users to understand what will be affected when enabling compression.
src/runtime/internal/static.ts (1)
18-18: LGTM! Runtime encoding map extended appropriately.The addition of
zstd: ".zst"follows the existing pattern and keeps the runtime code minimal and side-effect free, adhering to coding guidelines. Thesrc/utils/compress.tsutility properly handles zstd support through capability detection—checking ifzlib.zstdCompressis defined at runtime rather than assuming a specific Node.js version, with appropriate guards around all zstd operations.src/utils/compress.ts (7)
1-7: LGTM!Imports are appropriate and follow the coding guidelines - using
pathefor path operations and ESM imports throughout.
9-10: LGTM!Clean encoding-to-suffix mapping with proper
as constassertion for type safety.
36-42: Verify: Early return prevents adding new encoding formats to pre-compressed files.The OR logic causes the function to skip a file entirely if any enabled compression format already exists. This means:
- If a user has existing
.gzfiles and later enableszstd, no.zstfiles will be generated for those assets.- The per-encoding check at lines 65-67 would handle this correctly, making this early return an optimization that changes behavior.
Is this intentional? If users are expected to regenerate all compressed assets when adding a new format, consider documenting this behavior. Otherwise, consider changing to AND logic or removing this early check.
44-53: LGTM!Appropriate criteria for skipping compression: minimum file size, source maps, and non-compressible MIME types.
55-59: LGTM!Clean encodings array construction with proper type guard. The
zstd && zstdSupportedcheck ensures zstd is only included when both configured and available.
107-166: LGTM!Helper functions are unchanged and correctly identify compressible MIME types. The comprehensive MIME type list from AWS CloudFront documentation is appropriate.
86-96: The runtime check at line 33 (zlib.zstdCompress !== undefined) is already in place and properly guards all usage ofzstdCompressthroughout the file, gracefully handling Node.js versions < 22.15.0 where the API is unavailable. No action needed.
src/utils/compress.ts
Outdated
| fileContents, | ||
| { | ||
| params: { | ||
| [zlib.constants.ZSTD_c_compressionLevel]: 19, |
There was a problem hiding this comment.
It sees too much CPU consumption. For JS assets, 3-5 might be enough
There was a problem hiding this comment.
| [zlib.constants.ZSTD_c_compressionLevel]: 19, | |
| [zlib.constants.ZSTD_c_compressionLevel]: 4, |
There was a problem hiding this comment.
I agree, but the documentation stated The best compression level is used., so I was doing it this way. I will consider changing the documentation as well.
There was a problem hiding this comment.
Default is 3 (most balanced), i think we can leave it as is and reword docs as default/balanced
commit: |
2 participants
🔗 Linked issue
resolves #2772
❓ Type of change
📚 Description
Added zstd support to compressPublicAssets with zlib.zstdCompress.
📝 Checklist