fix(build): bundle image-size in vinext - #2913
Conversation
commit: |
|
Performance benchmarksCompared 0 improved · 0 regressed · 6 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(build): bundle image-size in vinext
Verified locally: built the package, confirmed bundling, ran the cited tests, and checked format. This LGTM.
What I verified
- Both usage sites are build-time only, matching the PR claim:
src/server/metadata-route-build-data.ts:3(static import)src/index.ts:6173(dynamic import in the pluginloadhook)
image-size@2.0.2has zero dependencies, so bundling produces a self-contained 24KB chunk with no external imports — a clean candidate foralwaysBundle(same rationale asam-i-vibing/pathslash).- Build output is correct:
- Static import →
import imageSize from "../deps/.pnpm/image-size@2.0.2/deps/image-size/dist/index.js"indist/server/metadata-route-build-data.js. - Dynamic import → rewritten to
await import("./deps/.pnpm/image-size@2.0.2/deps/image-size/dist/index.js")indist/index.js(relative, not a machine-specific absolute path — this is exactly the dynamic-import trap theexternalizeBareThirdPartySpecifierscomment guards against, and it's handled correctly here via the newid === "image-size"early-return). - Emitted under
dist/deps/...(notdist/node_modules/...), sobuild/standalone.ts'snode_modulespruning won't drop it.
- Static import →
- Published
package.jsonno longer listsimage-sizeunderdependencies(nowdevDependencies);files: ["dist"]ships the bundleddist/depsoutput. - Tests pass:
tests/image-imports.test.ts(27) +tests/metadata-route-build-data.test.ts(10) — all green. - Format check passes.
- Lockfile diff only moves the
image-sizeimporter entry from prod to dev deps forpackages/vinext; the otherimage-sizereference in the lockfile isfumadocs-core's own transitive dep and is unrelated.
Minor observations (non-blocking)
neverBundleusesid.includes("image-size"), a substring match. I confirmed only oneimage-size@*package exists in the tree, so there's no collision today. This is consistent with the existingam-i-vibing/pathslashsubstring pattern, so it's fine to keep for consistency — just noting the same fragility applies to all four entries.- Consider a regression test asserting the built output contains no bare
image-sizespecifier (the PR description mentions doing this manually). Not required, but would lock in the guarantee that this dep stays out of consumer install graphs.
Nice, well-scoped change with thorough manual validation. No issues found.
|
Review posted to PR #2913. SummaryI reviewed Verified:
Non-blocking notes raised in the review:
Clean, well-scoped change with thorough manual validation. |
Summary
image-sizepackage into vinext's published tooling outputimage-sizefrom runtime dependencies to dev dependenciesimage-sizein consumer install graphsValidation
vp checkvp test run tests/image-imports.test.ts tests/metadata-route-build-data.test.tsvp run vinext#buildvinextand confirmed its publishedpackage.jsonhas noimage-sizedependency while the bundled implementation is present underdist/depsimage-sizepackage, verified dimension extraction, and completed a production Pages Router build with a static image importimage-sizereferences or parser code