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: |
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):
|
…#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
from
dda07d1 to
e7a7c9a
Compare
|
/bigbonk review |
There was a problem hiding this comment.
Thorough review of the prerender usability enhancements. The feature set is well-scoped and the test coverage is strong. I have a few substantive concerns and several minor observations.
Key concerns:
- The
detectsDynamicApiUsagenaming is inconsistent with the rest of the codebase (descriptive vs imperative) PrerenderProgresshas duplicated counter state that will be a maintenance trapcompileRouteGlobhas a potentially surprising**behavior where/api/**/usersdoesn't match/api/users- The
onProgresserror handler can flood stderr - The multi-line block comment false positive should be documented as a known limitation in the source
Looks good:
- Dynamic API detection regex work is solid —
import typeand inlinetypemodifiers are correctly handled - Route filtering with compiled regex caching is clean
- Prerender status priority logic (rendered > skipped/error) is correct for the multi-path dynamic route case
- The dry-run mode is a useful addition
- Test coverage is comprehensive (44 new tests covering edge cases like aliased imports, mixed type modifiers, multi-line imports)
| * - 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.
Agreeing with the previous review: detectsDynamicApiUsage reads as a descriptor ("this function detects...") rather than an imperative. Every other function in this file uses imperative naming: extractExportConstString, hasNamedExport, classifyAppRoute.
| export function detectsDynamicApiUsage(code: string): boolean { | |
| export function detectDynamicApiUsage(code: string): boolean { |
This is exported and referenced in tests, so the rename touches tests/build-report.test.ts too. Worth doing now before external consumers depend on the name.
| process.stderr.write(`\r${padded}`); | ||
| } | ||
|
|
||
| finish(rendered: number, skipped: number, errors: number): void { |
There was a problem hiding this comment.
finish() accepts its own rendered/skipped/errors parameters computed by the caller (lines 465-471), while update() independently tracks the same counters in this.rendered/this.skipped/this.errors (lines 66-68). The internal counters are only used for the live progress bar display.
This dual-tracking is a latent bug waiting to happen — a future maintainer could reasonably switch finish() to use this.rendered etc. (since the fields exist), and the counts would silently diverge if any update() call arrives without a status.
I'd recommend one of:
- Make
finish()use the internal counters and drop the parameters - Remove the internal counters entirely and have the caller pass the running counts into
update()
Option 1 seems cleanest since finish() is always called after all update() calls.
| .replace(/[.+?^${}()|[\]\\]/g, "\\$&") | ||
| .replace(/\*\*/g, DOUBLE_STAR) // placeholder for ** | ||
| .replace(/\*/g, "[^/]*") // * matches within segment | ||
| .replace(new RegExp(DOUBLE_STAR, "g"), ".*"); // ** matches across segments |
There was a problem hiding this comment.
Note: ** in a mid-path position like /api/**/users does NOT match /api/users (zero intermediate segments). This is because ** compiles to .* but the surrounding / separators are literal, so the compiled regex is ^\/api\/.*\/users$ which requires at least one / between api and users.
This may surprise users expecting shell-glob semantics where ** can match zero segments. Minimatch and other glob libraries typically handle this. Consider documenting this limitation in the JSDoc for compileRouteGlob, or adjusting the compilation so that /**/ generates (?:/.*)?/ to allow zero-segment matching.
| 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.
Routes classified as unknown get action = "render" here, but the dry-run output shows a plain → render which is indistinguishable from confidently-static routes. For static routes, rendering is expected to succeed; for unknown routes, it's speculative and may fail.
Consider distinguishing this in the output:
| const action = type === "api" || type === "ssr" ? "skip" : "render"; | |
| const action = type === "api" || type === "ssr" ? "skip" : "render"; | |
| const speculative = type === "unknown" ? " (speculative)" : ""; | |
| console.log(` ${sym} ${route.pattern} → ${action}${speculative}`); |
| try { | ||
| options.onProgress?.(update); | ||
| } catch (err: unknown) { | ||
| process.stderr.write( | ||
| `[vinext] onProgress callback error: ${err instanceof Error ? err.message : String(err)}\n`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
If onProgress is consistently broken (e.g., a TypeError from bad destructuring), this writes an error message to stderr on every single route. For a project with hundreds of routes, that's hundreds of identical error lines flooding the output.
Consider a log-once pattern:
| try { | |
| options.onProgress?.(update); | |
| } catch (err: unknown) { | |
| process.stderr.write( | |
| `[vinext] onProgress callback error: ${err instanceof Error ? err.message : String(err)}\n`, | |
| ); | |
| } | |
| try { | |
| options.onProgress?.(update); | |
| } catch (err: unknown) { | |
| if (!this._onProgressErrorLogged) { | |
| this._onProgressErrorLogged = true; | |
| process.stderr.write( | |
| `[vinext] onProgress callback error (further errors suppressed): ${err instanceof Error ? err.message : String(err)}\n`, | |
| ); | |
| } | |
| } |
(The same pattern applies to the identical block in the Pages Router phase at line 440-446.)
Note: this would need a flag on the runPrerender scope, not on PrerenderProgress, since the error handling is in the closure.
| // Anchored to line start (^) to avoid matching commented-out imports. | ||
| // Uses multiline flag so ^ matches each line in multi-line source. | ||
| // Negative lookbehind (?<!\btype\s) skips inline type modifiers like |
There was a problem hiding this comment.
The comment says the ^ anchor avoids matching commented-out imports, but this only works for single-line // comments. As confirmed by testing, a multi-line block comment where import starts on its own line is a false positive:
const x = 1; /*
import { headers } from "next/headers";
*/This is acceptable for a heuristic, but the comment should document the limitation explicitly so future readers know:
| // Anchored to line start (^) to avoid matching commented-out imports. | |
| // Uses multiline flag so ^ matches each line in multi-line source. | |
| // Negative lookbehind (?<!\btype\s) skips inline type modifiers like | |
| // Anchored to line start (^) to avoid matching commented-out imports. | |
| // Uses multiline flag so ^ matches each line in multi-line source. | |
| // Limitation: multi-line block comments where `import` starts on a new line | |
| // will produce false positives — this is a known tradeoff of regex-based analysis. | |
| // Negative lookbehind (?<!\btype\s) skips inline type modifiers like |
| } else if (!prerenderStatusMap.has(r.route)) { | ||
| // Only set skipped/error if not already rendered (a dynamic route may | ||
| // have some rendered paths and some errors). | ||
| prerenderStatusMap.set(r.route, r.status); | ||
| } |
There was a problem hiding this comment.
The status priority logic is: rendered always wins (overwrites any prior status), then first-seen non-rendered status sticks. This means if the results array has [{ route: "/x", status: "skipped" }, { route: "/x", status: "error" }], the route gets skipped — the error is silently hidden.
In practice a single route probably can't be both skipped and errored, but the defensive option would be to give error priority over skipped:
| } else if (!prerenderStatusMap.has(r.route)) { | |
| // Only set skipped/error if not already rendered (a dynamic route may | |
| // have some rendered paths and some errors). | |
| prerenderStatusMap.set(r.route, r.status); | |
| } | |
| } else if (!prerenderStatusMap.has(r.route)) { | |
| prerenderStatusMap.set(r.route, r.status); | |
| } else if (r.status === "error" && prerenderStatusMap.get(r.route) === "skipped") { | |
| // Error takes priority over skipped for the same route. | |
| prerenderStatusMap.set(r.route, r.status); | |
| } |
| export { stripServerExports as _stripServerExports }; | ||
| export { asyncHooksStubPlugin as _asyncHooksStubPlugin }; | ||
|
|
||
| // Public prerender API |
There was a problem hiding this comment.
These are exported without the _ prefix used by the internal exports above, signaling stable public API. Once external consumers depend on runPrerender, compileRouteGlob, matchRouteGlob, and PrerenderProgress, changing their signatures is a breaking change.
Is this intentional? If there's any uncertainty about the API shape (e.g., whether compileRouteGlob should be public or just matchRouteGlob), consider _ prefixing them for now. The types (RunPrerenderOptions, PrerenderResult, etc.) are fine since they're narrower contracts.
Specifically, PrerenderProgress as a class export is a strong commitment — consumers could subclass it. If the intent is just to let external code observe progress, the onProgress callback is sufficient and PrerenderProgress could remain internal.
| expect(detectsDynamicApiUsage("")).toBe(false); | ||
| }); | ||
|
|
||
| // ── Edge cases ────────────────────────────────────────────────────────────── | ||
|
|
||
| it("ignores commented-out imports", () => { | ||
| const code = `// import { headers } from "next/headers";\nexport default function Page() {}`; |
There was a problem hiding this comment.
Good test for single-line /* ... */ block comments. However, the multi-line block comment case that produces a false positive (confirmed by testing) is not covered:
const x = 1; /*
import { headers } from "next/headers";
*/Worth adding as a documented known-limitation test:
| expect(detectsDynamicApiUsage("")).toBe(false); | |
| }); | |
| // ── Edge cases ────────────────────────────────────────────────────────────── | |
| it("ignores commented-out imports", () => { | |
| const code = `// import { headers } from "next/headers";\nexport default function Page() {}`; | |
| it("ignores block-commented imports", () => { | |
| const code = `/* import { headers } from "next/headers"; */\nexport default function Page() {}`; | |
| // Block comments don't start with `import` at line start, so the ^ anchor skips them | |
| expect(detectsDynamicApiUsage(code)).toBe(false); | |
| }); | |
| it("false positive: multi-line block comment with import on new line (known limitation)", () => { | |
| // When a block comment spans multiple lines and `import` starts at the beginning | |
| // of a new line, the regex cannot distinguish it from real code. This is a known | |
| // tradeoff of regex-based analysis — acceptable for a build report heuristic. | |
| const code = `const x = 1; /*\nimport { headers } from "next/headers";\n*/`; | |
| expect(detectsDynamicApiUsage(code)).toBe(true); | |
| }); |
|
Review submitted successfully. Here's a summary of the feedback posted on PR #732: 9 inline comments covering:
|
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)