fix: handle SVGs in image optimization to match Next.js behavior#236
fix: handle SVGs in image optimization to match Next.js behavior#236southpolesteve merged 1 commit intomainfrom
Conversation
commit: |
|
|
/bigbonk review please |
There was a problem hiding this comment.
Review: SVG Image Optimization
This PR correctly implements SVG handling to match Next.js behavior across the client-side image component, server-side optimization, and production server paths. The approach is sound.
What works well
- Client-side auto-skip logic (
image.tsx): Whensrcends with.svganddangerouslyAllowSVGis not enabled,unoptimizedis automatically set totrueso SVGs bypass/_vinext/imageentirely. This matches Next.js'sget-img-props.tsbehavior. - Server-side passthrough (
image-optimization.ts): SVGs that reach the optimizer skip sharp processing (correct — SVG is vector, resize is meaningless) and get security headers applied. - Security headers: CSP (
script-src 'none'),X-Content-Type-Options: nosniff, andContent-Dispositiondefaults all match Next.js. - Config propagation:
dangerouslyAllowSVGis injected into client builds viaprocess.env.__VINEXT_IMAGE_DANGEROUSLY_ALLOW_SVGdefine, and into server builds viavinextConfig.imagesserialization andimage-config.json. - Test coverage: 149 lines of new tests covering
isSafeImageContentTypewith/without SVG flag,handleImageOptimizationSVG passthrough/blocking, security headers, custom CSP/Content-Disposition, and client-side auto-skip logic. Solid coverage.
Coverage across all server paths
| Path | SVG handling | Status |
|---|---|---|
image-optimization.ts (Workers) |
SVG passthrough + security headers | ✅ |
prod-server.ts App Router |
Reads image-config.json, applies SVG blocking |
✅ |
prod-server.ts Pages Router |
Reads from vinextConfig.images, applies SVG blocking |
✅ |
deploy.ts Pages Router worker |
Extracts imageConfig from vinextConfig.images |
✅ |
index.ts vinext:image-config |
Writes image-config.json for App Router builds |
✅ |
Dev servers (index.ts, app-dev-server.ts) |
Redirect-only, no SVG blocking | ✅ (acceptable — dev doesn't optimize images) |
Minor observations (non-blocking)
-
Dev/prod behavior difference: In dev, SVGs always work because the image endpoint just redirects to the original asset URL. In prod, SVGs are blocked unless
dangerouslyAllowSVG: true. This is actually reasonable since dev mode doesn't transform images at all, but it's a subtle difference developers might notice when deploying. -
App Router worker entry in
deploy.ts: The Pages Router path explicitly extractsimageConfigfromvinextConfig.images, but the App Router path relies onimage-config.jsonbeing read at runtime inprod-server.ts. This works but the two paths use different config propagation mechanisms — worth keeping in mind for future maintenance.
LGTM — nice work matching the Next.js behavior faithfully.
|
Review posted on PR #236. Here's a summary of what I found: Verdict: Approved The PR correctly implements SVG handling to match Next.js behavior across all critical paths:
Two minor non-blocking observations noted: dev/prod behavior difference (acceptable since dev doesn't optimize images) and the two different config propagation mechanisms between App Router and Pages Router worker entries. |
|
Thanks @southpolesteve. Co-authored-by: Dhruv Sharma dhruvsharma@users.noreply.github.com This isnt me, is it possible to use dhruvcoding67@gmail.com? 🥹 |
SVGs are auto-skipped on the client side (set unoptimized=true) unless dangerouslyAllowSVG is enabled in next.config. When dangerouslyAllowSVG is true, SVGs that reach the server are passed through without transformation, with security headers (CSP, Content-Disposition) applied. Config options added to NextConfig.images: - dangerouslyAllowSVG (default: false) - contentDispositionType (default: "inline") - contentSecurityPolicy (default: "script-src 'none'; frame-src 'none'; sandbox;") Co-authored-by: Dhruv Sharma <dhruvcoding67@gmail.com>
d1f9bf9 to
fcbf801
Compare
Summary
Fixes #205. SVGs passed to
/_vinext/imagenow work correctly, matching Next.js behavior.Based on the approach from #215 by @illegalcall (Dhruv Sharma). Thank you for the contribution! This PR takes the same direction with some fixes to match Next.js behavior more precisely.
What changed
Client-side: auto-skip SVGs (default behavior)
When
srcends with.svganddangerouslyAllowSVGis not enabled in config, theImagecomponent automatically setsunoptimized = true. This means SVGs bypass/_vinext/imageentirely and load directly from their source URL. This matches what Next.js does.Server-side: SVG passthrough with security headers
When
dangerouslyAllowSVG: trueis set innext.config, SVGs that reach the image optimizer are passed through without transformation (no sharp processing). Security headers are applied:Content-Security-Policy: script-src 'none'; frame-src 'none'; sandbox;Content-Dispositionset per config (default:inline)X-Content-Type-Options: nosniffNew config options in
next.config.imagesdangerouslyAllowSVG(default:false) - allow SVGs through the optimizercontentDispositionType(default:"inline") - controls Content-Disposition headercontentSecurityPolicy(default:"script-src 'none'; frame-src 'none'; sandbox;")How Next.js does it
From Next.js source (
shared/lib/get-img-props.ts):The auto-skip is conditional on
!dangerouslyAllowSVG, not unconditional. This is the key difference from #215, which always skipped SVGs regardless of config.Files changed
config/next-config.ts- Added SVG-related config typesshims/image.tsx- Client-side conditional SVG auto-skipserver/image-optimization.ts- SVG passthrough logic, configurable security headers,ImageConfigtypeindex.ts- InjectdangerouslyAllowSVGconfig into client and build outputsserver/prod-server.ts- Thread image config through to optimization handlerdeploy.ts- Pages Router worker entry passes image configtests/shims.test.ts- Unit tests for all SVG handling pathsTests
Added tests covering:
dangerouslyAllowSVGis false (default)dangerouslyAllowSVGis trueisSafeImageContentTypeaccepts/rejects SVG based on confighandleImageOptimizationdangerouslyAllowSVGis false