fix(build): honor inline next config for static export - #2543
Conversation
|
/bigbonk review for issues |
|
Performance benchmarksCompared 1 improved · 2 regressed · 6 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
|
@james-elicx Bonk workflow was cancelled. View workflow run · To retry, trigger Bonk again. |
|
/bigbonk review for issues |
1 similar comment
|
/bigbonk review for issues |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: honor inline next config for static export
Reviewed the full diff, ran the targeted tests (tests/deploy-prerender-config.test.ts + tests/prerender-config.test.ts, 23 passing), and typechecked/linted all four changed source files (clean). The core change is correct: both production prerender entrypoints (cli.ts:699, deploy.ts:877) now pass nextConfigOverride, so inline-only output: "export" is honored, and the deploy path resolves inline config inside the selected CLOUDFLARE_ENV.
Correctness — looks good
findVinextNextConfigInPluginscorrectly skipsnull/undefined(if (nextConfig) return nextConfig), so the always-present__vinextNextConfig: options.nextConfig ?? nullproperty doesn't accidentally match when no inline config is set.loadBuildViteConfigMetadatais now called once inbuildApp()(line 494) and the duplicate later call was removed — no doubleloadConfigFromFile.deploy.tswraps nextConfig resolution inwithCloudflareEnv(buildEnv, ...), matching the existing pattern used for cache/prerender/routeRoot config.shouldLoadVinextPrerenderConfignow reads the inline-awarenextConfig.output, so export mode short-circuits vinext prerender config loading as intended.- Both prerender call sites are covered — no other
runPrerenderinvocations exist that would need the override.
Non-blocking observations
-
Redundant disk config read in
run-prerender.ts:188. WhennextConfigOverrideis supplied (now the case for every production prerender),resolveNextConfig(await loadNextConfig(root), root)still runs on line 188, then is fully spread-over by the override. For inline-only apps with nonext.config.*on disk this is harmless (loadNextConfigreturnsnull), but it's wasted work and re-reads/parses disk config that the caller already resolved. Consider skipping the load when an override is present, or documenting why the base load is still needed (e.g. to catch keys the override doesn't set — but a full override spread means the base is only useful for thebuildIdfile read below, which is separate). Worth a follow-up. -
Vite config is loaded multiple times per deploy.
loadVinextNextConfigFromViteConfig,loadVinextCacheConfigFromViteConfig,loadVinextPrerenderConfigFromViteConfig, and the routeRoot loader each independently callloadProjectViteApi+loadConfigFromFile. This PR adds a fourthloadConfigFromFileon the deploy path. Pre-existing pattern, but as the count grows it may be worth loading the Vite config once and reading all vinext plugin properties from a singleloaded.config.pluginspass.
Neither observation blocks merge. The change is well-scoped, tested, and the parity between cli.ts and deploy.ts prerender paths is maintained.
|
Review posted to PR #2543. SummaryI reviewed the full diff, ran the targeted tests (23 passing), and typechecked/linted all changed source files (clean). Verdict: no blocking issues. The change correctly makes Correctness checks that passed:
Two non-blocking observations noted in the review:
|
|
/bigbonk review for issues |
commit: |
There was a problem hiding this comment.
Review: honor inline next config for static export
Reviewed the full diff (including the follow-up commit 08d2bc7), ran the targeted tests (tests/deploy-prerender-config.test.ts, tests/prerender-config.test.ts, tests/prerender.test.ts — 115 passing), and ran vp check on all changed source files (format/lint/type — clean).
The core change is correct and well-tested: both production prerender entrypoints (cli.ts:702, deploy.ts:898) now pass a fully-resolved nextConfig, and run-prerender.ts uses it directly instead of reloading disk config — so inline-only output: "export" is honored end-to-end. The deploy consolidation into loadDeployViteConfigMetadata (one loadConfigFromFile per deploy) directly addresses the prior review's observation #2, and the run-prerender.ts change addresses observation #1.
Non-blocking findings
1. Undocumented behavioral regression: deploy now always loads the Vite config. The previous deploy path deliberately skipped loading the Vite config when the prerender decision was already settled (--prerender-all or disk next.config output: "export"). This was guarded by two tests using a throwing vite config — both deleted in this PR (does not load Vite config when the prerender-all flag already wins, does not load Vite config when static export already wins). The new loadDeployViteConfigMetadata runs unconditionally at deploy.ts:846, so a project with a broken/throwing vite.config.ts that previously deployed under --prerender-all will now fail.
This eager load is inherent to the feature (you can't decide to skip loading the config you need to read inline nextConfig from), so I'm not asking to change the behavior. But the deletion of those guard tests removes a real invariant with no replacement assertion or comment. Consider adding a short comment at the loadDeployViteConfigMetadata call site noting that the Vite config must be loaded eagerly to discover inline nextConfig, so the reason it's no longer lazy is captured for the next reader.
2. Async/sync flatten inconsistency across the four plugin finders. The new findVinextNextConfigInPlugins uses an async flattenPluginOptions that awaits Promise plugin entries (and there's an explicit test: discovers inline Next.js config through promised plugin composition). But the three sibling finders called on the same plugins array in loadDeployViteConfigMetadata — findVinextCacheConfigInPlugins, findVinextPrerenderConfigInPlugins, findVinextRouteRootConfigInPlugins — use synchronous flatten helpers that do not resolve Promises (config/prerender.ts:70, cache/cache-adapters-virtual.ts:62). loadConfigFromFile returns the raw user plugins without resolving promises, so a user who wraps vinext() in Promise.resolve(...) would get their inline nextConfig discovered but their prerender/cache/routeRoot config silently dropped. Not a regression (those finders were already sync), but the PR now introduces an asymmetry where one property is promise-aware and three are not. Worth a follow-up to unify them (or a comment noting the limitation).
3. Redundant disk read still present in emitPrerenderPathManifest. run-prerender.ts was fixed to skip loadNextConfig when a full config is supplied, but the parallel path in build/prerender-paths.ts:375 still does resolveNextConfig(await loadNextConfig(root)) and then fully spreads nextConfigOverride (the complete resolved config) over it. Result is correct (output: "export" is honored there too), but the disk read is dead work whenever the override is present — the same pattern this PR just removed from run-prerender.ts. Both cli.ts and deploy.ts call emitPrerenderPathManifest with the full resolved config, so this path could take the same nextConfig treatment.
Correctness checks that passed
findVinextNextConfigInPluginscorrectly skips the always-present__vinextNextConfig: options.nextConfig ?? null(if (nextConfig) return nextConfigshort-circuits onnull), so no false match when no inline config is set.loadBuildViteConfigMetadatais now called exactly once inbuildApp()and the duplicate later call was removed — no doubleloadConfigFromFileinvinext build.- Deploy resolves inline config inside
withCloudflareEnv(buildEnv, ...), and the movedcacheConfigload preserves the same env scoping it had when it lived insidepopulateKVCacheFromPrerenderedArtifacts. Verified by theresolves function-form inline config inside the selected Cloudflare environmenttest (CLOUDFLARE_ENV === "preview"→output: "export",buildId: "preview"). run-prerender.tsbuildId reuse still works:configis always a fresh object andreadBuiltBuildIdoverridesconfig.buildIdafter the merge, in both thenextConfigandnextConfigOverridebranches.- Both
runPrerendercall sites are covered — no other invocations need the override.
None of the three findings block merge. The change is well-scoped and the cli/deploy prerender parity is maintained.
|
Review posted to PR #2543. SummaryReviewed the full diff (including follow-up commit Verdict: no blocking issues. The core change correctly makes Three non-blocking findings I flagged:
Correctness checks that passed: |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: honor inline next config for static export
Reviewed the full diff through the latest commit fd695b6 (which the two prior bonk reviews predate), ran the targeted suites (deploy-prerender-config, prerender-config, prerender, prerender-paths, cache-adapters-config — 145 passing), and ran vp check on all 8 changed source files (format/lint/type — clean).
Verdict: no blocking issues. The core change is correct and well-tested, and the follow-up commit fd695b6 resolves both non-blocking findings from the earlier reviews.
The two prior review findings are now addressed
-
Async/sync flatten asymmetry (prior finding #2) — fixed.
findVinextCacheConfigInPlugins,findVinextPrerenderConfigInPlugins, andfindVinextRouteRootConfigInPluginsare now allasyncwith promise-awareflattenPluginOptions, matchingfindVinextNextConfigInPlugins. All 7 call sites (cli.ts,deploy.ts, and theloadVinext*FromViteConfigwrappers) nowawaitthem, and new tests cover promised plugin composition for cache, prerender, and route-root config. No remaining asymmetry. -
Redundant disk read in
emitPrerenderPathManifest(prior finding #3) — fixed.prerender-paths.ts:380-385now takes a fullnextConfigand skipsloadNextConfigentirely when supplied, falling back to disk-load +nextConfigOverrideonly when it isn't. Bothcli.ts:706anddeploy.ts:881pass the fully-resolvednextConfig, so the dead disk read is gone. New testdoes not reload disk config when supplied resolved configuses a throwingnext.config.mjsto prove the load is skipped. -
Documented eager Vite-config load (prior finding #1). The
loadDeployViteConfigMetadatacall atdeploy.ts:846-848now carries a comment explaining why the load must be eager (inlinenextConfigcan decide export/prerender behavior). The consolidation to a singleloadConfigFromFileper deploy is verified by the newloads Vite config once for all deploy metadatatest (asserts a load-count of exactly 1).
Correctness checks that passed
findVinextNextConfigInPluginsshort-circuits on the always-present__vinextNextConfig: options.nextConfig ?? null(if (nextConfig) return nextConfigskipsnull), so no false match when inline config is unset.flattenPluginOptionsuses strictvalue instanceof Promise, so real VitePluginobjects (which are notPromiseinstances) are never mis-awaited; only genuinePromise<Plugin>entries inPluginOption[]are resolved.- cli/deploy prerender parity is maintained: both resolve inline config before the output-mode decision (
cli.ts:495-536,deploy.ts:851-867) and pass the same resolvednextConfigintorunPrerenderandemitPrerenderPathManifest. buildIdreuse is intact in both branches ofrun-prerender.tsandprerender-paths.ts(fresh object spread, thenreadBuiltBuildIdoverride).- Deploy env scoping preserved:
cacheConfigis loaded once insidewithCloudflareEnv(buildEnv, ...), matching the scoping it had inside the oldpopulateKVCacheFromPrerenderedArtifacts; thefunction-form inline config inside the selected Cloudflare environmenttest confirmsCLOUDFLARE_ENV === "preview"→output: "export",buildId: "preview".
Non-blocking observations (follow-up, not required for this PR)
-
flattenPluginOptionsis now triplicated. Identical async copies live inconfig/next-config.ts:359,config/prerender.ts:70, andcache/cache-adapters-virtual.ts:62. Worth extracting into one shared helper (e.g. underutils/) to avoid the three diverging. -
Hybrid Pages Router plugin extraction still uses sync flatten. The separate
loadConfigFromFileatcli.ts:613(which extracts user transform plugins for the hybrid SSR build) uses.flat(Infinity)and does not resolve promised plugins. This is unrelated to config discovery and pre-existing — not a regression — but it means a user who wraps their transform plugins inPromise.resolve(...)would have them silently dropped from the hybrid Pages build. Consider unifying with the new async flatten in a follow-up.
Neither observation blocks merge. The change is well-scoped, the cli/deploy parity holds, and the follow-up commit closed the open review items.
|
Review posted to PR #2543. SummaryReviewed the full diff through the latest commit Verdict: no blocking issues. The core change correctly makes Key finding: the follow-up commit
Correctness checks passed: Two non-blocking follow-ups noted: |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: honor inline next config for static export
Reviewed the full diff through the latest commit dfb36cc — which the three prior bonk reviews all predate (they stopped at fd695b6). Ran the targeted suites (deploy-prerender-config, prerender-config, prerender, prerender-paths, plugin-options, cache-adapters-config — 147 passing) and ran vp check on all 9 changed source files (format/lint/type — clean).
Verdict: no blocking issues. The core fix is correct and well-tested: both vinext build and vinext-cloudflare deploy now resolve inline vinext({ nextConfig }) before the output-mode decision and thread the fully-resolved config into runPrerender / emitPrerenderPathManifest instead of reloading disk config.
The new commit dfb36cc closes both remaining follow-ups from prior reviews
-
flattenPluginOptionstriplication (prior finding #1) — fixed. The three copies inconfig/next-config.ts,config/prerender.ts, andcache/cache-adapters-virtual.tsare removed and now import a single shared helper fromutils/plugin-options.ts. The extracted version returns the array (rather than mutating a passed-in target), which is a cleaner signature; all call sites updated accordingly. -
Hybrid Pages Router sync flatten (prior finding #2) — fixed.
cli.ts:613's previous.flat(Infinity)extraction is replaced by the new promise-awareselectHybridPagesUserPlugins, so users who wrap transform plugins inPromise.resolve(...)are no longer silently dropped from the hybrid Pages SSR build. The framework-plugin exclusion filter (vinext:,vite:react,rsc:,vite-rsc-load-module-dev-proxy,vite-plugin-cloudflare) is preserved verbatim, and a new test asserts both promised selection and family exclusion.
Correctness checks that passed
- CLI build reordering is safe.
loadBuildViteConfigMetadatamoved from after the plugin setup (old line ~563) to before the output-mode decision (cli.ts:495), soresolvedNextConfig— and therefore__VINEXT_SHARED_BUILD_ID,outputMode, and the standalone/export branches — now correctly reflect inlinenextConfig. Nothing between the old and new call position reads state the move invalidates. The duplicate later call was removed (no doubleloadConfigFromFile). - Deploy env scoping is preserved and consistent.
loadDeployViteConfigMetadata(singleloadConfigFromFilefor all four metadata pieces) and the inline-config resolution are both wrapped inwithCloudflareEnv(buildEnv, ...), so a function-form inlinenextConfigthat readsCLOUDFLARE_ENVis evaluated inside the selected env — verified byresolves function-form inline config inside the selected Cloudflare environment(CLOUDFLARE_ENV === "preview"→output: "export",buildId: "preview").buildEnvand thewranglerEnvexpression evaluate to the same value, so the movedcacheConfigload keeps its original scoping. findVinextNextConfigInPluginsshort-circuits on the always-present__vinextNextConfig: options.nextConfig ?? null(if (nextConfig) return nextConfigskipsnull), so no false match when inline config is unset.flattenPluginOptionsuses strictvalue instanceof Promise, so real VitePluginobjects (notPromiseinstances) are never mis-awaited; only genuinePromise<Plugin>entries are resolved. Ordering is preserved (Promise.all(...).flat()).- Redundant disk read is gone from both prerender paths.
run-prerender.ts:190andprerender-paths.ts:380skiploadNextConfigentirely when the resolvednextConfigis supplied (which both cli and deploy now do), falling back to disk-load +nextConfigOverrideonly for the legacy test path.buildIdreuse viareadBuiltBuildIdis intact in both branches.does not reload disk config when supplied resolved configproves the skip with a throwingnext.config. - Inline function config gets
defaultConfig: {}andPHASE_PRODUCTION_BUILDviaresolveNextConfigInput→resolveConfigValue, matching how disk-config functions are resolved. - Both
runPrerendercall sites (cli + deploy) are covered — no missed invocations.
Non-blocking observations (follow-up, not required for this PR)
-
cacheConfigis now resolved eagerly even when there is no cache adapter.populateKVCacheFromPrerenderedArtifactsstill guards onviteConfigHasCacheAdapter(root)and returns early, butviteConfigMetadata.cacheConfigis always computed up front now. This is pure wasted parsing (no correctness impact), and it's more than offset by the consolidation to a singleloadConfigFromFileper deploy — just noting the resolved-but-unused case. -
Two
--prerender-all/ disk-export guard tests were deleted with no replacement. The previous deploy path lazily skipped loading the Vite config when the prerender decision was already settled; the tests using a throwingvite.config.tsto prove that (does not load Vite config when the prerender-all flag already wins/...when static export already wins) are gone. The eager load is inherent to the feature — you can't skip loading the config you must read inlinenextConfigfrom — and the newdeploy.ts:846comment plus theloads Vite config once for all deploy metadataload-count test capture the new invariant. So this is acceptable as-is; flagging only that the removed invariant is no longer asserted in the negative direction (a brokenvite.config.tswill now fail a--prerender-alldeploy that previously succeeded).
Neither observation blocks merge. The change is well-scoped, cli/deploy parity is maintained, and the latest refactor commit closed the open follow-ups from the earlier reviews.
|
Review posted to PR #2543. SummaryReviewed the full diff through the latest commit Verdict: no blocking issues. The fix correctly makes Key finding: the new commit
Correctness verified: CLI build reordering is safe (inline config now flows into Two non-blocking observations: |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: honor inline next config for static export
Independently reviewed the full diff through f011d06, ran the targeted suites (deploy-prerender-config, prerender-config, prerender-paths, plugin-options, cache-adapters-config — 58 passing), and ran vp check on all 9 changed source files (format/lint/type — clean).
Verdict: no blocking issues. The fix correctly makes vinext build and vinext-cloudflare deploy resolve inline vinext({ nextConfig }) before the output-mode decision and thread the fully-resolved config into runPrerender / emitPrerenderPathManifest instead of reloading disk config.
Correctness checks that passed
- Dev/build/deploy parity on inline-vs-disk semantics. The plugin's
confighook (index.ts:1848) treats inlinenextConfigas a full override of disk config (with a drift warning). The new build (cli.ts:496) and deploy (deploy.ts) paths now match this exactly: inline present →resolveNextConfigInput, elseloadNextConfig. No accidental merge, so no divergence from the existing runtime behavior. - Function-form parity. Inline function configs resolve through the same
resolveConfigValue(config, PHASE_PRODUCTION_BUILD)withdefaultConfig: {}used for disk function configs — verified by theresolves function-form inline config inside the selected Cloudflare environmenttest (CLOUDFLARE_ENV === "preview"→output: "export",buildId: "preview"). - CLI reorder is safe.
loadBuildViteConfigMetadatamoved ahead of the output-mode decision; the only later consumer of its result (emptyOutDiratcli.ts:578,routeRootConfigatcli.ts:690) still runs after the new call site. Duplicate later call removed — no doubleloadConfigFromFile. - Deploy env scoping preserved.
loadDeployViteConfigMetadataand the inline-config resolution are both wrapped inwithCloudflareEnv(buildEnv, ...), and the movedcacheConfigload keeps the same env scoping it had insidepopulateKVCacheFromPrerenderedArtifacts. The cache double-guard (viteConfigHasCacheAdapter(root)in both loader and consumer) stays consistent. findVinextNextConfigInPluginsshort-circuits on the always-present__vinextNextConfig: options.nextConfig ?? null(if (nextConfig) return nextConfigskipsnull), so no false match when inline config is unset.flattenPluginOptionsuses strictvalue instanceof Promise, so real VitePluginobjects are never mis-awaited; only genuinePromise<Plugin>entries resolve, and ordering is preserved (Promise.all(...).flat()). All four plugin finders are now uniformly async — no remaining sync/async asymmetry.- Redundant disk read removed from both prerender paths (
run-prerender.ts:190,prerender-paths.ts:380);buildIdreuse viareadBuiltBuildIdintact in both branches. No caller passes bothnextConfigandnextConfigOverride, so the two-field precedence is never ambiguous at runtime. - Deleted
--prerender-all/disk-export guard tests were replaced, not dropped: the old "does not load Vite config when …" throwing-config tests became positive "loads Vite config even when …" assertions, plus aloads Vite config once for all deploy metadataload-count test. The new eager load is documented at theloadDeployViteConfigMetadatacall site. Correctly codifies the new invariant.
Non-blocking observations (follow-up, not required)
-
EmitPrerenderPathManifestOptionslacks JSDoc onnextConfigvsnextConfigOverrideprecedence.RunPrerenderOptionsdocuments both fields and their merge semantics; the parallelprerender-paths.ts:38type does not, even though thenextConfig-wins-over-nextConfigOverridebehavior is identical. Worth a short comment for the next reader. -
cacheConfigguard is duplicated.loadDeployViteConfigMetadatacomputes it only whenviteConfigHasCacheAdapter(root), andpopulateKVCacheFromPrerenderedArtifactsre-checks the same predicate before using it. Harmless (and it keeps the consumer self-contained), but the two guards must stay in sync if either predicate changes.
Neither observation blocks merge. The change is well-scoped, cli/deploy parity holds, and the follow-up commits closed every substantive item from the earlier reviews.
|
Review posted to PR #2543. SummaryI independently reviewed the full diff through Verdict: no blocking issues. The fix correctly resolves inline Key correctness findings verified:
Two non-blocking follow-ups noted: The prior bonk reviews' substantive observations were all addressed in the follow-up commits; my review confirms the current state is correct and complete. |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: honor inline next config for static export
Independently reviewed the full diff through the latest commit 5ad66ff (refactor(deploy): use resolved cache metadata) — which all six prior bonk reviews predate (they stopped at f011d06). Ran the targeted suites (deploy-prerender-config, prerender-config, prerender-paths, plugin-options, cache-adapters-config — 58 passing; prerender — 91 passing) and ran vp check on all 9 changed source files (format/lint/type — clean).
Verdict: no blocking issues. The fix correctly resolves inline vinext({ nextConfig }) before the output-mode decision in both vinext build and vinext-cloudflare deploy, and threads the fully-resolved config into runPrerender / emitPrerenderPathManifest instead of reloading disk config.
The new commit 5ad66ff is safe
The commit does two things, both verified correct:
-
Removed the
viteConfigHasCacheAdapter(root)early-return frompopulateKVCacheFromPrerenderedArtifacts(deploy.ts:307). This is safe: the guard now lives solely inloadDeployViteConfigMetadata(deploy.ts:279), which setscacheConfig: nullwhen no adapter is detected. The consumer'sresolveKvDataAdapterConfig(null)returnsnull(deploy-config.ts:139-140), preserving the exact early-return behavior. Therootparam is still used bybuildPrerenderKVPairs, so no dead argument. The net effect is that the same text-scan guard is applied once (at metadata-load time) instead of twice. -
Added JSDoc to
EmitPrerenderPathManifestOptionsdistinguishingnextConfig(fully resolved, wins) fromnextConfigOverride(merged on disk config, ignored whennextConfigis supplied) — this closes the exact doc-parity follow-up flagged in the prior review.
Correctness checks that passed
- CLI reorder is safe.
loadBuildViteConfigMetadatamoved ahead of the output-mode decision (cli.ts:495); its only later consumer,emptyOutDiratcli.ts:578, still runs after the new call site, and the duplicate later call was removed — no doubleloadConfigFromFile. InlinenextConfignow correctly flows into__VINEXT_SHARED_BUILD_ID,outputMode, and the standalone/export branches. - Deploy env scoping preserved.
loadDeployViteConfigMetadataand the inline-config resolution are both wrapped inwithCloudflareEnv(buildEnv, ...)(deploy.ts:848,deploy.ts:851). The finder only extracts the raw function reference; the actual function evaluation happens inresolveNextConfigInputinside the secondwithCloudflareEnv, so function-form configs readingCLOUDFLARE_ENVresolve in the correct env (verified by thefunction-form inline config inside the selected Cloudflare environmenttest). - Resolve pipeline parity. Both
cli.ts:496-499anddeploy.ts:851-857feed inline config throughresolveNextConfigInput→resolveNextConfig, producing the sameResolvedNextConfigshape as the disk path.resolveNextConfigInputcorrectly does not treat adefaultkey specially (inline receives the value, not a module namespace). findVinextNextConfigInPluginsshort-circuits on the always-present__vinextNextConfig: options.nextConfig ?? null(if (nextConfig) return nextConfigskipsnull), so no false match when inline config is unset. Attached to thevinext:configplugin object (index.ts:1738).flattenPluginOptionsuses strictvalue instanceof Promise, so real VitePluginobjects are never mis-awaited; ordering preserved viaPromise.all(...).flat(). All four plugin finders are uniformly async — no sync/async asymmetry remains.- Redundant disk read removed from both prerender paths (
run-prerender.ts:190,prerender-paths.ts:385);buildIdreuse viareadBuiltBuildIdintact in both branches. No production caller passes bothnextConfigandnextConfigOverride, so the precedence is never ambiguous at runtime; legacy test paths that pass neither still fall back to disk load correctly.
Non-blocking observations (follow-up, not required for this PR)
-
Four
load*FromViteConfigwrappers are now dead code.loadVinextNextConfigFromViteConfig(next-config.ts:374),loadVinextCacheConfigFromViteConfig(cache-adapters-virtual.ts:77),loadVinextPrerenderConfigFromViteConfigandloadVinextRouteRootConfigFromViteConfig(prerender.ts) are no longer referenced by any production code or test — deploy now uses the batchedloadDeployViteConfigMetadata, and cli usesloadBuildViteConfigMetadata. They remain exported (and thus in theinternalsurface). Worth removing in a follow-up to avoid leaving three single-load helpers alongside the new consolidated loaders. -
cacheConfigguard is duplicated across load and consume.loadDeployViteConfigMetadatacomputescacheConfigonly whenviteConfigHasCacheAdapter(root), and after5ad66ffthe consumer relies entirely oncacheConfigbeingnullin that case. This is correct today, but the two must stay in sync — if a future change ever populatescacheConfigwithout the text-scan guard, the removed consumer-side guard means KV upload would run unconditionally. A one-line comment atpopulateKVCacheFromPrerenderedArtifactsnoting it now relies on the loader's guard would make the coupling explicit.
Neither observation blocks merge. The change is well-scoped, cli/deploy parity holds, and the latest commit 5ad66ff is a clean, correct simplification.
|
Review posted to PR #2543. SummaryI independently reviewed the full diff through the latest commit Verdict: no blocking issues. Key finding — the new commit
Correctness verified: CLI reorder is safe (inline config flows into build ID / output mode), deploy env scoping preserved for function-form config inside Two non-blocking follow-ups:
The prior reviews' substantive items were all addressed in follow-up commits; my review confirms the current state (including the previously-unreviewed |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: honor inline next config for static export
Independently reviewed the full diff through the latest commit 6aa8da3 (refactor(config): remove unused inline loader) — which all seven prior bonk reviews predate (they stopped at 5ad66ff). Ran the targeted suites (deploy-prerender-config, prerender-config, prerender-paths, plugin-options, cache-adapters-config — 58 passing; prerender — 91 passing) and ran vp check on all 9 changed source files (format/lint/type — clean).
Verdict: no blocking issues. The fix correctly resolves inline vinext({ nextConfig }) before the output-mode decision in both vinext build and vinext-cloudflare deploy, and threads the fully-resolved config into runPrerender / emitPrerenderPathManifest instead of reloading disk config.
The new commit 6aa8da3 is a clean cleanup
- Removed the now-dead
loadVinextNextConfigFromViteConfigwrapper (next-config.ts) and its privateViteConfigLoadertype. Verified safe: it had no remaining production caller or test — build usesloadBuildViteConfigMetadataand deploy usesloadDeployViteConfigMetadata, both of which callfindVinextNextConfigInPluginsdirectly. This closes half of the "four dead wrappers" follow-up from the prior review. - Added a coupling comment at
populateKVCacheFromPrerenderedArtifacts(deploy.ts:312) noting thatloadDeployViteConfigMetadatareturnsnullcacheConfig unless a cache adapter is declared — this makes the single-sidedviteConfigHasCacheAdapterguard explicit at the consumer, closing the exact coupling follow-up flagged in the prior review.
Correctness checks that passed
- CLI reorder is safe.
loadBuildViteConfigMetadataruns ahead of the output-mode decision (cli.ts:495); its only later consumers (emptyOutDir,outputMode,__VINEXT_SHARED_BUILD_ID, standalone/export branches) all read the now-inline-awareresolvedNextConfig. No duplicateloadConfigFromFile— the previous second call was removed. - Deploy env scoping preserved. Both
loadDeployViteConfigMetadata(line 849) and inline-config resolution (line 852) are wrapped inwithCloudflareEnv(buildEnv, ...). The finder extracts only the raw function reference; the function is actually invoked byresolveNextConfigInputinside the secondwithCloudflareEnv, so function-form configs readingCLOUDFLARE_ENVresolve in the correct env (verified by thefunction-form inline config inside the selected Cloudflare environmenttest).buildEnv(line 846) and the cache-upload env expression (line 910) evaluate identically, so the movedcacheConfigload keeps its original scoping. - Resolve-pipeline parity. Build (
cli.ts:496-499) and deploy (deploy.ts:852-857) both feed inline config throughresolveNextConfigInput→resolveNextConfig, producing the sameResolvedNextConfigas the disk path. Inline is treated as a full override (matching the plugin'sconfighook), not a merge — no accidental dev/build divergence.resolveNextConfigInputcorrectly does not special-case adefaultkey (inline receives the value, not a module namespace). findVinextNextConfigInPluginsshort-circuits on the always-present__vinextNextConfig: options.nextConfig ?? null(if (nextConfig) return nextConfigskipsnull), so no false match when inline config is unset. Attached to thevinext:configplugin object (index.ts:1738).flattenPluginOptionsuses strictvalue instanceof Promise, so real VitePluginobjects are never mis-awaited; ordering is preserved viaPromise.all(...).flat(). All four plugin finders are uniformly async — no sync/async asymmetry remains.- Redundant disk read removed from both prerender paths (
run-prerender.ts:190,prerender-paths.ts:385); each skipsloadNextConfigwhen the resolvednextConfigis supplied (both cli and deploy do), falling back to disk-load +nextConfigOverrideonly for the legacy test path.buildIdreuse viareadBuiltBuildIdis intact in both branches; the throwing-next.configtest proves the skip. No production caller passes bothnextConfigandnextConfigOverride, so the documented precedence is never ambiguous at runtime. - Deleted
--prerender-all/disk-export guard tests were replaced, not dropped — the throwing-config negatives became positive "loads Vite config even when…" assertions plus aloads Vite config once for all deploy metadataload-count test. The eager load is documented at theloadDeployViteConfigMetadatacall site (deploy.ts:847-848).
Non-blocking observations (follow-up, not required for this PR)
-
Three
load*FromViteConfigwrappers remain dead code.6aa8da3removedloadVinextNextConfigFromViteConfig, butloadVinextCacheConfigFromViteConfig(cache-adapters-virtual.ts:77),loadVinextPrerenderConfigFromViteConfigandloadVinextRouteRootConfigFromViteConfig(prerender.ts:103,115) have no remaining production caller or test (confirmed by grep — onlydist/type declarations reference them). Since one sibling was just removed for exactly this reason, the other three are worth removing in the same follow-up for consistency; leaving them creates the appearance that three single-load helpers still coexist with the consolidatedloadDeployViteConfigMetadata. -
Hybrid Pages Router extraction at
cli.ts:613is now the only sync flatten path. It was migrated to the promise-awareselectHybridPagesUserPluginsin an earlier commit, so this is already addressed — noting only that it and the config finders now shareutils/plugin-options.ts, so any future change to promise-resolution semantics lands in one place.
Both observations are cosmetic dead-code cleanup and do not affect behavior. The change is well-scoped, cli/deploy/disk parity holds, and the latest commit 6aa8da3 is a correct, low-risk cleanup that closed the coupling and one of the dead-wrapper follow-ups from the prior review.
|
Review posted to PR #2543. SummaryI independently reviewed the full diff through the latest commit Verdict: no blocking issues. The new commit
Correctness verified: CLI reorder is safe (inline config flows into build ID / output mode), deploy env scoping preserved for function-form config inside One non-blocking follow-up: three sibling The change is well-scoped and cli/deploy/disk parity holds. |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: honor inline next config for static export
Independently reviewed the full diff through the latest commit 08b979d (test(config): preserve internal metadata loaders) — which all eight prior bonk reviews predate (they stopped at 6aa8da3). Ran the targeted suites (deploy-prerender-config, prerender-config, prerender-paths, plugin-options, cache-adapters-config — 60 passing) and ran vp check on all 9 changed source files (format/lint/type — clean).
Verdict: no blocking issues. The fix correctly resolves inline vinext({ nextConfig }) before the output-mode decision in both vinext build and vinext-cloudflare deploy, and threads the fully-resolved config into runPrerender / emitPrerenderPathManifest instead of reloading disk config.
The new commit 08b979d is a clean test-only addition
The single previous review's outstanding follow-up — "three load*FromViteConfig wrappers are dead code" — is resolved by deciding to keep them as a tested internal surface rather than deleting them:
tests/cache-adapters-config.test.tsnow coversloadVinextCacheConfigFromViteConfigend-to-end with a promised plugin, andtests/prerender-config.test.tsnow coversloadVinextPrerenderConfigFromViteConfigandloadVinextRouteRootConfigFromViteConfigwith promised plugin composition.
Both tests assert the promise-aware path resolves correctly through the wrapper, which is a legitimate reason to keep the wrappers exported. No source change in this commit, so no new correctness risk.
Correctness checks that passed
- CLI reorder is safe.
loadBuildViteConfigMetadataruns ahead of the output-mode decision (cli.ts:495); inlinenextConfignow flows into__VINEXT_SHARED_BUILD_ID,outputMode(cli.ts:537), and the standalone/export branches. Its only later consumer,emptyOutDiratcli.ts:578, still runs after the new call site. The duplicate laterloadBuildViteConfigMetadatacall was removed — no doubleloadConfigFromFile. - Resolve-pipeline parity with the plugin runtime. Build (
cli.ts:496-499) and deploy (deploy.ts:852-857) feed inline config throughresolveNextConfigInput→resolveNextConfig, matching the plugin'sconfighook (index.ts:1856), which treats inlinenextConfigas a full override of disk config (with the drift warning), not a merge. No accidental dev/build divergence.resolveNextConfigInputcorrectly does not special-case adefaultkey (inline receives the value, not a module namespace —next-config.ts:931-933), and function-form inline configs resolve viaresolveConfigValuewith the samedefaultConfig: {}+PHASE_PRODUCTION_BUILDas disk function configs. - Deploy env scoping preserved. Both
loadDeployViteConfigMetadata(deploy.ts:849) and inline-config resolution (deploy.ts:852) are wrapped inwithCloudflareEnv(buildEnv, ...). The finder extracts only the raw function reference; the function is invoked byresolveNextConfigInputinside the secondwithCloudflareEnv, so function-form configs readingCLOUDFLARE_ENVresolve in the correct env (verified byresolves function-form inline config inside the selected Cloudflare environment→CLOUDFLARE_ENV === "preview"yieldsoutput: "export",buildId: "preview").buildEnv(deploy.ts:846) and the cache-upload env expression (deploy.ts:910) evaluate identically, so the movedcacheConfigload keeps its original scoping. - Cache guard coupling is correct.
viteConfigHasCacheAdapteris a pure text-scan of the vite config file (deploy-config.ts:97-111), so it is env-independent and safe to call insideloadDeployViteConfigMetadata. The single-sided guard (loader setscacheConfig: nullwhen no adapter) is safe:resolveKvDataAdapterConfig(null)returnsnull(deploy-config.ts:139-140), preserving the exact early-return the consumer previously performed. The coupling comment atdeploy.ts:312documents the invariant. findVinextNextConfigInPluginsshort-circuits on the always-present__vinextNextConfig: options.nextConfig ?? null(next-config.ts:364—if (nextConfig) return nextConfigskipsnull), so no false match when inline config is unset. The property is attached in the plugin object atindex.ts:1738.flattenPluginOptionsuses strictvalue instanceof Promise(utils/plugin-options.ts:2), so real VitePluginobjects (notPromiseinstances) are never mis-awaited; only genuinePromise<Plugin>entries resolve, and ordering is preserved viaPromise.all(...).flat(). All four plugin finders are uniformly async — no sync/async asymmetry remains. The hybrid Pages Router extraction now shares the same helper viaselectHybridPagesUserPlugins, preserving the framework-plugin exclusion filter verbatim.- Redundant disk read removed from both prerender paths.
run-prerender.ts:190andprerender-paths.ts:385skiploadNextConfigwhen a resolvednextConfigis supplied (which both cli and deploy do), falling back to disk-load +nextConfigOverrideonly for the legacy test path.buildIdreuse viareadBuiltBuildIdis intact in both branches (fresh object spread, then override). No production caller passes bothnextConfigandnextConfigOverride, so the documented precedence (nextConfigwins) is never ambiguous at runtime. - Both
runPrerendercall sites (cli + deploy) covered — no missed invocations.
Non-blocking observations (follow-up, not required for this PR)
- The
nextConfig-vs-nextConfigOverridetwo-field API exists only for the legacy test path.RunPrerenderOptionsandEmitPrerenderPathManifestOptionsboth carrynextConfig(production) andnextConfigOverride(tests, disk-merge). All production callers pass onlynextConfig; only tests passnextConfigOverride. This is documented via JSDoc, so it's fine, but a future cleanup could collapse the test path ontonextConfigto remove the dual-field surface entirely. - Two
--prerender-all/disk-export guard tests were replaced, not restored. The old throwing-vite.config.tsnegatives became positive "loads Vite config even when…" assertions plus theloads Vite config once for all deploy metadataload-count test. This correctly codifies the new eager-load invariant (a brokenvite.config.tsnow fails a--prerender-alldeploy that previously succeeded), which is inherent to the feature and documented atdeploy.ts:847-848. Flagging only that the behavioral change is intentional and captured.
The prior reviews' substantive items were all addressed across the follow-up commits, and the latest 08b979d closes the final dead-code follow-up by keeping and testing the internal loaders. The change is well-scoped, cli/deploy/disk parity holds, and I found no correctness issues.
|
Review posted to PR #2543. SummaryI independently reviewed the full diff through the latest commit Verdict: no blocking issues. The new commit Correctness verified:
Two non-blocking follow-ups noted: the The change is well-scoped and I found no correctness issues. |
2 participants
Summary
vinext({ nextConfig })through Vite plugin metadata for CLI/deploy config discoveryCLOUDFLARE_ENVRoot cause
vinext buildandvinext-cloudflare deploydecided whether to use static export fromnext.config.*before reading the Vite plugin's inlinenextConfig. Even after selecting prerendering, the prerender runner reloaded disk config, so inline-onlyoutput: "export"did not receive export output/error semantics.Validation
vp test run tests/deploy-prerender-config.test.ts tests/prerender-config.test.ts(23 tests)vp check packages/cloudflare/src/deploy.ts packages/vinext/src/cli.ts packages/vinext/src/config/next-config.ts packages/vinext/src/index.ts tests/deploy-prerender-config.test.ts tests/prerender-config.test.tsvp run @vinext/cloudflare#buildvp run vinext#buildnextConfig.output = "export"; confirmed export-mode log and HTML atdist/client/index.htmlanddist/client/old-school.htmlCloses #2542