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 8 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
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