Add standalone self-host output and align init production scripts#178
Add standalone self-host output and align init production scripts#178sankalpmukim wants to merge 13 commits intocloudflare:mainfrom
Conversation
|
/bigbonk review this PR |
commit: |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Standalone self-host output
The overall approach is solid — extracting computeLazyChunks to break the circular import, adding standalone output as a post-build step, and the transitive dependency copying are all well-structured. A few issues to address before merge:
Must-fix
server.jsmixes CJS and ESM in an incompatible way — usesrequire()and__dirname(CJS) alongside top-levelawait import()(ESM-ish). This will break if the standalone directory has apackage.jsonwith"type": "module"or if Node runs it as ESM. Needs to be consistently CJS or ESM.HOSTNAMEenv var collides with the system-set hostname — on Linux,HOSTNAMEis set by the OS to the machine's hostname (e.g.ip-172-31-1-42), which would override the default0.0.0.0binding. Next.js standalone usesHOSTNAMEtoo, but that's a known footgun. Consider usingHOSTor documenting theHOSTNAMEpitfall.- Regex objects are shared across files but only reset at the inner loop — the three regexes (
importExportRE, etc.) are created once and reused across all files. ThelastIndex = 0reset on line 89 is correct, but only because the regexes are declared inside the function (not at module scope). This is fine but fragile — a future refactor moving them to module scope would silently break. A comment would help.
Should-fix
- No
package.jsonwritten intostandalone/— without apackage.jsonwith"type": "module", the embeddedvinextpackage (which has"type": "module") may have module resolution issues since Node walks up to find the nearestpackage.json. optionalDependenciesfailures are silently swallowed —copyPackageAndRuntimeDepscatches resolve failures andcontinues. This is correct for optional deps but also silently drops required deps that fail to resolve (e.g. if they useexportsmaps that don't exposepackage.json).- Missing test for symlinked
node_modules—dereference: trueis used incpSync, which is correct, but there's no test covering symlinked packages (common with pnpm).
|
Review posted with "Request Changes". Here's a summary of the key findings: Must-fix issues:
Should-fix issues:
Additional suggestions:
|
|
Working on the suggestions |
Generate dist/standalone from output: 'standalone' with a runnable server entry and runtime deps, and align init scripts with vinext build/start for production self-host workflows.
sankalpmukim
force-pushed
the
feat/selfhost-standalone-build-start
branch
from
e64eebc to
17a416e
Compare
|
Implemented the requested updates and rebased on latest upstream main (commit Addressed must-fix items:
Addressed key should-fix items:
Validation run:
|
|
@southpolesteve gentle nudge... |
|
/bigbonk review |
There was a problem hiding this comment.
Review: Standalone self-host output (round 2)
Good progress addressing the first round of feedback. The CJS package.json fix, HOST env var switch, exports-map fallback resolution, and symlink test coverage are all solid. A few issues remain — some are bugs, some are cleanup.
Must-fix
- Duplicate
PHASE_PRODUCTION_BUILDconstant —PHASE_PRODUCTION_BUILDis already exported fromshims/constants.ts:75. The PR re-declares it innext-config.ts:278instead of importing from the canonical source. This creates two independent constants with the same value, which is a maintenance hazard — if the string ever changes in one place, the other silently drifts. computeLazyChunksextraction is incomplete — The newutils/lazy-chunks.tsis a copy of the function fromindex.ts, but neitherindex.tsnorprod-server.tsare updated to import from the new location. The function is now duplicated in two files. The PR description says "extractcomputeLazyChunksinto a shared utility so the production server no longer imports the full plugin module" butprod-server.ts:48still hasimport { computeLazyChunks } from "../index.js". Either complete the extraction or remove the new file.- Unused import in
index.ts— Line 54 addsimport { staticExportPages } from "./build/static-export.js"but this symbol is never used withinindex.ts(it's already re-exported on line 3844 viaexport { ... } from). This is a dead import that will fail lint. - Unused import in
cli.ts— Line 31 addsimport { detectPackageManager as _detectPackageManager } from "./utils/project.js"but_detectPackageManageris never used.detectPackageManageris already imported on line 24.
Should-fix
- Config loaded before build, standalone emitted after — The next config is now loaded at the top of
buildApp()(line 372), before the Vite build runs. This is fine and actually better (fails fast on bad config). But the standalone emission on line 511 happens after the build completes, meaning the entire Vite build runs even if the config hasoutput: "standalone"and the standalone step will fail (e.g. missing vinext dist). Not a blocker, but worth noting. - Early
returnon line 520 skipsprocess.exit(0)— The non-standalone path ends withprocess.exit(0)on line 543. The standalone path returns without callingprocess.exit(). In Node, this means the process will hang if any background timers or open handles exist (common with Vite's module runner). Consider addingprocess.exit(0)after the standalone output too.
The init script changes (vite dev → vinext dev, vite build → vinext build, + start:vinext) and the test updates all look correct.
| * Emit a warning when config loading fails, with a targeted hint for | ||
| * known plugin wrappers that are unnecessary in vinext. | ||
| */ | ||
| export const PHASE_PRODUCTION_BUILD = "phase-production-build"; |
There was a problem hiding this comment.
PHASE_PRODUCTION_BUILD is already exported from shims/constants.ts:75 (which is where PHASE_DEVELOPMENT_SERVER is imported from on line 11 of this file). Import it from there instead of re-declaring:
| export const PHASE_PRODUCTION_BUILD = "phase-production-build"; |
And update line 11 to:
import { PHASE_DEVELOPMENT_SERVER, PHASE_PRODUCTION_BUILD } from "../shims/constants.js";Then re-export it from this file if the CLI needs it:
export { PHASE_PRODUCTION_BUILD } from "../shims/constants.js";
packages/vinext/src/index.ts
Outdated
| type RequestContext, | ||
| } from "./config/config-matchers.js"; | ||
| import { scanMetadataFiles } from "./server/metadata-routes.js"; | ||
| import { staticExportPages } from "./build/static-export.js"; |
There was a problem hiding this comment.
This import is unused — staticExportPages is not referenced anywhere in index.ts other than the existing re-export on line 3844 (export { staticExportPages, staticExportApp } from "./build/static-export.js"). The export { ... } from form doesn't require a corresponding import. This will likely fail lint. Remove this line.
packages/vinext/src/cli.ts
Outdated
| import { loadNextConfig, resolveNextConfig } from "./config/next-config.js"; | ||
| import { loadNextConfig, resolveNextConfig, PHASE_PRODUCTION_BUILD } from "./config/next-config.js"; | ||
| import { emitStandaloneOutput } from "./build/standalone.js"; | ||
| import { detectPackageManager as _detectPackageManager } from "./utils/project.js"; |
There was a problem hiding this comment.
Dead import — _detectPackageManager is never used. detectPackageManager is already imported on line 24. Remove this line.
| import { detectPackageManager as _detectPackageManager } from "./utils/project.js"; |
| * @returns Array of chunk filenames (e.g. "assets/mermaid-NOHMQCX5.js") that | ||
| * should be excluded from modulepreload hints. | ||
| */ | ||
| export function computeLazyChunks(buildManifest: Record<string, BuildManifestChunk>): string[] { |
There was a problem hiding this comment.
This file duplicates computeLazyChunks from index.ts:652 but neither index.ts nor prod-server.ts are updated to import from here. The PR description says this extraction breaks the circular import so prod-server.ts doesn't need to import the full plugin, but prod-server.ts:48 still has:
import { computeLazyChunks } from "../index.js";To complete the extraction:
- Update
prod-server.tsto import from"../utils/lazy-chunks.js" - Update
index.tsto import and re-export from"./utils/lazy-chunks.js"(to preserve the public export on line 3860) - Remove the duplicate implementation from
index.ts:652-712
packages/vinext/src/cli.ts
Outdated
| ` Generated standalone output in ${path.relative(process.cwd(), standalone.standaloneDir)}/`, | ||
| ); | ||
| console.log(" Start it with: node dist/standalone/server.js\n"); | ||
| return; |
There was a problem hiding this comment.
The non-standalone path ends with process.exit(0) on line 543, but this return skips it. If any Vite module runner handles or background timers are still alive, the process will hang instead of exiting cleanly. Consider adding process.exit(0) here:
| return; | |
| return process.exit(0); |
Or add it after the return at the end of the standalone block.
| } | ||
|
|
||
| function writeStandaloneServerEntry(filePath: string): void { | ||
| const content = `#!/usr/bin/env node |
There was a problem hiding this comment.
Minor: The shebang (#!/usr/bin/env node) is nice for ./server.js execution, but the path.join(__dirname, "dist") means this script must be run from the standalone directory or via node dist/standalone/server.js from the project root — __dirname resolves to the script's directory either way, so it works. Just confirming this is intentional and correct.
| const packageRoot = path.dirname(packageJsonPath); | ||
| const packageTarget = path.join(targetNodeModulesDir, entry.packageName); | ||
| fs.mkdirSync(path.dirname(packageTarget), { recursive: true }); | ||
| fs.cpSync(packageRoot, packageTarget, { recursive: true, dereference: true }); |
There was a problem hiding this comment.
Potential performance issue for monorepos: dereference: true with cpSync will follow symlinks and copy full contents. In a hoisted pnpm monorepo, a single large package (e.g. @swc/core with platform-specific binaries) could be ~100MB+. Combined with the transitive dep walk, standalone output could balloon.
Not a blocker for the first version, but worth tracking. A future optimization could check if a package is already a real directory (not a symlink) and skip dereferencing in that case, or add size limits/warnings.
|
Review submitted. Here's a summary of the findings: Must-fix (4 issues)
Should-fix (2 issues)
|
…ks extraction, remove dead imports, add process.exit to standalone path
|
/bigbonk review |
There was a problem hiding this comment.
Review: Standalone self-host output (round 3)
The previous review rounds have been addressed well — the computeLazyChunks extraction is now complete (prod-server.ts and index.ts both import from utils/lazy-chunks.js), the PHASE_PRODUCTION_BUILD duplicate is resolved (imported from shims/constants.ts and re-exported), the CJS/ESM mismatch is fixed with the {"type": "commonjs"} package.json, and the HOST env var swap is correct.
Overall this is solid work. A few remaining items from minor to substantive:
Issues
-
importSideEffectREwill double-matchfromimports —\bimport\s*["']will match theimportkeyword inimport "foo"(side-effect), but\bfrom\s*["']will NOT match those. However, for a normal import likeimport { x } from "foo", thefromREcatches it ANDimportSideEffectREcould also match if Rolldown/Vite emits something likeimport"foo"(minified). Since results go into aSet, this is harmless but worth a brief note. -
export { ... } fromplacement between imports — Havingexport { PHASE_DEVELOPMENT_SERVER, PHASE_PRODUCTION_BUILD }sandwiched betweenimportstatements (line 12) is valid but unusual. The canonical pattern isexport { ... } from "..."as a re-export, or grouping exports after all imports. -
Missing
assetsfield inBuildManifestChunk— The original type inindex.tsincluded anassets?: string[]field. The newutils/lazy-chunks.tsversion drops it. WhilecomputeLazyChunksdoesn't useassets, consumers passing existing manifest chunks may have that field, and it's good practice to keep the type aligned with the Vite manifest format. -
Vinext deps in standalone are incomplete — Only
package.jsonanddist/are copied from the vinext package. If vinext has its own runtimedependencies(check the package.json), those won't be in the standalonenode_modules. Theserver.jsdoesimport("vinext/server/prod-server")which will then try to import vinext's own deps. Currently the app's own deps likely overlap enough that this works, but it's a latent gap.
None of these are blockers for merge.
| const files = walkFiles(serverDir).filter((filePath) => /\.(c|m)?js$/.test(filePath)); | ||
|
|
||
| const fromRE = /\bfrom\s*["']([^"']+)["']/g; | ||
| const importSideEffectRE = /\bimport\s*["']([^"']+)["']/g; |
There was a problem hiding this comment.
Nit: importSideEffectRE (\bimport\s*["']) can technically also match the import keyword in a normal import statement if the output is minified without spaces (e.g. import"foo"). Since all results go into a Set<string>, the overlap is harmless. But the intended division of labor between these regexes could be documented:
// fromRE: import { x } from "pkg" / export { x } from "pkg"
// importSideEffectRE: import "pkg" (side-effect only)
// dynamicImportRE: import("pkg")
// requireRE: require("pkg")
// Overlap between regexes is fine — results are collected in a Set.
| import { randomUUID } from "node:crypto"; | ||
| import { PHASE_DEVELOPMENT_SERVER } from "../shims/constants.js"; | ||
| import { PHASE_DEVELOPMENT_SERVER, PHASE_PRODUCTION_BUILD } from "../shims/constants.js"; | ||
| export { PHASE_DEVELOPMENT_SERVER, PHASE_PRODUCTION_BUILD }; |
There was a problem hiding this comment.
Minor style nit: this bare export { ... } sandwiched between import statements reads oddly. Since you're importing on line 11 and want to re-export, the more conventional pattern is a single export { ... } from re-export. That said, the current code already imports these values for local use (line 272's DEFAULT_PHASE), so you do need both the import and the re-export. Consider grouping the re-export at the bottom of the file with the other exports, or converting to:
import { PHASE_DEVELOPMENT_SERVER, PHASE_PRODUCTION_BUILD } from "../shims/constants.js";
// (re-exported at bottom of file)Not blocking.
| isDynamicEntry?: boolean; | ||
| imports?: string[]; | ||
| dynamicImports?: string[]; | ||
| css?: string[]; |
There was a problem hiding this comment.
The original BuildManifestChunk type in index.ts had an assets?: string[] field that this extraction drops. While computeLazyChunks doesn't use it, keeping the type aligned with the full Vite manifest chunk shape is better for future consumers:
| css?: string[]; | |
| css?: string[]; | |
| assets?: string[]; |
| fs.cpSync(vinextDistDir, path.join(vinextTargetDir, "dist"), { | ||
| recursive: true, | ||
| dereference: true, | ||
| }); |
There was a problem hiding this comment.
The vinext embedding copies package.json and dist/, but if vinext's own package.json lists runtime dependencies (e.g. a future addition), those won't be present in the standalone node_modules. Currently this works because prod-server.ts's dependencies overlap with the app's own deps that are already copied. But it's a latent gap — if vinext ever adds a runtime dep that users don't also depend on, standalone will break at startup.
Consider adding a follow-up to read vinext's package.json dependencies and feed them into copyPackageAndRuntimeDeps as well, or at minimum leave a TODO comment here.
| }); | ||
| } | ||
|
|
||
| function walkFiles(dir: string): string[] { |
There was a problem hiding this comment.
Since Node >= 22 is required, you could replace this recursive function with:
const files = fs.readdirSync(dir, { withFileTypes: true, recursive: true })
.filter(e => !e.isDirectory())
.map(e => path.join(e.parentPath, e.name));This is available since Node 18.17 and removes the manual recursion. Not blocking — the current implementation is clear.
| const rootPkg = readPackageJson(path.join(root, "package.json")); | ||
| const rootOptional = new Set(Object.keys(rootPkg.optionalDependencies ?? {})); | ||
| const copied = new Set<string>(); | ||
| const queue: QueueEntry[] = initialPackages.map((packageName) => ({ |
There was a problem hiding this comment.
The optional flag for initial packages is determined by checking the root optionalDependencies. But packages discovered via collectServerExternalPackages (server bundle scan) are always added with optional: rootOptional.has(packageName). This means a server-scanned package that's NOT in the root package.json at all (not in deps, devDeps, or optionalDeps) will have optional = false and throw if it can't be resolved.
This is actually the correct behavior — if the server bundle imports it, it's required. Just wanted to confirm this was intentional, since server-scanned packages could theoretically include specifiers that are bundled inline (not externalized) and thus don't need to be in node_modules. In practice, Vite externalizes all bare imports in SSR builds by default, so this should be fine.
|
Review posted successfully. Here's a summary of what I found: Previous review issues now resolved:
New items I flagged (all non-blocking):
|
…bottom, add assets field to BuildManifestChunk
|
/bigbonk review |
There was a problem hiding this comment.
Review: Standalone self-host output (round 4)
The previous review rounds' feedback has been addressed well. The computeLazyChunks extraction is complete, the PHASE_PRODUCTION_BUILD import is canonical, CJS/ESM is handled with {"type": "commonjs"}, and HOST replaces HOSTNAME. The code is in solid shape.
I found one real bug and a few non-blocking items.
Bug
- Vinext's own runtime dependencies are not copied into standalone output — The standalone builder copies vinext's
package.jsonanddist/directory, but vinext has 6 runtimedependencies(rsc-html-stream,magic-string,vite-tsconfig-paths, etc.). Whenserver.jsrunsimport("vinext/server/prod-server"), the prod server imports from these packages (e.g.rsc-html-streamis used for RSC streaming). If the user's app doesn't also depend on these packages, the standalone server will crash at startup with module-not-found errors. This is a real gap —rsc-html-streamin particular is unlikely to be in a typical user'sdependencies.
Non-blocking
-
The
resolveNextConfigcall on line 371 omits therootargument (previously passed explicitly asprocess.cwd()). This works becauseresolveNextConfigdefaults toprocess.cwd(), but the implicit reliance is worth noting — if the function signature ever changes its default, this would silently break. -
The config is loaded early (line 371, before the Vite build), which is good for fail-fast. But if config loading succeeds and the Vite build then takes minutes, a standalone emission failure (e.g. missing vinext dist) wastes that build time. Consider validating the vinext dist existence before starting the Vite build when
outputMode === "standalone". -
The test coverage is good — exports-map fallback, symlink handling, missing dep errors, and the devDependency-via-server-scan case are all covered. One gap: no test for the vinext runtime dep transitivity issue noted above.
| fs.cpSync(vinextDistDir, path.join(vinextTargetDir, "dist"), { | ||
| recursive: true, | ||
| dereference: true, | ||
| }); |
There was a problem hiding this comment.
Bug: vinext has runtime dependencies (rsc-html-stream, magic-string, vite-tsconfig-paths, @unpic/react, @vercel/og, vite-plugin-commonjs) that are not copied into the standalone node_modules. Only vinext's package.json and dist/ are embedded.
When server.js starts and imports vinext/server/prod-server, the prod server will try to import these packages. If the user's app doesn't also list them as dependencies (and there's no reason they would — rsc-html-stream is an internal vinext dependency), the standalone server crashes with ERR_MODULE_NOT_FOUND.
Fix: after copying vinext's dist, read vinext's package.json dependencies and feed them into copyPackageAndRuntimeDeps:
| }); | |
| fs.cpSync(vinextDistDir, path.join(vinextTargetDir, "dist"), { | |
| recursive: true, | |
| dereference: true, | |
| }); | |
| // Also copy vinext's own runtime dependencies so prod-server imports resolve. | |
| const vinextPkg = readPackageJson(path.join(vinextPackageRoot, "package.json")); | |
| const vinextRuntimeDeps = runtimeDeps(vinextPkg).filter( | |
| (name) => !copied.has(name) && name !== "vinext", | |
| ); | |
| if (vinextRuntimeDeps.length > 0) { | |
| const vinextResolver = createRequire(path.join(vinextPackageRoot, "package.json")); | |
| const vinextQueue: QueueEntry[] = vinextRuntimeDeps.map((packageName) => ({ | |
| packageName, | |
| resolver: vinextResolver, | |
| optional: false, | |
| })); | |
| // Reuse the BFS copy logic for vinext's transitive deps | |
| const extraQueue = [...vinextQueue]; | |
| while (extraQueue.length > 0) { | |
| const entry = extraQueue.shift(); | |
| if (!entry || copied.has(entry.packageName)) continue; | |
| const packageJsonPath = resolvePackageJsonPath(entry.packageName, entry.resolver); | |
| if (!packageJsonPath) continue; | |
| const packageRoot = path.dirname(packageJsonPath); | |
| const packageTarget = path.join(standaloneNodeModulesDir, entry.packageName); | |
| fs.mkdirSync(path.dirname(packageTarget), { recursive: true }); | |
| fs.cpSync(packageRoot, packageTarget, { recursive: true, dereference: true }); | |
| copied.add(entry.packageName); | |
| const depPkg = readPackageJson(packageJsonPath); | |
| const depResolver = createRequire(packageJsonPath); | |
| for (const depName of runtimeDeps(depPkg)) { | |
| if (!copied.has(depName)) { | |
| extraQueue.push({ packageName: depName, resolver: depResolver, optional: false }); | |
| } | |
| } | |
| } | |
| } |
Alternatively, refactor copyPackageAndRuntimeDeps to accept additional seed entries so the BFS logic isn't duplicated. The key point is that vinext's deps must be resolved from vinext's own package.json location (using a resolver rooted there), not from the app root — they may not be hoisted into the app's node_modules.
| * - <outDir>/standalone/dist/{client,server} | ||
| * - <outDir>/standalone/node_modules (runtime deps only) | ||
| */ | ||
| export function emitStandaloneOutput(options: StandaloneBuildOptions): StandaloneBuildResult { |
There was a problem hiding this comment.
Nit: emitStandaloneOutput is a synchronous function that does a lot of I/O. For large apps with many dependencies, this could block the event loop for a while. Not a problem today since it runs at the end of a CLI build command, but worth noting if this is ever called from a context where responsiveness matters.
| console.log(`\n vinext build (Vite ${getViteVersion()})\n`); | ||
|
|
||
| const isApp = hasAppDir(); | ||
| const resolvedNextConfig = await resolveNextConfig( |
There was a problem hiding this comment.
Minor: resolveNextConfig is called without the root argument here, relying on the default process.cwd(). The old code (before this PR) passed process.cwd() explicitly at the call site. Being explicit is slightly better for clarity, especially since other callers in the codebase (e.g. deploy.ts:1338, run-prerender.ts:131) do pass root explicitly.
| const resolvedNextConfig = await resolveNextConfig( | |
| const resolvedNextConfig = await resolveNextConfig( | |
| await loadNextConfig(process.cwd(), PHASE_PRODUCTION_BUILD), | |
| process.cwd(), | |
| ); |
| const packageRoot = path.dirname(packageJsonPath); | ||
| const packageTarget = path.join(targetNodeModulesDir, entry.packageName); | ||
| fs.mkdirSync(path.dirname(packageTarget), { recursive: true }); | ||
| fs.cpSync(packageRoot, packageTarget, { recursive: true, dereference: true }); |
There was a problem hiding this comment.
Consider adding filter to skip node_modules directories inside packages (some packages ship their own nested node_modules). Without a filter, you'll recursively copy all nested node_modules too, which can balloon the standalone size. cpSync supports a filter callback:
fs.cpSync(packageRoot, packageTarget, {
recursive: true,
dereference: true,
filter: (src) => !src.includes('/node_modules/'),
});This is a should-fix — nested node_modules in copied packages means duplicated (and potentially version-mismatched) transitive deps that should instead be resolved through the BFS walk.
|
Review posted successfully. Here's a summary of what I found: Bug (must-fix)Vinext's own runtime dependencies are not copied into standalone output. The standalone builder embeds vinext's Non-blocking items
|
…ead of regex scan Replace the fragile post-build regex scan of emitted JS files and the package.json#dependencies over-seeding with a new vinext:server-externals-manifest Vite plugin. The plugin collects chunk.imports + chunk.dynamicImports from the SSR/RSC writeBundle hook and writes dist/server/vinext-externals.json — the authoritative, compiler-derived list of packages left external in the server bundle. emitStandaloneOutput now reads this manifest as its sole seed for the BFS node_modules copy, so standalone output only includes packages the server actually imports at runtime. Removed collectServerExternalPackages, walkFiles, the four regex constants, and the package.json deps seeding.
|
/bigbonk review |
There was a problem hiding this comment.
Review: Standalone self-host output (round 5)
Good work iterating on prior feedback. The architecture is much improved since the initial version — using the bundler's own externals manifest (vinext:server-externals-manifest) instead of regex scanning is the right call, the computeLazyChunks extraction is complete, the CJS/ESM {"type": "commonjs"} fix is in place, and the HOST env var switch is correct.
One must-fix remains from previous rounds that hasn't been addressed. The rest are non-blocking observations.
Must-fix
- Vinext's own runtime dependencies are not copied into standalone output — This was flagged in rounds 3 and 4 and is still present. vinext has 6 runtime
dependencies(rsc-html-stream,magic-string,vite-tsconfig-paths,@unpic/react,@vercel/og,vite-plugin-commonjs). The standalone builder copies vinext'spackage.jsonanddist/, but not these deps. Whenserver.jsstarts andprod-server.tstries to importrsc-html-stream(used for RSC streaming), it will crash withERR_MODULE_NOT_FOUNDunless the user's app happens to also depend on it.rsc-html-streamin particular is not something a typical Next.js app would list in its owndependencies.
Non-blocking
-
pendingWritesis tracked but never read — Inserver-externals-manifest.ts,pendingWritesis incremented on line 104 but never used in any condition. The comment on line 61-62 says it's for knowing when to flush, but the actual flush logic on line 117-121 runs unconditionally every call. Either remove the unused variable or use it. -
cpSyncwithoutnode_modulesfilter — Flagged in round 4, still applicable.cpSyncwithdereference: trueand no filter will recursively copy nestednode_modulesinside packages, potentially duplicating transitive deps at different versions than what the BFS walk resolved. Afilter: (src) => !src.includes('/node_modules/')callback would prevent this. Not blocking for v1, but could cause subtle version mismatches in standalone output. -
resolveNextConfigcalled without explicitroot— The call on line 371-373 ofcli.tsomits therootparameter toresolveNextConfig, relying on itsprocess.cwd()default. Other callers in the codebase pass it explicitly. Being explicit is better for clarity and resilience to signature changes. -
Fail-fast for vinext dist validation — The vinext dist existence check happens after the full Vite build completes (line 264 of
standalone.ts). Ifoutput: "standalone"is set, validating that the vinext dist exists before starting the expensive build would avoid wasting build time on an inevitable failure. This could be a quick pre-flight check inbuildApp()beforebuilder.buildApp().
| fs.cpSync(vinextDistDir, path.join(vinextTargetDir, "dist"), { | ||
| recursive: true, | ||
| dereference: true, | ||
| }); |
There was a problem hiding this comment.
Must-fix: vinext's own runtime dependencies are missing from standalone output.
vinext's package.json lists 6 runtime dependencies: rsc-html-stream, magic-string, vite-tsconfig-paths, @unpic/react, @vercel/og, vite-plugin-commonjs. Only vinext's package.json and dist/ are copied here — its deps are not.
When server.js starts and imports vinext/server/prod-server, that module will try to import rsc-html-stream (used for RSC stream handling). If the user's app doesn't also depend on it (and there's no reason it would — it's an internal vinext dependency), the server crashes at startup.
Fix: after copying vinext's dist, read vinext's package.json dependencies and feed them through the same copyPackageAndRuntimeDeps BFS logic. The resolver must be rooted at vinext's package location (not the app root), since these deps may not be hoisted into the app's node_modules:
| }); | |
| fs.cpSync(vinextDistDir, path.join(vinextTargetDir, "dist"), { | |
| recursive: true, | |
| dereference: true, | |
| }); | |
| // Copy vinext's own runtime dependencies so prod-server imports resolve. | |
| const vinextPkg = readPackageJson(path.join(vinextPackageRoot, "package.json")); | |
| const vinextRuntimeDeps = runtimeDeps(vinextPkg).filter( | |
| (name) => !copiedPackages.includes(name) && name !== "vinext", | |
| ); | |
| if (vinextRuntimeDeps.length > 0) { | |
| const vinextResolver = createRequire(path.join(vinextPackageRoot, "package.json")); | |
| const vinextNodeModulesDir = standaloneNodeModulesDir; | |
| const vinextCopied = new Set(copiedPackages); | |
| const vinextQueue: QueueEntry[] = vinextRuntimeDeps.map((name) => ({ | |
| packageName: name, | |
| resolver: vinextResolver, | |
| optional: false, | |
| })); | |
| while (vinextQueue.length > 0) { | |
| const entry = vinextQueue.shift(); | |
| if (!entry || vinextCopied.has(entry.packageName)) continue; | |
| const pkgJsonPath = resolvePackageJsonPath(entry.packageName, entry.resolver); | |
| if (!pkgJsonPath) continue; | |
| const pkgRoot = path.dirname(pkgJsonPath); | |
| const pkgTarget = path.join(vinextNodeModulesDir, entry.packageName); | |
| fs.mkdirSync(path.dirname(pkgTarget), { recursive: true }); | |
| fs.cpSync(pkgRoot, pkgTarget, { recursive: true, dereference: true }); | |
| vinextCopied.add(entry.packageName); | |
| copiedPackages.push(entry.packageName); | |
| const depPkg = readPackageJson(pkgJsonPath); | |
| const depResolver = createRequire(pkgJsonPath); | |
| for (const depName of runtimeDeps(depPkg)) { | |
| if (!vinextCopied.has(depName)) { | |
| vinextQueue.push({ packageName: depName, resolver: depResolver, optional: false }); | |
| } | |
| } | |
| } | |
| } |
Alternatively, refactor copyPackageAndRuntimeDeps to accept an optional resolver override and additional seed entries, so the BFS logic isn't duplicated.
| if (!outDir) outDir = dir; | ||
| } | ||
|
|
||
| pendingWrites++; |
There was a problem hiding this comment.
Nit: pendingWrites is incremented here but never read. The comment on line 61-62 says it exists to track when it's safe to flush, but the flush on lines 117-121 runs unconditionally on every call. Either remove the unused variable or add a condition that uses it.
| const packageRoot = path.dirname(packageJsonPath); | ||
| const packageTarget = path.join(targetNodeModulesDir, entry.packageName); | ||
| fs.mkdirSync(path.dirname(packageTarget), { recursive: true }); | ||
| fs.cpSync(packageRoot, packageTarget, { recursive: true, dereference: true }); |
There was a problem hiding this comment.
Non-blocking: consider adding a filter callback to skip nested node_modules directories inside copied packages. Without it, packages that ship their own node_modules (or have nested deps installed) will be fully duplicated, potentially at different versions than what the BFS walk would resolve from the project root.
| fs.cpSync(packageRoot, packageTarget, { recursive: true, dereference: true }); | |
| fs.cpSync(packageRoot, packageTarget, { recursive: true, dereference: true, filter: (src) => !src.includes(`${path.sep}node_modules${path.sep}`) }); |
|
|
||
| for (const item of Object.values(bundle)) { | ||
| if (item.type !== "chunk") continue; | ||
| const chunk = item as { imports: string[]; dynamicImports: string[] }; |
There was a problem hiding this comment.
Minor: the Rollup/Vite OutputChunk type has imports and dynamicImports as required string[] fields (never undefined), so the ?? [] fallback is unnecessary. Not wrong, just defensive code that may cause confusion about whether these can actually be undefined.
Also worth noting: chunk.imports in Rollup output bundles contains filenames of other chunks in the bundle, not npm specifiers. For externalized packages, these show up as bare specifiers in chunk.imports because they weren't bundled into a chunk file. The packageNameFromSpecifier helper handles this correctly (filtering out relative/absolute paths), but a brief comment explaining why bare specifiers appear in chunk.imports would help future readers.
|
Review submitted. Here's a summary of the findings: Must-fix (1 issue)Vinext's own runtime dependencies are not copied into standalone output. This was flagged in rounds 3 and 4 and remains unaddressed. vinext has 6 runtime dependencies ( Non-blocking (4 items)
What's been resolved from prior rounds
|
… cpSync, remove unused pendingWrites
|
/bigbonk review |
There was a problem hiding this comment.
Review: Standalone self-host output
This PR is in solid shape. The prior review rounds have been addressed well:
computeLazyChunksextraction is complete — bothprod-server.tsandindex.tsimport fromutils/lazy-chunks.jsPHASE_PRODUCTION_BUILDis properly imported fromshims/constants.ts(no duplicate)- CJS/ESM is handled with
{"type": "commonjs"}standalone package.json HOSTenv var correctly replacesHOSTNAMEprocess.exit(0)on the standalone path- vinext's own runtime deps are now copied into standalone
node_modules/ node_modulesfilter prevents nested dep duplication incpSync- The server-externals-manifest plugin replaces fragile regex scanning with the bundler's own import graph
The architecture is clean: bundler writes authoritative externals manifest → standalone builder reads it → BFS copies exactly what's needed. Test coverage is thorough (transitive deps, symlinks, exports-map fallback, missing deps, vinext runtime deps).
I have a few remaining items — one functional concern and a few minor nits. None are blockers for merge.
| const isApp = hasAppDir(); | ||
| const resolvedNextConfig = await resolveNextConfig( | ||
| await loadNextConfig(process.cwd(), PHASE_PRODUCTION_BUILD), | ||
| ); |
There was a problem hiding this comment.
Nit: resolveNextConfig is called without the root parameter, relying on its process.cwd() default. Every other caller in the codebase passes root explicitly (index.ts:1059, deploy.ts:1338, run-prerender.ts:131). Being explicit is better for consistency and resilience to future signature changes.
| ); | |
| const resolvedNextConfig = await resolveNextConfig( | |
| await loadNextConfig(process.cwd(), PHASE_PRODUCTION_BUILD), | |
| process.cwd(), | |
| ); |
| dereference: true, | ||
| // Skip any nested node_modules/ — the BFS walk resolves deps at their | ||
| // correct hoisted location, so nested copies would be stale duplicates. | ||
| filter: (src) => path.basename(src) !== "node_modules", |
There was a problem hiding this comment.
This filter uses path.basename(src) !== "node_modules" which will also filter out any file or directory literally named node_modules at any depth in the package tree. In practice this is fine — no package ships a file named node_modules — but for precision, checking against a path segment would be slightly more robust:
filter: (src) => !src.includes(`${path.sep}node_modules`),Not blocking — the current approach works correctly for all real-world cases.
| // the production build. This is the authoritative list of packages the server | ||
| // bundle actually imports at runtime — determined by the bundler's own graph, | ||
| // not regex scanning or package.json#dependencies. | ||
| const initialPackages = readServerExternalsManifest(serverDir).filter( |
There was a problem hiding this comment.
If the build is App Router, the externals manifest is written to dist/server/ by the plugin. But serverDir here is path.join(outDir, "server"). The manifest plugin walks up from options.dir to find a parent named "server" — for SSR it could be dist/server/ssr → walks up to dist/server. This should work, but consider adding a brief comment here noting that the manifest is always at dist/server/vinext-externals.json regardless of sub-directories (rsc vs ssr), which is why we read from serverDir directly.
| // Always embed the exact vinext runtime that produced this build. | ||
| const vinextPackageRoot = resolveVinextPackageRoot(options.vinextPackageRoot); | ||
| const vinextDistDir = path.join(vinextPackageRoot, "dist"); | ||
| if (!fs.existsSync(vinextDistDir)) { |
There was a problem hiding this comment.
This validation happens after the full Vite build completes. If output: "standalone" is set and the vinext dist doesn't exist, the entire build runs before this error fires — wasting potentially minutes. Consider adding a pre-flight check in buildApp() (in cli.ts) before builder.buildApp() when outputMode === "standalone":
if (outputMode === "standalone") {
const vinextDist = path.dirname(fileURLToPath(import.meta.url));
// Quick sanity check that vinext's dist/ exists before running the full build
}Not blocking for v1, but a nice UX improvement.
|
|
||
| for (const item of Object.values(bundle)) { | ||
| if (item.type !== "chunk") continue; | ||
| const chunk = item as { imports: string[]; dynamicImports: string[] }; |
There was a problem hiding this comment.
Minor: the Rollup/Vite OutputChunk type has imports and dynamicImports as required string[] fields (never undefined), so the ?? [] fallback is unnecessary. Not wrong — defensive code is fine — but the cast to { imports: string[]; dynamicImports: string[] } already implies they're present. Could simplify by removing the cast and using item directly with an OutputChunk type guard, or just drop the ?? [].
| for (const item of Object.values(bundle)) { | ||
| if (item.type !== "chunk") continue; | ||
| const chunk = item as { imports: string[]; dynamicImports: string[] }; | ||
| for (const specifier of [...(chunk.imports ?? []), ...(chunk.dynamicImports ?? [])]) { |
There was a problem hiding this comment.
Worth a brief comment explaining why bare specifiers appear in chunk.imports. In a normal Rollup build, chunk.imports contains filenames of other chunks in the bundle. But for externalized packages, the specifiers remain as bare npm identifiers (e.g. "react") because they weren't bundled into chunk files. This is non-obvious and would help future readers understand the design.
| for (const specifier of [...(chunk.imports ?? []), ...(chunk.dynamicImports ?? [])]) { | |
| for (const specifier of [...(chunk.imports ?? []), ...(chunk.dynamicImports ?? [])]) { | |
| // In Rollup output, chunk.imports normally contains filenames of other | |
| // chunks. But externalized packages remain as bare npm specifiers | |
| // (e.g. "react") since they weren't bundled. packageNameFromSpecifier | |
| // filters out chunk filenames (relative/absolute paths) and extracts | |
| // the package name from bare specifiers. |
| return null; | ||
| } | ||
|
|
||
| export { PHASE_DEVELOPMENT_SERVER, PHASE_PRODUCTION_BUILD }; |
There was a problem hiding this comment.
Minor style nit: this re-export at the bottom of the file is fine functionally, but since both PHASE_DEVELOPMENT_SERVER and PHASE_PRODUCTION_BUILD are already imported on line 11, a export { ... } from "../shims/constants.js" re-export would be more idiomatic and wouldn't require the trailing bare export block. That said, the current approach works and the intent is clear.
|
Review posted successfully. Here's a summary of what I found: Overall assessment: The PR is in good shape. All must-fix items from prior review rounds have been addressed. The architecture is clean — the server-externals-manifest plugin uses the bundler's own import graph instead of regex scanning, the BFS dependency copy handles transitive deps correctly, and vinext's own runtime dependencies are now included. Items flagged (all non-blocking):
|
…comment, pre-flight check, bare-specifier comment, export-from re-export
|
/bigbonk review |
There was a problem hiding this comment.
Review: Standalone self-host output
Overall this is solid, well-structured work. The architecture decisions are good — using the bundler's own import graph via vinext:server-externals-manifest instead of regex scanning, the BFS copy with transitive dep resolution, the computeLazyChunks extraction to break the prod-server → index.ts import chain, and vinext's own runtime deps being included. Test coverage is thorough and hits the important edge cases (exports-map fallback, symlinks, missing deps, vinext runtime deps).
Prior review rounds have been addressed well. I have a few remaining items — one functional concern, a couple of minor issues, and some nits.
| // would be stale duplicates. Check only the relative portion of the | ||
| // path (after packageRoot) so the source path's own node_modules | ||
| // ancestor doesn't accidentally filter out the package itself. | ||
| filter: (src) => !path.relative(packageRoot, src).includes(`node_modules`), |
There was a problem hiding this comment.
The node_modules filter string match lacks path.sep boundaries, so it would also filter out a hypothetical file or directory whose name merely contains node_modules as a substring (e.g. not_node_modules_v2/). In practice this doesn't happen with real packages, but the more robust check is:
| filter: (src) => !path.relative(packageRoot, src).includes(`node_modules`), | |
| filter: (src) => { | |
| const rel = path.relative(packageRoot, src); | |
| return !rel.split(path.sep).includes("node_modules"); | |
| }, |
This checks against actual path segments rather than doing a substring search. Non-blocking — the current approach works for all realistic cases.
|
|
||
| function writeStandaloneServerEntry(filePath: string): void { | ||
| const content = `#!/usr/bin/env node | ||
| const path = require("node:path"); |
There was a problem hiding this comment.
The generated server.js uses require() (line 172) and __dirname (line 182) — CJS constructs. This works because of the {"type": "commonjs"} package.json written into the standalone directory. However, the CJS require("node:path") on line 172 is only used for path.join — the actual vinext import uses await import(). Since the only reason for require here is path.join, and the rest is ESM-style dynamic import, consider whether a fully-ESM server.js using import.meta.dirname (Node >= 21.2, and vinext requires >= 22) would be cleaner and eliminate the need for the CJS package.json entirely.
Not blocking — the current CJS approach is correct and the {"type": "commonjs"} package.json ensures it works.
| path.join(vinextPackageRoot, "package.json"), | ||
| path.join(vinextTargetDir, "package.json"), | ||
| ); | ||
| fs.cpSync(vinextDistDir, path.join(vinextTargetDir, "dist"), { |
There was a problem hiding this comment.
The vinext dist copy (cpSync) doesn't use the same node_modules filter that package copies use (line 130-138). If vinext's dist/ ever contained a node_modules directory (unlikely but possible during development), it would be copied. Consider adding the same filter here for consistency.
Non-blocking.
|
|
||
| for (const item of Object.values(bundle)) { | ||
| if (item.type !== "chunk") continue; | ||
| const chunk = item; |
There was a problem hiding this comment.
Minor: const chunk = item; is a redundant alias — item is already narrowed to OutputChunk by the item.type !== "chunk" guard above. You can use item directly.
| const chunk = item; | |
| const chunk = item; |
Not blocking — this is purely a style nit.
| // For App Router RSC: options.dir is dist/server. | ||
| // For App Router SSR: options.dir is dist/server/ssr. | ||
| // We always want dist/server as the manifest location. | ||
| if (!outDir) { |
There was a problem hiding this comment.
The outDir resolution walks up from options.dir looking for a parent named "server". This works for the current directory layout (dist/server, dist/server/ssr) but is fragile if a user's project path happens to contain a directory named server above the dist directory (e.g. /home/user/server/my-app/dist/server/ssr). The walk would stop at the wrong level.
A safer approach would be to look for "server" only within the dist tree — e.g. walking up at most until you find a directory that contains both a sibling client directory. Or simply hardcode the known structure: if basename(dir) === "ssr", use dirname(dir), otherwise use dir directly.
Low risk in practice since this only fires during writeBundle where options.dir is the Vite-controlled output directory.
| ); | ||
| } | ||
|
|
||
| const DEFAULT_PHASE = PHASE_DEVELOPMENT_SERVER; |
There was a problem hiding this comment.
Nit: DEFAULT_PHASE is only used to avoid repeating PHASE_DEVELOPMENT_SERVER in two default parameter values (lines 306 and 355). This is fine, but the indirection adds a layer of naming that doesn't clearly convey why this is the default. A comment would help:
| const DEFAULT_PHASE = PHASE_DEVELOPMENT_SERVER; | |
| // Used as the default `phase` argument for config loading functions. | |
| // Dev-server phase is the safe default since config loading happens at | |
| // dev startup and inside the Vite plugin (which only runs in dev). | |
| const DEFAULT_PHASE = PHASE_DEVELOPMENT_SERVER; |
packages/vinext/src/cli.ts
Outdated
| // Without this, a missing dist/ (e.g. from a broken install) only surfaces after | ||
| // the full multi-minute Vite build completes, when emitStandaloneOutput runs. | ||
| if (outputMode === "standalone") { | ||
| const vinextDistDir = path.resolve(path.dirname(fileURLToPath(import.meta.url)), "..", "dist"); |
There was a problem hiding this comment.
This pre-flight check resolves vinext's dist using import.meta.url, which points to the CLI source file's location. During development (running from source), import.meta.url is the .ts file under packages/vinext/src/cli.ts, so the .. walk goes to packages/vinext/src/../dist = packages/vinext/dist. In a published package, import.meta.url would be dist/cli.js, so ../dist = dist — also correct.
But emitStandaloneOutput uses resolveVinextPackageRoot() which does path.resolve(currentDir, "..", "..") (two levels up from dist/build/standalone.js). These two different path computation strategies could diverge if the file layout changes. Consider extracting this into a shared helper or at least adding a comment noting the two locations must agree.
tests/standalone-build.test.ts
Outdated
| @@ -0,0 +1,462 @@ | |||
| import { afterEach, beforeEach, describe, expect, it } from "vitest"; | |||
There was a problem hiding this comment.
Nit: the import uses "vitest" but the project's AGENTS.md says imports should come from "vite-plus/test". If this repo enforces that convention, this should be updated.
| import { afterEach, beforeEach, describe, expect, it } from "vitest"; | |
| import { afterEach, beforeEach, describe, expect, it } from "vite-plus/test"; |
Check whether other test files in this repo use "vitest" or "vite-plus/test" — if existing tests use "vitest", this is fine as-is.
| } | ||
| } | ||
|
|
||
| function copyPackageAndRuntimeDeps( |
There was a problem hiding this comment.
Functional concern: copyPackageAndRuntimeDeps creates a rootResolver using createRequire(path.join(root, "package.json")). When called for vinext's own runtime deps (line 302-306), root is vinextPackageRoot. If vinext is installed inside the user's node_modules (typical case), its package.json location is something like /app/node_modules/vinext/package.json. The resolver created from there can resolve vinext's own deps if they're hoisted to /app/node_modules/. But in a pnpm strict layout where vinext's deps are in a .pnpm store and only symlinked into vinext's own node_modules, the rootOptional set computed from vinext's package.json would be used to determine optionality.
The issue: vinext's package.json lists its deps in dependencies, not optionalDependencies, so rootOptional would be empty and all vinext deps would be treated as required (which is correct). However, rootPkg.optionalDependencies is read from vinext's package.json, which has no optionalDependencies field — so Object.keys(rootPkg.optionalDependencies ?? {}) returns []. This is fine.
Actually, looking more carefully, this is correct. Disregard the concern — the function works correctly for both the app root and vinext root cases.
|
Review submitted successfully. Here's a summary of what I flagged: All items are non-blocking. The PR is in good shape after 6 prior review rounds. The major issues from earlier rounds (CJS/ESM mismatch, Items I flagged:
|
- harden outDir resolution in server-externals-manifest: replace fragile walk-up heuristic with direct basename check (ssr -> dirname, else dir), avoiding misfires when a user's project path contains a 'server' segment - remove redundant 'chunk' alias in server-externals-manifest writeBundle; use 'item' directly after the type guard narrows it to OutputChunk - add node_modules filter to vinext dist/ cpSync in standalone.ts, matching the same defensive filter used for app package copies - rewrite standalone server.js as pure ESM using import.meta.dirname (Node >= 21.2, vinext requires >= 22); drop CJS require/__dirname and switch standalone package.json to 'type':'module' - add explanatory comment for DEFAULT_PHASE constant in next-config.ts - add comment in cli.ts noting pre-flight path and resolveVinextPackageRoot must stay in sync
|
/bigbonk review |
There was a problem hiding this comment.
Review: Standalone self-host output
This has been through several review rounds and the architecture is now solid. The key decisions are good:
- Using
vinext:server-externals-manifest(bundler's own import graph) instead of regex scanning is the right approach — authoritative and maintenance-free. - The BFS transitive dependency copy with
node_modulesfiltering is well-structured. - The
computeLazyChunksextraction toutils/lazy-chunks.tscleanly breaks theprod-server → index.tsimport chain. - vinext's own runtime deps are properly copied from vinext's package root (not the app root).
- Test coverage is thorough — transitive deps, symlinks, exports-map fallback, missing deps, vinext runtime deps, and no-manifest fallback are all covered.
I found one actual bug and a few non-blocking items. The bug should be fixed before merge; the rest are improvement suggestions.
Bug (must-fix)
- Test file imports from
"vitest"instead of"vite-plus/test"— Every other test file in this repo imports from"vite-plus/test"(seeinit.test.ts:1,routing.test.ts:1,shims.test.ts:1). The standalone test uses"vitest"directly, which violates the project convention documented in AGENTS.md. This will likely fail or cause issues with the Vite+ toolchain.
Non-blocking
- Divergent vinext dist path computation — The CLI pre-flight check and
resolveVinextPackageRoot()use different traversal strategies to find vinext's dist. A comment documents this, but extracting a shared helper would eliminate the fragility. outDirresolution assumes onlyssras a sub-directory — Thebasename(dir) === "ssr"check in the manifest plugin is correct for the current layout (RSC →dist/server, SSR →dist/server/ssr) but would silently break if another sub-directory were added (e.g.rscsub-dir). Low risk given the current architecture.- Standalone
server.jsusesimport.meta.dirname— The comment says this, but thepackage.jsonstill sets"type": "module"(correct for ESM). The earlier review rounds flagged a CJS/ESM mismatch that was fixed by writing{"type": "commonjs"}, but the current code correctly writes{"type": "module"}sinceserver.jsis now pure ESM. Just confirming this is intentional and consistent.
tests/standalone-build.test.ts
Outdated
| @@ -0,0 +1,462 @@ | |||
| import { afterEach, beforeEach, describe, expect, it } from "vitest"; | |||
There was a problem hiding this comment.
Bug: Every other test file in this repo imports from "vite-plus/test", not "vitest" directly. This is a project convention documented in AGENTS.md ("Import JavaScript modules from vite-plus") and enforced across the codebase (init.test.ts, routing.test.ts, shims.test.ts, etc. all use "vite-plus/test").
| import { afterEach, beforeEach, describe, expect, it } from "vitest"; | |
| import { afterEach, beforeEach, describe, expect, it } from "vite-plus/test"; |
packages/vinext/src/cli.ts
Outdated
| // Must stay in sync with resolveVinextPackageRoot() in standalone.ts, which | ||
| // uses path.resolve(currentDir, "..", "..") from dist/build/standalone.js | ||
| // and arrives at the same package root via a different traversal depth. | ||
| const vinextDistDir = path.resolve(path.dirname(fileURLToPath(import.meta.url)), "..", "dist"); |
There was a problem hiding this comment.
This path computation resolves vinext's dist via import.meta.url → one level up. But resolveVinextPackageRoot() in standalone.ts uses import.meta.url → two levels up (from dist/build/standalone.js). The comment on lines 382-386 documents this coupling, which is good, but the two strategies will silently diverge if either file moves.
Consider extracting this into a shared helper (e.g. in utils/) that both cli.ts and standalone.ts import, so there's a single source of truth for locating vinext's package root.
| // bundled into chunk files. packageNameFromSpecifier filters out chunk | ||
| // filenames (relative/absolute paths) and extracts the package name from | ||
| // bare specifiers — which is exactly what the standalone BFS needs. | ||
| for (const specifier of [...item.imports, ...item.dynamicImports]) { |
There was a problem hiding this comment.
Minor: item.imports and item.dynamicImports on OutputChunk are required string[] fields in Rollup's type definitions — they're never undefined. The spread into a new array is fine for iteration, but the earlier reviewer's note about ?? [] being unnecessary applies here too (since there's no ?? in this version, just confirming this is correct as-is).
Also worth noting for future readers: the comment on lines 96-101 explaining why bare specifiers appear in chunk.imports is excellent — this is non-obvious and saves someone a trip to the Rollup docs.
| fs.rmSync(standaloneDir, { recursive: true, force: true }); | ||
| fs.mkdirSync(standaloneDistDir, { recursive: true }); | ||
|
|
||
| fs.cpSync(clientDir, path.join(standaloneDistDir, "client"), { |
There was a problem hiding this comment.
Nit: The dist/client and dist/server copies here don't use the node_modules filter that copyPackageAndRuntimeDeps uses (line 138). This is fine — build output directories shouldn't contain node_modules — but for consistency with the defensive vinext dist copy on line 295, you could add the filter here too. Very low priority.
| } | ||
| } | ||
|
|
||
| function copyPackageAndRuntimeDeps( |
There was a problem hiding this comment.
Nice design: making copyPackageAndRuntimeDeps accept an alreadyCopied set parameter means it's reusable for both the app's externals and vinext's own runtime deps without duplicating the BFS logic. The earlier review rounds suggested exactly this refactor, and it's well-executed.
| return null; | ||
| } | ||
|
|
||
| export { PHASE_DEVELOPMENT_SERVER, PHASE_PRODUCTION_BUILD } from "../shims/constants.js"; |
There was a problem hiding this comment.
Minor style note: This re-export at the end of the file is clean. Both constants are already imported on line 11, so this export { ... } from form is the right pattern — it re-exports without creating a duplicate binding. Earlier review rounds flagged a bare export { ... } sandwiched between imports; this final form is correct.
|
Review submitted successfully. Here's a summary of what I found: Bug (must-fix)
Non-blocking items
The architecture is solid overall — using the bundler's own import graph via the externals manifest, the BFS dependency copy with |
|
- fix test import: use 'vite-plus/test' instead of 'vitest' in standalone-build.test.ts, matching the convention used by every other test file in the repo (documented in AGENTS.md) - extract resolveVinextPackageRoot into utils/vinext-root.ts so cli.ts and standalone.ts share a single source of truth; removes the coupling comment added in round 7 and the duplicated path traversal logic - add node_modules filter to dist/client and dist/server cpSync calls for defensive consistency with the vinext dist/ and package copies
|
/bigbonk review |
There was a problem hiding this comment.
Review: Standalone self-host output
This PR is well-structured and addresses a real need — output: "standalone" for self-hosted VPS deployments. The architecture is clean: the bundler's own import graph (via vinext:server-externals-manifest) seeds the BFS dependency copy, avoiding fragile regex scanning. The computeLazyChunks extraction to break the prod-server → index.ts import chain is a good refactor.
After 8+ review rounds, the major issues from earlier iterations (CJS/ESM mismatch, HOSTNAME collision, missing vinext runtime deps, duplicate phase constants, incomplete extraction) have all been resolved. The remaining items below are non-blocking improvements.
Items
-
public/directory not filtered fornode_modules— Thepublic/directory copy (line 249) uses nofiltercallback, unlike all othercpSynccalls in this function. While apublic/directory containingnode_modulesis extremely unlikely, it breaks the defensive consistency established by the rest of the function. -
copyPackageAndRuntimeDepsreturn value is misleading — The function returns[...copied], but when called with analreadyCopiedset, the returned array includes entries that were already in the set before the call (i.e., packages this invocation didn't copy). The return value is only used indirectly viacopiedSetinemitStandaloneOutput, so it's not buggy, but the function signature suggests it returns "what was copied" rather than "everything in the set". -
readServerExternalsManifestsilently returns[]on parse errors — If the manifest JSON is malformed (e.g., a partial write from a crashed build), the standalone output will silently produce an emptynode_moduleswith no server dependencies. A warning log in thecatchblock would help debugging. -
Scoped package handling in
packageNameFromSpecifier— When a specifier is just"@scope"with no slash after it (malformed),parts.length >= 2is false and it returnsnull. This is correct behavior, but the function doesn't document that bare scope specifiers are intentionally rejected. -
Minor:
server.jsentry hardcodesPORTandHOSTenv var names — These are reasonable defaults, but worth noting that Next.js standalone usesPORTandHOSTNAME(with the known footgun). The switch toHOSTis an intentional divergence that should be documented in the README's standalone section for users migrating from Next.js standalone.
Overall the code quality is good, the test coverage is thorough (transitive deps, symlinks, exports-map fallback, missing deps, vinext runtime deps, no-manifest fallback), and the architecture is sound. The items above are all non-blocking.
| fs.cpSync(publicDir, path.join(standaloneDir, "public"), { | ||
| recursive: true, | ||
| dereference: true, | ||
| }); |
There was a problem hiding this comment.
Nit: This is the only cpSync call in the function that doesn't have a node_modules filter. While public/ containing node_modules is extremely unlikely, adding the filter would be consistent with the defensive pattern used everywhere else in this function.
| }); | |
| fs.cpSync(publicDir, path.join(standaloneDir, "public"), { | |
| recursive: true, | |
| dereference: true, | |
| filter: (src) => !path.relative(publicDir, src).split(path.sep).includes("node_modules"), | |
| }); |
| } | ||
| try { | ||
| return JSON.parse(fs.readFileSync(manifestPath, "utf-8")) as string[]; | ||
| } catch { |
There was a problem hiding this comment.
Silent failure on malformed JSON could make debugging painful — a user with a corrupted vinext-externals.json from a crashed build would get a standalone directory with zero server dependencies, and no indication why.
Consider logging a warning:
| } catch { | |
| } catch (err) { | |
| console.warn(`[vinext] Warning: failed to parse ${manifestPath}, proceeding without externals manifest`); | |
| return []; | |
| } |
| } | ||
| } | ||
|
|
||
| return [...copied]; |
There was a problem hiding this comment.
The return type suggests "packages copied by this call", but when alreadyCopied is passed, it returns the full accumulated set including pre-existing entries. This isn't buggy (callers use copiedSet directly), but the semantics are slightly misleading. A brief doc comment noting this would help:
// Returns the full set of copied packages (including those in `alreadyCopied` before this call).| // dist/server. Using basename avoids the fragile walk-up heuristic | ||
| // which would misfire if a user's project path contains a directory | ||
| // named "server" above the dist output (e.g. /home/user/server/my-app/). | ||
| outDir = path.basename(dir) === "ssr" ? path.dirname(dir) : dir; |
There was a problem hiding this comment.
The outDir is set only once (from the first server env that fires writeBundle). If the ssr environment fires before rsc for some reason, outDir would be set to path.dirname(dir) (going up from dist/server/ssr to dist/server), which is correct. But if a future environment name other than ssr had a sub-directory layout (e.g. dist/server/rsc), this logic would miss it.
Since the current architecture only has ssr as a sub-directory, this is fine. Just confirming the assumption is documented in the comment — which it is (lines 86-90). No change needed.
| startProdServer({ | ||
| port, | ||
| host, | ||
| outDir: join(import.meta.dirname, "dist"), |
There was a problem hiding this comment.
Good: the generated entry is pure ESM with import.meta.dirname (Node >= 21.2), and the standalone package.json correctly sets "type": "module". The CJS/ESM mismatch from the original version is fully resolved.
Minor observation: import.meta.dirname was stabilized in Node 21.2 but the vinext docs/package.json should confirm the minimum Node version requirement covers this (Node >= 22 does).
|
|
||
| If your `next.config.*` sets `output: "standalone"`, `vinext build` emits a self-hosting bundle at `dist/standalone/`. Start it with: | ||
|
|
||
| ```bash |
There was a problem hiding this comment.
Consider adding a note that standalone mode uses HOST (not HOSTNAME) for the bind address. Users migrating from Next.js standalone may have HOSTNAME configured in their deployment environments, and the behavioral difference would be confusing without documentation.
Something like:
Environment variables: `PORT` (default 3000), `HOST` (default 0.0.0.0).
Note: Next.js standalone uses `HOSTNAME`, but vinext uses `HOST` to avoid
collision with the system-set HOSTNAME variable on Linux.
| return null; | ||
| } | ||
|
|
||
| export { PHASE_DEVELOPMENT_SERVER, PHASE_PRODUCTION_BUILD } from "../shims/constants.js"; |
There was a problem hiding this comment.
Clean re-export pattern. Both constants are imported on line 11 for local use and re-exported here for external consumers (CLI). This is the right approach — no duplicate declarations.
|
Review submitted successfully. Here's a summary of my findings: Overall assessment: The PR is in good shape after 8+ review rounds. All major issues from prior rounds are resolved — the CJS/ESM mismatch, Items I flagged (all non-blocking):
|
- add node_modules filter to public/ cpSync for defensive consistency with all other cpSync calls in emitStandaloneOutput - warn on malformed vinext-externals.json instead of silently returning an empty list; includes the error string to aid debugging - add doc comment on copyPackageAndRuntimeDeps clarifying that the return value is the full accumulated set (including pre-existing entries), not only packages copied by the current call - document HOST (not HOSTNAME) env var in README standalone section, with a note explaining the difference from Next.js standalone behaviour
|
/bigbonk review |
There was a problem hiding this comment.
Review: Standalone self-host output
This PR has been through many review rounds and the quality shows. The architecture is well-designed:
- Externals manifest via bundler graph (
vinext:server-externals-manifest) is the right approach — authoritative, no regex fragility. - BFS transitive dependency copy with
node_modulesfiltering is solid. computeLazyChunksextraction toutils/lazy-chunks.tscleanly breaks theprod-server → index.tsimport chain.- vinext's own runtime deps are correctly copied from vinext's package root.
resolveVinextPackageRootcentralizes the path computation in a shared utility.- Pre-flight check catches missing vinext dist before the expensive build.
- Test coverage is thorough — transitive deps, symlinks, exports-map fallback, missing deps, vinext runtime deps, no-manifest fallback.
Prior must-fix issues (CJS/ESM mismatch, HOSTNAME collision, missing vinext runtime deps, duplicate phase constants, incomplete extraction) are all resolved.
I found two small issues worth noting — neither is a hard blocker but one affects correctness in edge cases.
| // entry point is pure ESM — no need for CJS require() or __dirname. | ||
| const content = `#!/usr/bin/env node | ||
| import { join } from "node:path"; | ||
| import { startProdServer } from "vinext/server/prod-server"; |
There was a problem hiding this comment.
Static import in generated entry may fail at startup.
The generated server.js uses a static import for vinext/server/prod-server. Node will attempt to resolve this import before executing any code. If the standalone node_modules/vinext is somehow incomplete or the exports map in vinext's package.json doesn't expose ./server/prod-server, this will throw an ERR_MODULE_NOT_FOUND with no user-friendly error message.
Using a dynamic import() inside a try/catch would let you surface a better error:
| import { startProdServer } from "vinext/server/prod-server"; | |
| const { startProdServer } = await import("vinext/server/prod-server"); |
Alternatively, a comment confirming this is intentional (since prod-server is a documented export) would be fine. Non-blocking.
| // dist/server. Using basename avoids the fragile walk-up heuristic | ||
| // which would misfire if a user's project path contains a directory | ||
| // named "server" above the dist output (e.g. /home/user/server/my-app/). | ||
| outDir = path.basename(dir) === "ssr" ? path.dirname(dir) : dir; |
There was a problem hiding this comment.
The outDir is only set once (from the first server env that fires writeBundle). If ssr fires first, the basename === "ssr" check correctly walks up to dist/server. But if both rsc and ssr fire and rsc fires first, outDir is set to dist/server directly (correct).
However, the ordering assumption is implicit. If a future environment (e.g. a hypothetical edge sub-env) also outputs to a sub-directory like dist/server/edge, this basename === "ssr" check would not walk up for it. The comment on lines 86-90 documents the assumption, which is good enough for now, but consider making this more robust by checking basename(dir) !== "server" instead of hard-coding "ssr":
| outDir = path.basename(dir) === "ssr" ? path.dirname(dir) : dir; | |
| outDir = path.basename(dir) === "server" ? dir : path.dirname(dir); |
This handles any sub-directory name, not just ssr. Non-blocking.
| // bundled into chunk files. packageNameFromSpecifier filters out chunk | ||
| // filenames (relative/absolute paths) and extracts the package name from | ||
| // bare specifiers — which is exactly what the standalone BFS needs. | ||
| for (const specifier of [...item.imports, ...item.dynamicImports]) { |
There was a problem hiding this comment.
Nit: the spread into a new array ([...item.imports, ...item.dynamicImports]) allocates on every chunk. Since this runs during writeBundle (not hot path), it's fine for correctness. But if you ever want to micro-optimize, you could iterate both arrays separately:
for (const specifier of item.imports) { ... }
for (const specifier of item.dynamicImports) { ... }Truly non-blocking — just noting it.
| return JSON.parse(fs.readFileSync(packageJsonPath, "utf-8")) as PackageJson; | ||
| } | ||
|
|
||
| function runtimeDeps(pkg: PackageJson): string[] { |
There was a problem hiding this comment.
Minor: runtimeDeps includes optionalDependencies in the returned list, which is correct for the BFS seed. But the name runtimeDeps doesn't clearly convey that it includes optional deps. A name like allRuntimeDeps or a brief doc comment would help:
/** Returns both `dependencies` and `optionalDependencies` keys — the full set of potential runtime deps. */Non-blocking.
| try { | ||
| const entryPath = resolver.resolve(packageName); | ||
| let dir = path.dirname(entryPath); | ||
| while (dir !== path.dirname(dir)) { |
There was a problem hiding this comment.
The walk-up loop checks pkg.name === packageName to confirm the right package.json was found. This is correct and handles the case where the entry resolves deep inside a nested directory (e.g. node_modules/@scope/pkg/dist/esm/index.js).
One edge case: scoped packages where resolvePackageJsonPath is called with @scope/pkg — the pkg.name in package.json would be @scope/pkg, so the equality check works. Just confirming this is tested via the dep-hidden test case. Looks good.
| return null; | ||
| } | ||
|
|
||
| export { PHASE_DEVELOPMENT_SERVER, PHASE_PRODUCTION_BUILD } from "../shims/constants.js"; |
There was a problem hiding this comment.
Clean pattern — re-exporting from the canonical source (shims/constants.ts) without creating duplicate declarations. The cli.ts import from config/next-config.js keeps the dependency graph clean (CLI doesn't import from shims/ directly).
| console.log(`\n vinext build (Vite ${getViteVersion()})\n`); | ||
|
|
||
| const isApp = hasAppDir(); | ||
| const resolvedNextConfig = await resolveNextConfig( |
There was a problem hiding this comment.
The config is loaded at the top of buildApp() with PHASE_PRODUCTION_BUILD, which is correct — function-form configs that switch on phase will now get the right behavior during builds.
The loadNextConfig + resolveNextConfig calls here replace the previous loadNextConfig call that happened after the build (old line ~465). This means config is now loaded twice if the Vite plugin also loads it during config(). For production builds this is fine (config is typically fast to load), but worth being aware of. No action needed.
| @@ -0,0 +1,462 @@ | |||
| import { afterEach, beforeEach, describe, expect, it } from "vite-plus/test"; | |||
There was a problem hiding this comment.
Good — this correctly uses "vite-plus/test" matching the project convention. The earlier review round that flagged "vitest" has been addressed.
|
Review submitted successfully. Here's a summary of what I found: Overall assessment: The PR is in good shape after 9+ review rounds. All previously flagged must-fix issues have been resolved. The architecture is clean and well-designed. Key strengths:
Items flagged (all non-blocking):
|
3 participants
Summary
output: \"standalone\"support invinext buildthat emitsdist/standalonewith a runnableserver.js, built artifacts, and copied runtime dependencies for self-hosted VPS deploymentsnext.configloading for build mode, and extractcomputeLazyChunksinto a shared utility so the production server no longer imports the full plugin modulevinext initto adddev:vinext,build:vinext, andstart:vinextscripts using the vinext CLI, plus docs/check metadata and new unit coverageTesting
corepack pnpm vitest run tests/init.test.ts tests/standalone-build.test.tscorepack pnpm exec oxlint packages/vinext/src/build/standalone.ts packages/vinext/src/cli.ts packages/vinext/src/init.ts packages/vinext/src/utils/lazy-chunks.ts tests/standalone-build.test.ts tests/init.test.ts packages/vinext/src/config/next-config.ts packages/vinext/src/server/prod-server.ts packages/vinext/src/index.ts packages/vinext/src/check.tscorepack pnpm --filter vinext run build