Repository navigation
feat(build): support prerender vite config - #2415
Conversation
|
/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: feat(build): support prerender vite config
Reviewed the full diff, ran the targeted test suites (prerender-config, deploy-prerender-config, cli-args, deploy — all green), and vp check on every changed file (clean). The implementation is solid and follows the repo's layering guidance well.
What's good
- Clean extraction. Prerender decision logic lives in a typed
config/prerender.tsmodule rather than inline in the generated/CLI code, consistent with AGENTS.md ("move real behavior into normal typed modules").resolveVinextPrerenderDecisionandformatVinextPrerenderLabelare pure and unit-tested. - Dev/deploy parity. Both
cli.ts buildApp()anddeploy.tsroute through the sameresolveVinextPrerenderDecision+formatVinextPrerenderLabelhelpers, so the build and deploy paths can't drift. TheenablePrerenderSourceMapshandling matches in both. - Priority is correct and explicit.
--prerender-all>output: 'export'>vinext({ prerender }), matching the PR description and the prior||behavior, now with distinct labels per trigger. - Validation throws on bad input (
false, non-"*"routes) with actionable messages, andundefined/nullare treated as disabled with no false positives infindVinextPrerenderConfigInPlugins(thenullproperty value is falsy and skipped). flattenPluginOptionscorrectly handles vinext'sPluginOption[]return and nested arrays.
Minor observations (non-blocking)
-
CLI
buildwiring lacks an integration test. The deploy path getstests/deploy-prerender-config.test.ts, but there's no equivalent end-to-end test exercisingloadBuildViteConfigMetadata→resolveVinextPrerenderDecisioninsidecli.ts buildApp(). The unit pieces are covered, but the build-command wiring (the analog of what the deploy test verifies) is only covered transitively. Consider a focused test mirroring the deploy one for thevinext buildpath. -
Non-standard plugin property. Attaching
__vinextPrerenderConfigto thevinext:configplugin object and reading it back viafindVinextPrerenderConfigInPluginsrelies on Vite preserving unknown plugin keys (it does today). This is a reasonable, low-cost channel, but it's an implicit contract worth a one-line comment near the property definition inindex.tsso a future reader knows why a Vite plugin carries a custom field. -
Deploy loads the Vite config an extra time. When
!skipBuild,runBuildinstantiates the config and then the prerender step callsloadVinextPrerenderConfigFromViteConfigagain (instantiating plugins a second time). Dynamicimport()of Vite is cached so the cost is small, but instantiating the user's plugin array twice could surprise plugins with construction-time side effects. Not a regression introduced by correctness concerns — just noting it.
Overall: LGTM. No correctness or parity issues found; the items above are coverage/clarity nits.
|
Review posted to PR #2415. SummaryI reviewed PR #2415 ( The implementation is solid. No correctness, parity, or security issues found. Highlights:
Minor, non-blocking observations I noted in the review:
All three are coverage/clarity nits, not defects. |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: feat(build): support prerender vite config
Reviewed the full diff (16 files), ran the targeted suites (prerender-config, deploy-prerender-config, init-platform, init, init-cloudflare — 181 tests, all green), and ran vp check on all 7 changed source files (clean: format + lint + types). The implementation is solid and follows the repo's layering and parity guidance.
What's good
- Clean extraction per AGENTS.md. Decision logic lives in a typed
config/prerender.tsmodule with pure, unit-tested helpers (normalizeVinextPrerenderConfig,resolveVinextPrerenderDecision,formatVinextPrerenderLabel,findVinextPrerenderConfigInPlugins) rather than inline in the CLI/deploy code. - Build/deploy parity. Both
cli.ts buildApp()anddeploy.tsstep 6a route through the sameresolveVinextPrerenderDecision+formatVinextPrerenderLabelhelpers, and both gateenablePrerenderSourceMapsidentically, so the two prerender entry points can't drift. - Priority ordering is correct and explicit.
--prerender-all>output: 'export'>vinext({ prerender }), matching the prior||behavior with distinct labels per trigger. - Validation surfaces early.
normalizeVinextPrerenderConfigis called at the top ofvinext(), so invalid config (false, non-"*"routes) throws at config-load time with actionable messages;undefined/nullare correctly treated as disabled with no false positives. - End-to-end coverage exists for the build path. The
ppr-impact-demofixture and Playwright project were migrated from--prerender-alltovinext({ prerender: { routes: "*" } })+ plainvinext build, so theloadBuildViteConfigMetadata→resolveVinextPrerenderDecisionwiring is exercised end-to-end by E2E, not just transitively. The deploy path additionally gets a focused integration test. - No arg collision.
parsePrerenderArguses exact match for--prerenderandstartsWith("--prerender="), so it won't accidentally match the build/deploy--prerender-allflag (which lives on a different command anyway). - AST
initediting reuses the established pattern.hasVinextPrerender/ensureVinextPrerendermirror the existing cache/image-optimizer helpers (including the SpreadElement guard and the empty-args vs object-args branches), and throw a clear error when plugin options aren't a static object.
Minor observations (non-blocking)
-
Deploy loads the user's Vite config an extra time. In
deploy.tsstep 6a,loadVinextPrerenderConfigFromViteConfigcallsloadConfigFromFileunconditionally — even when--prerender-allis already set (where the result is discarded) and even when!skipBuild(whererunBuildalready instantiated the config). Dynamicimport()of Vite is cached so the cost is small, but instantiating the user's plugin array a second time could surprise plugins with construction-time side effects. Cheap win: short-circuit the config load whenoptions.prerenderAllis already true, since the flag wins regardless. -
__vinextPrerenderConfigis an implicit cross-process contract. The normalized config is smuggled fromindex.tsonto thevinext:configplugin object and read back viafindVinextPrerenderConfigInPluginsafter a freshloadConfigFromFile. This relies on Vite preserving unknown plugin keys (true today) and is a reasonable low-cost channel — worth a one-line comment near the property definition inindex.tsandVINEXT_PRERENDER_CONFIG_PLUGIN_PROPERTYso a future reader knows why a Vite plugin carries a custom field.
Both are clarity/efficiency nits, not defects.
Verdict: LGTM. No correctness, parity, or security issues found.
|
Review posted to PR #2415. SummaryI reviewed PR #2415 ( The implementation is solid — no correctness, parity, or security issues found. Key strengths:
Two non-blocking observations I flagged:
Both are clarity/efficiency nits, not defects. Verdict: LGTM. |
|
/bigbonk review for issues |
commit: |
|
There was a problem hiding this comment.
Review: feat(build): support prerender vite config
Reviewed the full diff (16 files). Ran the targeted suites — prerender-config, deploy-prerender-config, init-platform, init, init-cloudflare (182 tests, all green) — and vp check on all 7 changed source files (clean: format + lint + types).
The implementation is solid: clean extraction into a typed, unit-tested config/prerender.ts; both build (cli.ts) and deploy (deploy.ts) route through the same resolveVinextPrerenderDecision + formatVinextPrerenderLabel helpers so they can't drift; priority ordering (--prerender-all > output: 'export' > vinext({ prerender })) is correct; validation throws early with actionable messages; null config is correctly skipped in findVinextPrerenderConfigInPlugins; the init AST helpers mirror the existing cache/image patterns; and no --prerender / --prerender-all arg collision (exact-match switch in cli-args.ts, exact + --prerender= match in parsePrerenderArg).
One finding worth addressing before merge
deploy.ts step 6a now loads the Vite config unconditionally, which is a behavioral change for the --prerender-all deploy path — not just an efficiency nit.
On main, step 6a only read next.config and never touched the Vite config:
if (options.prerenderAll || isStaticExport) { ... }After this PR, the path always calls loadVinextPrerenderConfigFromViteConfig → loadConfigFromFile:
const vite = await loadProjectViteApi(info.root);
const vinextPrerenderConfig = await withCloudflareEnv(buildEnv, () =>
loadVinextPrerenderConfigFromViteConfig(vite, info.root),
);
const prerenderDecision = resolveVinextPrerenderDecision({
prerenderAllFlag: options.prerenderAll,
vinextPrerenderConfig,
nextOutput: nextConfig.output,
});Because prerenderAllFlag wins inside resolveVinextPrerenderDecision regardless of vinextPrerenderConfig, the config load is pure overhead when --prerender-all is set — but it now also runs in cases where it never ran before:
vinext deploy --prerender-all --skip-build: previously no Vite config was loaded at any point in deploy; now it is. IfloadConfigFromFilethrows (malformed config, a plugin with a construction-time side effect that fails, etc.), a deploy that previously succeeded now fails. This is a new failure mode, not just extra work.- Even in the
!skipBuildcase,runBuildalready instantiated the user's plugin array viacreateBuilder; step 6a now instantiates it a second time, which can surprise plugins with construction-time side effects.
The deploy tests added here only cover the config-triggered path (vinext({ prerender })), so the --prerender-all case where the new load is unnecessary isn't exercised.
Suggested fix: short-circuit the config load when the flag already wins, since it takes priority regardless:
const vinextPrerenderConfig = options.prerenderAll
? null
: await withCloudflareEnv(buildEnv, () =>
loadVinextPrerenderConfigFromViteConfig(vite, info.root),
);(Also lets you skip loadProjectViteApi entirely in the flag case.) This restores prior behavior for --prerender-all and removes the redundant instantiation, while keeping the config-triggered path intact.
Minor (non-blocking)
__vinextPrerenderConfigis an implicit cross-load contract — the normalized config is attached to thevinext:configplugin object inindex.tsand read back viafindVinextPrerenderConfigInPluginsafter a freshloadConfigFromFile. This relies on Vite preserving unknown plugin keys (true today). A one-line comment near the property definition inindex.tsandVINEXT_PRERENDER_CONFIG_PLUGIN_PROPERTYwould help a future reader.
Parity note (not a defect)
Both prerender entry points (cli.ts buildApp() and deploy.ts step 6a) are covered and share the same helpers, including enablePrerenderSourceMaps. The Node platform's init path intentionally only writes prerender into newly-generated configs (existing configs are skipped when !force), matching its prior behavior — no AST update path exists there, which is consistent, not a regression.
Verdict: LGTM once the --prerender-all config-load short-circuit is addressed (or explicitly accepted as intended).
|
Review posted to PR #2415. SummaryReviewed the full diff (16 files), ran the targeted suites ( One finding worth addressing before merge:
Since Minor: Everything else (parity, validation, arg collisions, Node init behavior) checks out. Verdict: LGTM once the |
Performance benchmarksCompared 0 improved · 1 regressed · 5 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
|
/bigbonk review for issues |
Review: feat(build): support prerender vite configRe-reviewed the full diff at The prior finding is fully addressedMy previous review flagged that const shouldLoadVinextPrerenderConfig = !options.prerenderAll && nextConfig.output !== "export";
const vinextPrerenderConfig = shouldLoadVinextPrerenderConfig
? await withCloudflareEnv(buildEnv, async () => {
const vite = await loadProjectViteApi(info.root);
return loadVinextPrerenderConfigFromViteConfig(vite, info.root);
})
: null;This restores prior behavior: when The What's good
Parity note (not a defect)The Node Verdict: LGTM. No correctness, parity, or security issues found. The one prior blocking finding is resolved and covered by tests. |
|
/bigbonk review for issues |
|
Review posted to PR #2415. SummaryI re-reviewed PR #2415 ( Verdict: LGTM — no issues found. The one blocking finding from the prior review round is now resolved. Key points:
|
|
/bigbonk review for issues |
Review: feat(build): support prerender vite configRe-reviewed the full diff at Verdict: LGTM — no correctness, parity, or security issues found. The one prior blocking finding is fully resolved and tested, and the changes not covered by earlier rounds check out. Prior blocking finding is resolved and coveredThe earlier review flagged that New since the last review round: adapter warning changes (commit
|
|
Review posted to PR #2415. SummaryI re-reviewed PR #2415 ( Verdict: LGTM — no correctness, parity, or security issues found. Key points:
No new issues introduced; everything checks out. |
Summary
vinext({ prerender: true })andvinext({ prerender: { routes: "*" } })as build/deploy prerender triggers--prerender-allsupported and highest priorityValidation
vp check packages/vinext/src/config/prerender.ts packages/vinext/src/index.ts packages/vinext/src/cli.ts packages/vinext/src/deploy.ts tests/prerender-config.test.ts tests/deploy-prerender-config.test.tsvp test run tests/prerender-config.test.ts tests/deploy-prerender-config.test.ts tests/cli-args.test.ts tests/deploy.test.tsReview