feat: static pre-rendering at build time#274
feat: static pre-rendering at build time#274NathanDrake2406 wants to merge 13 commits intocloudflare:mainfrom
Conversation
commit: |
Add runStaticExport() orchestrator that connects the existing staticExportPages/staticExportApp functions to the CLI build pipeline. When next.config has output: 'export', vinext build now: 1. Runs the Vite build 2. Starts a temporary dev server 3. Renders all static pages to out/ 4. Prints a summary Also adds 'vinext start' rejection for export builds, matching Next.js behavior (ported from test/e2e/app-dir-export/test/start.test.ts). Phase 1 of cloudflare#9
Add Phase 2 orchestrator that pre-renders static pages after a production build: 1. Starts a temporary prod server in-process 2. Detects static routes via Vite dev server module inspection 3. Fetches each static route and writes HTML to dist/server/pages/ For Pages Router: skips pages with getServerSideProps, expands dynamic routes via getStaticPaths (fallback: false only). For App Router: skips force-dynamic pages, expands dynamic routes via generateStaticParams with parent param resolution. Phase 2 of cloudflare#9
Add resolvePrerenderedHtml() helper and pre-render check to both App Router and Pages Router production servers. Pre-rendered files in dist/server/pages/ are served directly with text/html before falling back to SSR. Includes path traversal protection and supports both /page.html and /page/index.html patterns for trailingSlash compatibility. Phase 2 of cloudflare#9
After a non-export build completes, automatically pre-render static pages to dist/server/pages/. The prod server serves these directly without SSR on first request. Phase 2 of cloudflare#9
1. Remove App Router pre-rendered HTML shortcut — was bypassing RSC streaming and middleware/auth pipeline 2. Move Pages Router pre-rendered check after middleware/basePath/ redirects/rewrites pipeline (step 7b instead of 1b) 3. Skip ISR pages (revalidate != false) in collectStaticRoutes() to prevent freezing dynamic content as static HTML 4. basePath handling covered by fix cloudflare#2 (uses resolvedPathname) 5. Temp Vite servers now check for project vite.config and use it when present, so user plugins/aliases are available 6. vinext start guard now checks config.output directly instead of relying on out/ directory existence heuristic
… to warning Dynamic routes without generateStaticParams (App Router) or getStaticPaths (Pages Router) in output: 'export' mode now produce a warning and skip the route instead of failing the build. This enables legitimate use cases like CMS apps with no published content and SPA-style client-rendered dynamic routes. Addresses vercel/next.js#61213 and vercel/next.js#55393.
Must-fixes: - Update stale file header comments (warning, not error) - Sanitize getOutputPath() against path traversal from user params - Fix revalidate filtering: only skip revalidate=0 (force-dynamic), not all ISR pages — they should be pre-rendered - Fix clearTimeout leak on fetch failure in prerenderStaticPages - Consume response body on non-ok fetch in staticExportApp - Fix revalidate:0 handling in Pages Router collectStaticRoutes (0 is falsy, so the old check didn't catch it) Should-fixes: - Wrap renderStaticPage in try/finally to clear SSR context on error - Extract createTempViteServer() helper (was duplicated 2x) - Extract expandDynamicAppRoute() helper (was duplicated with diverging behavior between staticExportApp and collectStaticRoutes) - Remove unnecessary server.listen() in collectStaticRoutes — only ssrLoadModule is needed, not an HTTP listener - Add Pages Router test for warning on missing getStaticPaths
NathanDrake2406
force-pushed
the
feat/static-prerender-build
branch
from
cd25163 to
ce830ad
Compare
- collectStaticRoutes now returns { urls, skipped } instead of just
URLs — every skipped route includes a reason (force-dynamic,
revalidate: 0, load failure, getServerSideProps, etc.)
- Skipped routes surface in PrerenderResult.skipped for CLI output
- Type createTempViteServer config as vite.InlineConfig instead of
Record<string, unknown>
- Skip App Router builds in prerenderStaticPages() — pre-rendered HTML
files were written to dist/server/pages/ but never served (the App
Router prod server delegates entirely to the RSC handler)
- Fix getStaticProps({}) probe to pass { params: {} } — prevents crash
when getStaticProps destructures context.params
- Replace sync readFileSync with async fs.promises.readFile in the
per-request pre-rendered HTML handler
- Move pagesPrerenderedDir computation and directory existence check to
server startup (was recomputed on every request)
- Remove stale TDD comments claiming functions don't exist yet, fix
JSDoc describing nonexistent subprocess fallback
Keep createTempViteServer (from this PR) and adopt upstream's ValidFileMatcher-based findFileWithExtensions signature. Fix prerendered page headers type to match widened middlewareHeaders.
|
/bigbonk review please |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Static pre-rendering at build time
Solid feature implementation overall. The architecture is well-structured: runStaticExport() as the orchestrator for output: "export", prerenderStaticPages() for selective pre-rendering in normal builds, and the prod server integration to serve pre-rendered HTML. The refactoring of renderStaticPage to use try/finally for SSR context cleanup and the extraction of expandDynamicAppRoute are good improvements.
I found one security issue that should be fixed, plus several correctness and robustness concerns.
Security
resolvePrerenderedHtmlhas a path prefix confusion bug —startsWith(resolvedDir)without a trailing separator means a directory namedpages-eviladjacent topageswould pass the check (see inline comment).
Correctness concerns
- The
getOutputPathtraversal guard is dead code —path.posix.normalizeresolves..segments, so theincludes('..')check after normalization can never be true. collectStaticRoutescallsgetStaticProps({ params: {} })on every non-dynamic page during route collection, purely to checkrevalidate. This has side effects (database queries, API calls) and runs with empty params which may not match the page's expectations.prerenderStaticPagesspins up two servers (a Vite dev server for route collection + a prod server for fetching). The dev server usesssrLoadModuleagainst source files while the prod server uses the built output — if there's any mismatch between source and built state, the route list will be wrong.
Test quality
- Tests are well-structured but the prerender tests depend on
hasServerEntry(a pre-existing build artifact). If the fixture hasn't been built, all 5 prod server tests silently skip viait.skipIf. This makes CI green even when the feature is untestable.
See inline comments for specific suggestions.
| const resolvedDir = path.resolve(dir); | ||
|
|
||
| const directPath = path.join(dir, `${normalized}.html`); | ||
| if (path.resolve(directPath).startsWith(resolvedDir) && fs.existsSync(directPath) && fs.statSync(directPath).isFile()) { |
There was a problem hiding this comment.
Bug (security): The startsWith check is vulnerable to prefix confusion. If the pre-rendered directory is /dist/server/pages, then a resolved path like /dist/server/pages-evil/hack.html will pass the startsWith check because the string /dist/server/pages-evil/... starts with /dist/server/pages.
The fix is to append path.sep (or /) to the resolved directory before the prefix check:
| if (path.resolve(directPath).startsWith(resolvedDir) && fs.existsSync(directPath) && fs.statSync(directPath).isFile()) { | |
| if (path.resolve(directPath).startsWith(resolvedDir + path.sep) && fs.existsSync(directPath) && fs.statSync(directPath).isFile()) { |
Same issue on line 306. In practice this is low-risk since the pathname comes from the URL which is already normalized upstream, but it's a defense-in-depth fix that should be applied.
| } | ||
|
|
||
| const indexPath = path.join(dir, normalized, "index.html"); | ||
| if (path.resolve(indexPath).startsWith(resolvedDir) && fs.existsSync(indexPath) && fs.statSync(indexPath).isFile()) { |
There was a problem hiding this comment.
Same prefix confusion bug as line 301:
| if (path.resolve(indexPath).startsWith(resolvedDir) && fs.existsSync(indexPath) && fs.statSync(indexPath).isFile()) { | |
| if (path.resolve(indexPath).startsWith(resolvedDir + path.sep) && fs.existsSync(indexPath) && fs.statSync(indexPath).isFile()) { |
| const normalized = path.posix.normalize(urlPath); | ||
| if (normalized.includes("..")) { | ||
| throw new Error(`Route path "${urlPath}" contains path traversal segments`); |
There was a problem hiding this comment.
This traversal guard is dead code. path.posix.normalize resolves .. segments, so after normalization, normalized will never contain ... For example, /foo/../../etc/passwd normalizes to /etc/passwd.
Either:
- Check before normalizing:
if (urlPath.includes('..'))— but this is overly broad and rejects legitimate paths. - Check the resolved output path against the
outDirboundary (which is the correct approach, but would need theoutDirpassed in). - Remove the dead check and add the boundary check at the write site in the callers.
Since the urlPath values come from route patterns and user-defined getStaticPaths/generateStaticParams, option 2 (checking fullPath.startsWith(resolvedOutDir + path.sep) at the write site) would be the most robust defense.
| if (typeof pageModule.getStaticProps === "function") { | ||
| try { | ||
| const propsResult = await pageModule.getStaticProps({ params: {} }); |
There was a problem hiding this comment.
Calling getStaticProps({ params: {} }) on every non-dynamic page during route collection is a side-effect concern. This function may make database queries, API calls, or other I/O — and it's being called with empty params purely to inspect the revalidate value. For dynamic routes with required params, this will likely throw (which the catch handles, but it means those routes are incorrectly skipped).
Consider a lighter-weight approach:
- Check for
export const revalidate = 0as a module-level export (similar to howdynamicis checked on line 1085) instead of executing the function. - Or skip the
getStaticPropsrevalidate check entirely for route collection — onlygetServerSidePropsandforce-dynamicare definitive signals that a page can't be pre-rendered. Pages withrevalidate: 0ingetStaticPropsare relatively rare.
|
|
||
| // Collect static routes using a temporary Vite dev server | ||
| const collected = await collectStaticRoutes(root, false); | ||
| result.skipped.push(...collected.skipped); | ||
|
|
||
| if (collected.urls.length === 0) { | ||
| result.warnings.push("No static routes found — nothing to pre-render"); | ||
| return result; | ||
| } | ||
|
|
||
| const staticUrls = collected.urls; | ||
|
|
||
| // Start temp production server in-process | ||
| const { startProdServer } = await import("../server/prod-server.js"); | ||
| const server = await startProdServer({ | ||
| port: 0, // Random available port | ||
| host: "127.0.0.1", | ||
| outDir: distDir, | ||
| }); | ||
| const addr = server.address() as import("node:net").AddressInfo; | ||
| const port = addr.port; |
There was a problem hiding this comment.
This function spins up two servers: a Vite dev server (via collectStaticRoutes on line 982) for route discovery, plus a production server (line 994) for HTML fetching. This is:
- Slow — two full server startups during build.
- Fragile — the dev server inspects source files while the prod server uses built output. If the source and build are out of sync (e.g., user edited a file after build), the route list won't match reality.
A more robust approach would be to either:
- Use only the prod server for both route detection and fetching (inspect the built server entry's route table), or
- Embed the list of static routes into the build output during the Vite build step so it's available without spinning up a dev server.
This doesn't block merging but is worth tracking as a follow-up for build performance.
| // ── Static export (output: "export") ────────────────────────── | ||
| const { loadNextConfig, resolveNextConfig } = await import( | ||
| /* @vite-ignore */ "./config/next-config.js" | ||
| ); | ||
| const rawConfig = await loadNextConfig(process.cwd()); | ||
| const resolvedConfig = await resolveNextConfig(rawConfig); |
There was a problem hiding this comment.
The config is loaded and resolved here, but runStaticExport (line 287) also loads and resolves the config internally. This means the config is parsed twice. Consider either:
- Passing the already-resolved config to
runStaticExport, or - Having the CLI only check
resolvedConfig.outputand letting the orchestrator handle everything else.
Also, if the config file has side effects or is expensive to load, double-loading could be a problem.
packages/vinext/src/cli.ts
Outdated
| const { loadNextConfig, resolveNextConfig } = await import( | ||
| /* @vite-ignore */ "./config/next-config.js" | ||
| ); | ||
| const startRawConfig = await loadNextConfig(process.cwd()); | ||
| const startResolvedConfig = await resolveNextConfig(startRawConfig); |
There was a problem hiding this comment.
Same double-loading issue as in buildApp(). The config is loaded here to check output: "export", then startProdServer will load its own config from the build output. Not a correctness issue but wasteful.
Also, the variable names startRawConfig / startResolvedConfig are a bit unusual. Since this is in its own function scope, rawConfig / resolvedConfig would be fine (no collision).
tests/build-prerender.test.ts
Outdated
| const serverEntryPath = path.join(outDir, "server", "entry.js"); | ||
| const pagesDir = path.join(outDir, "server", "pages"); | ||
| const prerenderedFile = path.join(pagesDir, "prerendered-test.html"); | ||
| const hasServerEntry = fs.existsSync(serverEntryPath); |
There was a problem hiding this comment.
The hasServerEntry check runs at module evaluation time, before any test setup. If the fixture hasn't been built (no dist/server/entry.js), all 5 prod-server tests silently skip. This means a CI configuration that doesn't pre-build fixtures will show green despite exercising nothing.
Consider either:
- Adding a build step in
beforeAllif the entry doesn't exist, or - Failing explicitly with a clear message like
throw new Error('Fixture not built')instead of skipping, so it's visible when the test infrastructure is broken.
| * `vinext build` to dist/server/pages/. | ||
| */ | ||
| function resolvePrerenderedHtml(dir: string, pathname: string): string | null { | ||
| if (!fs.existsSync(dir)) return null; |
There was a problem hiding this comment.
Nit: This fs.existsSync(dir) check runs on every call to resolvePrerenderedHtml, which itself is called on every request (via line 987). Since hasPrerenderedPages is already checked at line 986 before calling this function, this inner check is redundant. It won't cause a bug but it's an unnecessary filesystem syscall per request.
| let paramSets: Record<string, string | string[]>[]; | ||
| if (parentParamSets.length > 0) { | ||
| paramSets = []; | ||
| for (const parentParams of parentParamSets) { | ||
| const childResults = await generateStaticParams({ params: parentParams }); | ||
| if (Array.isArray(childResults)) { | ||
| for (const childParams of childResults) { | ||
| paramSets.push({ ...parentParams, ...childParams }); | ||
| } | ||
| } | ||
| } | ||
| } else { | ||
| paramSets = await generateStaticParams({ params: {} }); |
There was a problem hiding this comment.
If generateStaticParams throws (e.g., because a required API/database is unavailable at build time), the error propagates unhandled to the caller. In staticExportApp the caller catches this (line 766), but in collectStaticRoutes (line 1101) there's also a catch. However, in expandDynamicAppRoute itself there's no error context added, so the error message won't mention which route failed. Consider wrapping with route context:
try {
paramSets = await generateStaticParams({ params: {} });
} catch (e) {
throw new Error(`generateStaticParams() failed for ${route.pattern}: ${(e as Error).message}`);
}
|
The review has been submitted successfully. Here's a summary of the feedback I posted: Key findings:
|
Add path.sep suffix to startsWith checks to prevent prefix confusion (e.g. /dist/server/pages-evil/hack.html passing a check for /dist/server/pages). Also remove redundant fs.existsSync check since the caller already gates on hasPrerenderedPages at startup.
- Replace dead path traversal guard in getOutputPath with output
directory boundary check (validates resolved path stays within outDir)
- Eliminate dev server from prerenderStaticPages by using source-file
inspection for route classification instead of ssrLoadModule
- Fix createTempViteServer to always use configFile: false, preventing
RSC plugin resolution failures when loaded from user's vite.config
- Replace getStaticProps({ params: {} }) side effect with module-level
revalidate export check during route collection
- Add route context to generateStaticParams error messages
- Add port 0 check after Vite dev server bind
- Pass resolved config from CLI to runStaticExport to avoid double
config loading
- Rename start() config variables for clarity
Replace silent test skipping (it.skipIf) with an explicit error in beforeAll when the fixture's server entry doesn't exist. This prevents CI from showing green when the test infrastructure is broken.
|
Thanks for submitting this! Im going to take a little extra actual human time with this one just to be sure its the approach we want to go with and has no future potential conflict with TPR. Will try and tackle it this weekend |
2 participants
Summary
Implements build-time static pre-rendering for vinext, closing #9.
vinext buildwithoutput: "export"now runs full static export toout/(Phase 1)vinext buildin normal mode pre-renders static pages todist/server/pages/(Phase 2)vinext startrejects export builds with a helpful error messagePhase 1:
output: "export"supportAdds
runStaticExport()orchestrator that connects the existingstaticExportPages/staticExportAppfunctions to the CLI build pipeline. Loads config, detects router type, starts a temp dev server, scans routes, renders all static pages toout/.Phase 2: Selective pre-rendering in normal builds
Adds
prerenderStaticPages()that runs after production builds:getServerSideProps,force-dynamic, etc.)dist/server/pages/Files changed
packages/vinext/src/build/static-export.tsrunStaticExport(),prerenderStaticPages(),collectStaticRoutes()packages/vinext/src/cli.tsbuildApp(), export build rejection instart()packages/vinext/src/server/prod-server.tsresolvePrerenderedHtml()+ pre-render serving in both router handlerstests/build-static-export.test.tsrunStaticExport()(both routers)tests/build-prerender.test.tsprerenderStaticPages()Test plan
pnpm run typecheckpassespnpm run lintpassesCloses #9