feat(prerender): usability enhancements for pre-rendering (#565)#732
feat(prerender): usability enhancements for pre-rendering (#565)#732raed04 wants to merge 1 commit intocloudflare:mainfrom
Conversation
commit: |
…#565) - Improve progress bar: phase labels (App/Pages Router), elapsed time, render rate, real-time status breakdown (rendered/skipped/errors) - Improve route classification: detect dynamic API imports (headers, cookies, draftMode, connection, unstable_noStore) via static analysis to classify routes as dynamic instead of unknown - Add prerender annotations to build report: [prerendered], [skipped], [error] suffixes on routes, prerender summary section - Expose public API: runPrerender, PrerenderProgress, compileRouteGlob, matchRouteGlob, and associated types exported from package entry - Add route filtering: includeRoutes/excludeRoutes glob patterns with compiled regex caching via createRouteFilter - Add dry-run mode: scan and classify routes without rendering - Add external onProgress callback for programmatic progress tracking
raed04
force-pushed
the
feat/prerender-usability-565
branch
2 times, most recently
from
6d4ba1b to
dda07d1
Compare
Added: KV cache seeding at deploy time (closes #562)This commit adds automatic KV cache population during How it works
Key details
Security
Tests27 new unit tests covering key format, binary RSC, TTL clamping, unicode paths, batch splitting, API errors, and mixed manifests. All 213 tests pass. Files
|
raed04
force-pushed
the
feat/prerender-usability-565
branch
from
b6f5aee to
dda07d1
Compare
|
/bigbonk review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Solid PR — the feature set is coherent, the test coverage is thorough (44 new tests), and the implementation choices are sensible. A few things worth addressing before merge.
Summary of findings:
- One false-positive edge case in the dynamic API regex (multi-line block comments)
- Naming nit:
detectsDynamicApiUsagereads as a description, not a function name - The
PrerenderProgressclass tracks internal counters that duplicate the caller's counters — minor but confusing - Dry-run mode doesn't classify
unknownroutes distinctly in its output (intentional? worth a note) - The public API exports look good and the types are well-chosen
|
|
||
| /** Detects `import { headers | cookies | draftMode } from "next/headers"` */ | ||
| const DYNAMIC_HEADERS_RE = | ||
| /^\s*import\s*\{[^}]*(?<!\btype\s)\b(?:headers|cookies|draftMode)\b[^}]*\}\s*from\s*['"]next\/headers['"]/m; |
There was a problem hiding this comment.
The ^ anchor with /m correctly skips // import { headers } ... (single-line comments) because the // precedes import. However, it produces false positives for imports inside multi-line block comments that start on a prior line:
const x = 1; /*
import { headers } from "next/headers";
*/Here import starts at the beginning of a line (matching ^ in multiline mode), but the code is dead.
This is an inherent limitation of regex-based analysis and the JSDoc already documents the heuristic nature, so it's fine as-is for a build report. But worth adding to the "Limitations" comment block so future readers know.
| * - Does not detect `type` imports (which are erased at runtime — correct behavior) | ||
| * - Anchored to line start to avoid false positives from commented-out imports | ||
| */ | ||
| export function detectsDynamicApiUsage(code: string): boolean { |
There was a problem hiding this comment.
Naming nit: detectsDynamicApiUsage reads like a property/descriptor ("this function detects...") rather than an imperative. The rest of the codebase uses imperative names (extractExportConstString, hasNamedExport, classifyAppRoute). Consider detectDynamicApiUsage or hasDynamicApiImport for consistency.
Not a blocker since this is exported and already referenced in tests, but worth considering if you're touching it again.
| route: string, | ||
| status?: "rendered" | "skipped" | "error", | ||
| ): void { | ||
| if (status === "rendered") this.rendered++; |
There was a problem hiding this comment.
PrerenderProgress tracks rendered, skipped, and errors internally via update(), but finish() ignores these and takes its own rendered/skipped/errors parameters computed by the caller from allRoutes. The internal counters are only used for the live progress bar display.
This works correctly, but the duplication is a subtle maintenance trap — if someone later changes finish() to use the internal counters (since they exist), the counts could diverge (e.g., if update() is called without a status, the internal counters undercount).
Consider either:
- Making
finish()use the internal counters and drop the parameters, or - Removing the internal counters and passing
statusbreakdown toupdate()externally
Not blocking, but worth a note.
| for (const route of routes) { | ||
| const { type } = classifyAppRoute(route.pagePath, route.routePath, route.isDynamic); | ||
| const sym = SYMBOLS[type] ?? "?"; | ||
| const action = type === "api" || type === "ssr" ? "skip" : "render"; |
There was a problem hiding this comment.
In dry-run mode, routes classified as unknown get action = "render", which matches the actual prerender behavior (speculative rendering). But the output → render might be confusing to users — the route might fail at render time since it couldn't be statically classified.
Consider distinguishing this in the dry-run output, e.g., → render (speculative) or using the ? symbol's meaning to hint that the outcome is uncertain.
| export function compileRouteGlob(pattern: string): RegExp { | ||
| // Use a Unicode placeholder that won't appear in route patterns to | ||
| // distinguish ** from * during the replacement chain. | ||
| const DOUBLE_STAR = "\uFFFF"; |
There was a problem hiding this comment.
Minor: \uFFFF is a valid (though rare) Unicode character. If a route pattern ever contained it, this would produce incorrect regex output. A two-character sequence like \uFFFE\uFFFF or a longer sentinel would be safer, though in practice route patterns are ASCII so this is purely theoretical.
The approach is clean and the comment explains the rationale well.
| completedUrls += 1; | ||
| progress.update(completedUrls, totalUrls, route); | ||
| progress.update(completedUrls, totalUrls, route, status); | ||
| try { |
There was a problem hiding this comment.
Good defensive error handling on the onProgress callback. One thought: the error is written to process.stderr which is fine, but in a non-TTY context (CI), this message could be interleaved with other output. Consider logging it once and then suppressing subsequent callback errors for the same route (or using a flag to log once total) to avoid flooding stderr if the callback is consistently broken.
| export { stripServerExports as _stripServerExports }; | ||
| export { asyncHooksStubPlugin as _asyncHooksStubPlugin }; | ||
|
|
||
| // Public prerender API |
There was a problem hiding this comment.
The existing exports above this block use underscore-prefixed names (_parseStaticObjectLiteral, _stripServerExports) to signal internal/unstable APIs. The new prerender exports are un-prefixed, signaling they're public API.
This is intentional per the PR description — just confirming: are these APIs considered stable? Once external consumers depend on runPrerender, compileRouteGlob, etc., changing the signatures is a breaking change. If there's any uncertainty, consider _runPrerender to give room for iteration.
| }); | ||
|
|
||
| // ── Edge cases ────────────────────────────────────────────────────────────── | ||
|
|
There was a problem hiding this comment.
Good test. One subtle gap: the // comment test passes because // import doesn't start with import after ^. But this test doesn't cover the false positive case of multi-line block comments:
// This would be a false positive:
const x = 1; /*
import { headers } from "next/headers";
*/Worth adding as a documented known-limitation test with a comment explaining the expected true result and why it's acceptable.
| paths.push(r.path); | ||
| prerenderPathsMap.set(r.route, paths); | ||
| } | ||
| } else if (!prerenderStatusMap.has(r.route)) { |
There was a problem hiding this comment.
The priority logic — "rendered" wins over "skipped"/"error" for the same route — is smart for dynamic routes with multiple paths where some succeed and some fail. But the else if (!prerenderStatusMap.has(r.route)) guard means the first non-rendered status wins among skipped/error. If a route has both skipped and error results, whichever appears first in the array determines the status.
Is that intentional? It seems unlikely in practice (a route is either skipped or errored, not both), but if it can happen, you might want error to take priority over skipped.
|
Review submitted successfully. Here's a summary of the feedback posted on PR #732: Key findings (9 inline comments):
|
2 participants
Summary
Closes #565 — Pre-rendering usability enhancements.
headers,cookies,draftModefromnext/headers;connectionfromnext/server;unstable_noStore/noStorefromnext/cache) via static analysis — routes importing these are classified asssrinstead ofunknown[prerendered],[prerendered: N paths],[skipped], or[error]annotations; prerender summary section addedrunPrerender,PrerenderProgress,compileRouteGlob,matchRouteGloband types exported from package entry pointincludeRoutes/excludeRoutesglob patterns (*and**) with compiled regex cachingonProgressoption for programmatic tracking alongside built-in TTY progress barFiles changed
packages/vinext/src/build/report.tsSYMBOLSexportpackages/vinext/src/build/run-prerender.tspackages/vinext/src/index.tstests/build-report.test.tsTest plan
classifyAppRoutebehavioral change documented: routes importing dynamic APIs now returnssrinstead ofunknown(more accurate, errs toward restrictive)