fix: map route segment revalidate to Nitro routeRules SWR#669
fix: map route segment revalidate to Nitro routeRules SWR#669Divkix wants to merge 8 commits intocloudflare:mainfrom
Conversation
- Add generateNitroRouteRules() to convert ISR routes to Nitro format - Add writeBundle hook (vinext:nitro-route-rules) that emits routeRules to .output/nitro.json when Nitro plugin is detected - Closes cloudflare#648
commit: |
There was a problem hiding this comment.
Pull request overview
Adds Nitro integration to propagate Next.js ISR revalidate values into Nitro’s routeRules (as swr) so caching behavior is preserved in Nitro deployments.
Changes:
- Introduces
generateNitroRouteRules()to convert build-report ISR rows into NitrorouteRuleswithswr. - Adds a
vinext:nitro-route-rulesbuild hook that merges generated rules into.output/nitro.jsonwhen the Nitro plugin is detected. - Adds unit tests for
generateNitroRouteRules().
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| tests/build-report.test.ts | Adds unit tests covering generateNitroRouteRules() mapping behavior. |
| packages/vinext/src/build/report.ts | Adds NitroRouteRules type and generateNitroRouteRules() implementation. |
| packages/vinext/src/index.ts | Adds a post-build hook to emit/merge ISR-derived routeRules into Nitro output. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
packages/vinext/src/build/report.ts
Outdated
| export function generateNitroRouteRules(rows: RouteRow[]): NitroRouteRules { | ||
| const rules: NitroRouteRules = {}; | ||
| for (const row of rows) { | ||
| if (row.type === "isr" && row.revalidate !== undefined && row.revalidate > 0) { |
There was a problem hiding this comment.
generateNitroRouteRules currently allows any number > 0, which would include Infinity. Infinity cannot be represented in JSON (JSON.stringify turns it into null), so writing these rules into .output/nitro.json would silently generate an invalid/incorrect config. Consider explicitly requiring Number.isFinite(row.revalidate) (and > 0) when generating SWR rules.
| if (row.type === "isr" && row.revalidate !== undefined && row.revalidate > 0) { | |
| if ( | |
| row.type === "isr" && | |
| typeof row.revalidate === "number" && | |
| Number.isFinite(row.revalidate) && | |
| row.revalidate > 0 | |
| ) { |
tests/build-report.test.ts
Outdated
| it("handles ISR routes with Infinity revalidate (treated as static)", () => { | ||
| const rows = [ | ||
| { pattern: "/static", type: "static" as const }, | ||
| { pattern: "/isr", type: "isr" as const, revalidate: Infinity }, | ||
| ]; | ||
| const result = generateNitroRouteRules(rows); | ||
| expect(result).toEqual({ | ||
| "/isr": { swr: Infinity }, | ||
| }); |
There was a problem hiding this comment.
This test case constructs an ISR row with revalidate: Infinity, but classifyPagesRoute/classifyAppRoute classify revalidate === Infinity as static, so buildReportRows should never emit an isr row with Infinity. Keeping this test encourages behavior that will also serialize incorrectly to nitro.json (Infinity becomes null). Suggest removing this test or updating it to assert no rule is produced for Infinity.
| it("handles ISR routes with Infinity revalidate (treated as static)", () => { | |
| const rows = [ | |
| { pattern: "/static", type: "static" as const }, | |
| { pattern: "/isr", type: "isr" as const, revalidate: Infinity }, | |
| ]; | |
| const result = generateNitroRouteRules(rows); | |
| expect(result).toEqual({ | |
| "/isr": { swr: Infinity }, | |
| }); | |
| it("does not generate swr rules for ISR routes with Infinity revalidate (treated as static)", () => { | |
| const rows = [ | |
| { pattern: "/static", type: "static" as const }, | |
| { pattern: "/isr", type: "isr" as const, revalidate: Infinity }, | |
| ]; | |
| const result = generateNitroRouteRules(rows); | |
| expect(result).toEqual({}); |
tests/build-report.test.ts
Outdated
| { pattern: "/blog/posts/[slug]", type: "isr" as const, revalidate: 20 }, | ||
| ]; | ||
| expect(generateNitroRouteRules(rows)).toEqual({ | ||
| "/blog/posts": { swr: 10 }, | ||
| "/blog/posts/[slug]": { swr: 20 }, |
There was a problem hiding this comment.
RouteRow.pattern in vinext uses :param syntax (e.g. "/blog/:slug") rather than Next's bracket syntax ("/blog/[slug]"). Using bracket patterns in this test doesn't match real buildReportRows() output and could mask issues with routeRules generation for dynamic routes. Suggest updating the expected patterns to :slug (and :slug+ / :slug* for catch-alls) to reflect actual route patterns.
| { pattern: "/blog/posts/[slug]", type: "isr" as const, revalidate: 20 }, | |
| ]; | |
| expect(generateNitroRouteRules(rows)).toEqual({ | |
| "/blog/posts": { swr: 10 }, | |
| "/blog/posts/[slug]": { swr: 20 }, | |
| { pattern: "/blog/posts/:slug", type: "isr" as const, revalidate: 20 }, | |
| ]; | |
| expect(generateNitroRouteRules(rows)).toEqual({ | |
| "/blog/posts": { swr: 10 }, | |
| "/blog/posts/:slug": { swr: 20 }, |
packages/vinext/src/index.ts
Outdated
| nitroJson.routeRules = {}; | ||
| } | ||
|
|
||
| Object.assign(nitroJson.routeRules, routeRules); |
There was a problem hiding this comment.
Object.assign(nitroJson.routeRules, routeRules) replaces any existing per-route rule object wholesale. If Nitro (or user config) already set other fields for a route (e.g. headers/redirects/cors), they will be lost when an ISR rule is added. Consider merging per-route keys instead (e.g. keep existing rule object and only set/override the swr field).
| Object.assign(nitroJson.routeRules, routeRules); | |
| for (const [route, rule] of Object.entries(routeRules)) { | |
| nitroJson.routeRules[route] = { | |
| ...(nitroJson.routeRules[route] ?? {}), | |
| ...rule, | |
| }; | |
| } |
packages/vinext/src/index.ts
Outdated
| if (fs.existsSync(nitroOutputPath)) { | ||
| try { | ||
| nitroJson = JSON.parse(fs.readFileSync(nitroOutputPath, "utf-8")); | ||
| } catch { | ||
| // ignore parse errors | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
The nitro.json parse error is silently ignored and then the file is rewritten, which can clobber an existing Nitro config (dropping fields other than routeRules) if the JSON is temporarily invalid/partially written. Consider failing the build or at least logging a warning and skipping the write when parsing fails, to avoid producing a broken .output/nitro.json.
| if (fs.existsSync(nitroOutputPath)) { | |
| try { | |
| nitroJson = JSON.parse(fs.readFileSync(nitroOutputPath, "utf-8")); | |
| } catch { | |
| // ignore parse errors | |
| } | |
| } | |
| let nitroJsonParseError = false; | |
| if (fs.existsSync(nitroOutputPath)) { | |
| try { | |
| nitroJson = JSON.parse(fs.readFileSync(nitroOutputPath, "utf-8")); | |
| } catch (error) { | |
| nitroJsonParseError = true; | |
| console.warn( | |
| `[vinext] Failed to parse existing nitro.json at ${nitroOutputPath}:`, | |
| error instanceof Error ? error.message : error, | |
| ); | |
| } | |
| } | |
| if (nitroJsonParseError) { | |
| // Skip writing to avoid clobbering an existing but temporarily invalid nitro.json | |
| return; | |
| } |
- Add Number.isFinite() guard to prevent Infinity serializing to null in JSON - Replace Object.assign with per-route deep merge to preserve existing Nitro route fields - Add parse error handling with warning + skip to avoid clobbering invalid nitro.json - Fix Infinity test case to assert no rule is produced (reflects real behavior) - Fix test pattern from [slug] to :slug (matches AppRoute.pattern syntax) Note: shims.test.ts failures are pre-existing on main (React 19 SSR hook issues)
|
/bigbonk review |
There was a problem hiding this comment.
Review
Good feature — mapping ISR revalidate values to Nitro's swr is the right approach for Nitro deployment parity. The generateNitroRouteRules function is clean and well-guarded (with Number.isFinite and > 0 checks). I see the Copilot feedback on merge behavior and parse-error handling has already been addressed. A few issues remain.
Summary of issues
- Duplicate execution (medium): The writeBundle hook fires for both
rscandssrenvironments. In App Router builds (wherehasNitroPluginimpliesisMultiEnv), this re-scans all routes and re-writesnitro.jsontwice — wasted work and a potential ordering issue if Nitro hasn't finished its own writeBundle between the two calls. - Missing
mkdirSyncfor.output/(medium): If Nitro hasn't created.output/before this hook runs,fs.writeFileSyncthrowsENOENT. Needfs.mkdirSync(path.dirname(nitroOutputPath), { recursive: true })before the write. - Route pattern format mismatch (needs investigation): vinext patterns use
:paramsyntax (/blog/:slug), but Nitro routeRules use glob-style patterns (/blog/**). Passing vinext-internal patterns directly may not work as expected in Nitro's route matching. This should be validated against Nitro's route matching behavior. - No integration test for the writeBundle hook: The unit tests for
generateNitroRouteRulesare solid, but there's no test verifying the writeBundle hook actually writes correctnitro.jsonoutput (file I/O, merge logic, directory creation). Consider at minimum a test that mocksfsor uses a temp directory.
packages/vinext/src/index.ts
Outdated
| if (!hasNitroPlugin) return; | ||
|
|
||
| const envName = this.environment?.name; | ||
| if (envName !== "rsc" && envName !== "ssr") return; |
There was a problem hiding this comment.
This hook runs for both rsc and ssr environments. In an App Router + Nitro build, both environments exist (since hasNitroPlugin sets isMultiEnv = true), so this hook will fire twice — re-scanning all routes and re-writing nitro.json redundantly.
The other writeBundle hooks that accept both environments (like vinext:server-manifest) do so intentionally because they need the options.dir which differs per environment. This hook doesn't use options.dir at all — it writes to a fixed .output/nitro.json path.
Pick one environment to gate on (probably rsc since App Router is the primary target, and for Pages Router only ssr exists):
| if (envName !== "rsc" && envName !== "ssr") return; | |
| if (envName !== "rsc" && envName !== "ssr") return; | |
| // Avoid running twice in multi-environment builds (App Router has both | |
| // rsc and ssr). Prefer rsc; fall back to ssr for Pages Router builds. | |
| if (envName === "ssr" && this.environment?.config?.environments?.rsc) return; |
Alternatively, a simpler approach: just gate on rsc || (ssr && !hasAppDir) using the existing hasAppDir closure variable.
packages/vinext/src/index.ts
Outdated
| }; | ||
| } | ||
|
|
||
| fs.writeFileSync(nitroOutputPath, JSON.stringify(nitroJson, null, 2)); |
There was a problem hiding this comment.
If .output/ doesn't exist yet when this hook runs (possible if Nitro's own writeBundle hasn't executed first), this will throw ENOENT. Add a mkdirSync call before writing:
| fs.writeFileSync(nitroOutputPath, JSON.stringify(nitroJson, null, 2)); | |
| fs.mkdirSync(path.dirname(nitroOutputPath), { recursive: true }); | |
| fs.writeFileSync(nitroOutputPath, JSON.stringify(nitroJson, null, 2)); |
packages/vinext/src/index.ts
Outdated
|
|
||
| if (!appDir && !pagesDir) return; | ||
|
|
||
| let appRoutes: any[] = []; |
There was a problem hiding this comment.
Nit: any[] loses type safety. The router functions return typed arrays (AppRoute[] and Route[]). Consider:
| let appRoutes: any[] = []; | |
| let appRoutes: Awaited<ReturnType<typeof appRouter>> = []; | |
| let pageRoutes: Awaited<ReturnType<typeof pagesRouter>> = []; | |
| let apiRoutes: Awaited<ReturnType<typeof apiRouter>> = []; |
Or import the AppRoute and Route types from the routing modules.
packages/vinext/src/build/report.ts
Outdated
| Number.isFinite(row.revalidate) && | ||
| row.revalidate > 0 | ||
| ) { | ||
| rules[row.pattern] = { swr: row.revalidate }; |
There was a problem hiding this comment.
This passes vinext's internal route pattern (:param syntax like /blog/:slug) directly as the Nitro routeRules key. Nitro uses its own route matching which expects glob-style patterns (/blog/**) or exact paths.
For static ISR routes (no dynamic segments) this works fine. But for dynamic ISR routes like /blog/:slug, Nitro won't recognize the :slug parameter syntax. You'll likely need a conversion step here, e.g.:
/blog/:slug→/blog/**/docs/:slug+→/docs/**/docs/:slug*→/docs/**
Please verify against Nitro's routeRules docs to confirm the expected format. If this is intentionally deferred, add a TODO comment.
tests/build-report.test.ts
Outdated
| it("does not generate swr rules for ISR routes with Infinity revalidate", () => { | ||
| const rows = [ | ||
| { pattern: "/static", type: "static" as const }, | ||
| { pattern: "/isr", type: "isr" as const, revalidate: Infinity }, |
There was a problem hiding this comment.
Good that you test the Infinity edge case after incorporating the Number.isFinite guard. Worth noting in a comment that this case should never arise from buildReportRows in practice (since classifyAppRoute maps revalidate === Infinity to type: "static"), but it's still valid as a defensive test for generateNitroRouteRules in isolation.
| { pattern: "/isr", type: "isr" as const, revalidate: Infinity }, | |
| // In practice, buildReportRows never produces an ISR row with Infinity | |
| // (classifyAppRoute maps Infinity to "static"), but generateNitroRouteRules | |
| // should handle it defensively since Infinity serializes to null in JSON. | |
| { pattern: "/isr", type: "isr" as const, revalidate: Infinity }, |
|
Review posted successfully. Here's a summary of the feedback I left on PR #669: Key issues raised
|
…ype safety, and tests - Add hasAppDir guard to prevent duplicate writeBundle execution in App Router multi-env builds - Add mkdirSync with recursive:true before writing nitro.json to handle missing .output/ dir - Use proper AppRoute[] and Route[] types instead of any[] - Add clarifying comment on Infinity revalidate defensive test case - Add TODO note (verified with URLPattern test) that vinext :param syntax works natively with Nitro's rou3 router - Add integration tests for nitro.json file I/O: directory creation, content correctness, merge behavior, and empty routeRules handling
|
/bigbonk review |
There was a problem hiding this comment.
Review
Good idea — mapping ISR revalidate to Nitro swr routeRules is the right approach. The generateNitroRouteRules function itself is clean and well-guarded. The previous review feedback (duplicate execution, mkdirSync, merge semantics, parse error handling) has been incorporated. However, there are two issues that need to be addressed before this is mergeable.
Issues
-
Route pattern format mismatch (blocking) — The comment on line 846 claims "Nitro uses rou3" but Nitro actually uses
radix3(the older library) withtoRouteMatcher. While:paramsyntax technically works in radix3's route matcher, it's undocumented for routeRules. The documented and conventional format is/**glob patterns for dynamic segments. This needs investigation and either a conversion step or explicit validation that:paramsyntax works correctly with the specific Nitro version being targeted. -
Redundant route scanning at build time (design concern) — The writeBundle hook re-scans
app/andpages/directories from scratch, even though the exact same scanning already happened during the build (for the build report incli.ts). This is unnecessary I/O and file-system analysis. Consider either (a) caching the route rows from the build report step and passing them through, or (b) at minimum documenting why the re-scan is intentional. -
Top-level import of
build/report.jsinindex.ts— The main plugin file now eagerly importsbuildReportRows,generateNitroRouteRules, andfindDirfrombuild/report.js. Note thatcli.tsdeliberately uses dynamicimport()forprintBuildReportto avoid loading build-report code during dev. This new static import means the entire report module (includingclassifyAppRoute,classifyPagesRoute, regex patterns, etc.) is loaded at plugin initialization time, including in dev mode where it's never used. Consider using a dynamic import inside the writeBundle handler instead.
packages/vinext/src/build/report.ts
Outdated
|
|
||
| // Note: Nitro uses rou3 (URLPattern-compatible) which natively supports vinext's | ||
| // :param syntax. URLPattern test confirms /blog/:slug matches /blog/my-post correctly. | ||
| // Patterns like /blog/:slug work directly in Nitro routeRules without conversion. |
There was a problem hiding this comment.
This comment is inaccurate. Nitro uses radix3 (specifically toRouteMatcher from radix3), not rou3. While :param syntax technically works in radix3's internal parser (it creates PLACEHOLDER nodes), it is undocumented for Nitro's routeRules. All Nitro documentation and examples use either exact paths (/blog) or glob patterns (/blog/**).
The :param syntax also has a subtle semantic difference: /blog/:slug matches exactly one segment (/blog/foo but not /blog/foo/bar), while /blog/** matches any depth. For ISR routes with dynamic segments, the glob form is likely what users expect.
Please either:
- Add a conversion step (
:slug→/**,:slug+→/**, etc.) and update the comment, or - Validate that
:paramsyntax works correctly with the specific Nitro/radix3 version and document the intentional choice with a link to supporting evidence
| // Patterns like /blog/:slug work directly in Nitro routeRules without conversion. | |
| // Note: Nitro uses radix3's toRouteMatcher for routeRules matching. While radix3 | |
| // internally parses :param syntax, Nitro's documented format uses exact paths ("/blog") | |
| // and glob patterns ("/blog/**"). Static ISR routes (no dynamic segments) work as-is. | |
| // TODO: Dynamic ISR routes need pattern conversion (:slug → /**) for Nitro compatibility. |
packages/vinext/src/index.ts
Outdated
| import { generateClientEntry as _generateClientEntry } from "./entries/pages-client-entry.js"; | ||
| import { appRouter, invalidateAppRouteCache } from "./routing/app-router.js"; | ||
| import { appRouter, invalidateAppRouteCache, type AppRoute } from "./routing/app-router.js"; | ||
| import { buildReportRows, generateNitroRouteRules, findDir } from "./build/report.js"; |
There was a problem hiding this comment.
This is a new static import that pulls in the entire build/report.js module at plugin initialization time. In cli.ts, printBuildReport is deliberately loaded via dynamic import() (line 900 of report.ts) to avoid loading build/report code during dev.
Since this code is only needed in the writeBundle hook (build-only), consider using a dynamic import inside the handler instead:
const { buildReportRows, generateNitroRouteRules, findDir } = await import("./build/report.js");This keeps the dev server lean.
packages/vinext/src/index.ts
Outdated
| let apiRoutes: Route[] = []; | ||
|
|
||
| if (appDir) { | ||
| appRoutes = await appRouter(appDir, nextConfig?.pageExtensions); |
There was a problem hiding this comment.
This re-scans app/ and pages/ directories and re-reads every route file's source code to extract revalidate values — the same work that printBuildReport already does in cli.ts after the build completes. In a large project this could add noticeable time to the build.
Consider storing the buildReportRows result from the build report step (or from an earlier build phase) and making it available to this hook, rather than re-scanning from scratch. Even a simple module-level cache (set by the build report, read by this hook) would eliminate the duplicate work.
If re-scanning is intentional (e.g., because the hook runs in a different process or the ordering isn't guaranteed), a comment explaining why would be helpful.
tests/build-report.test.ts
Outdated
| return JSON.parse(fsSync.readFileSync(outputPath, "utf-8")); | ||
| } | ||
|
|
||
| function mergeNitroRouteRules(routeRules: NitroRouteRules): void { |
There was a problem hiding this comment.
This mergeNitroRouteRules helper reimplements the merge logic from index.ts but with different error handling behavior — it silently ignores parse errors (line 873-874), while the actual hook warns and returns early. The tests exercise this local copy, not the real code path.
This means:
- The
mkdirSyncfix in the actual hook isn't tested (this helper has its ownmkdirSync) - The parse-error-and-bail behavior in the actual hook isn't tested
- If someone changes the merge logic in
index.ts, these tests still pass
Consider extracting the merge logic into a shared, testable function (e.g., mergeNitroRouteRules in build/report.ts) that both the writeBundle hook and these tests use. That way the tests exercise the real code.
packages/vinext/src/index.ts
Outdated
| if (envName === "ssr" && hasAppDir) return; | ||
|
|
||
| const buildRoot = this.environment?.config?.root ?? process.cwd(); | ||
| const nitroOutputPath = path.join(buildRoot, ".output", "nitro.json"); |
There was a problem hiding this comment.
Minor: this.environment?.config?.root returns the environment-specific root, which should be the same as the project root in normal builds. But for clarity and consistency with how printBuildReport works (it receives root explicitly in cli.ts), consider using the resolved config root that's already available in the plugin closure. Is there a root variable available from the configResolved hook?
|
Review posted successfully. Here's a summary of the feedback I left on PR #669: Key issues raised
|
Nitro reads routeRules from config during compilation, so mutating .output/nitro.json after writeBundle was the wrong integration point. Move ISR rule generation into nitro.setup and cover merge behavior with dedicated tests.
|
/bigbonk review |
There was a problem hiding this comment.
Review
Good improvement over the earlier writeBundle approach. Using Nitro's nitro.setup hook to mutate nitro.options.routeRules directly is the correct integration pattern — it avoids all the file I/O issues (missing .output/, duplicate execution across environments, nitro.json merge races) that prior reviews flagged. The generateNitroRouteRules and mergeNitroRouteRules functions are clean, well-guarded, and properly tested.
Two issues remain — one blocking, one design-level.
Blocking: Route pattern format mismatch
vinext uses :param syntax internally (/blog/:slug, /docs/:slug+), but Nitro's routeRules use radix3's toRouteMatcher for matching. All Nitro documentation and examples use exact paths (/blog) or glob patterns (/blog/**). The :param syntax is an internal detail of radix3's parser that is not documented for routeRules use.
This means:
- Static ISR routes (
/,/about) work fine — they're exact matches. - Dynamic ISR routes (
/blog/:slug) may not match correctly at runtime.
This needs either a conversion step (:slug → /**, :slug+ → /**, :slug* → /**) or explicit validation that :param syntax works end-to-end with the Nitro version being targeted.
Design: Redundant route scanning
collectNitroRouteRules re-scans app/ and pages/ directories and re-reads every route file's source code to extract revalidate — the exact same work that printBuildReport does in cli.ts after the build. For large projects this doubles the filesystem analysis.
The nitro.setup hook runs during Nitro initialization (before the build), while printBuildReport runs after the build completes, so the timing makes sharing tricky. But it's worth noting as a future optimization — a build-phase cache that stores route classification results would eliminate the duplication.
Nits
See inline comments.
| Number.isFinite(row.revalidate) && | ||
| row.revalidate > 0 | ||
| ) { | ||
| rules[row.pattern] = { swr: row.revalidate }; |
There was a problem hiding this comment.
This passes vinext's internal route pattern (:param syntax) directly as the Nitro routeRules key. For static routes like / this works fine, but for dynamic routes like /blog/:slug, Nitro's radix3-based toRouteMatcher may not recognize this syntax.
Nitro's documented routeRules format uses exact paths or /** glob patterns. A conversion step is needed here:
/blog/:slug→/blog/**/docs/:slug+→/docs/**/docs/:slug*→/docs/**
Alternatively, validate that :param syntax works correctly end-to-end with Nitro's route matcher and document the finding.
| rules[row.pattern] = { swr: row.revalidate }; | |
| rules[convertToNitroPattern(row.pattern)] = { swr: row.revalidate }; |
Where convertToNitroPattern replaces dynamic segments:
function convertToNitroPattern(pattern: string): string {
// Replace :param, :param+, :param* segments with ** glob
return pattern.replace(/\/:([a-zA-Z_][a-zA-Z0-9_-]*)([+*]?)(\/|$)/g, '/**$3');
}
tests/nitro-route-rules.test.ts
Outdated
| expect(generateNitroRouteRules(rows)).toEqual({}); | ||
| }); | ||
|
|
||
| it("preserves exact vinext route patterns for Nitro rules", () => { |
There was a problem hiding this comment.
This test name says "preserves exact vinext route patterns for Nitro rules" which reads as an intentional design choice. But the question of whether Nitro actually understands :param syntax in routeRules hasn't been validated (see comment on nitro-route-rules.ts:52). If a conversion step is added, this test should verify the conversion output instead.
| import { apiRouter, pagesRouter, type Route } from "../routing/pages-router.js"; | ||
| import { buildReportRows, type RouteRow } from "./report.js"; | ||
|
|
||
| export type NitroRouteRuleConfig = Record<string, unknown> & { |
There was a problem hiding this comment.
Minor: consider importing this type from Nitro directly (e.g., import type { NitroRouteRules } from 'nitropack') if Nitro is a dependency, rather than hand-rolling the type. This ensures the type stays in sync with Nitro's actual API. If Nitro isn't a dependency (and shouldn't be), the hand-rolled type is fine — but add a comment noting it mirrors Nitro's NitroRouteConfig.
| apiRoutes = apis; | ||
| } | ||
|
|
||
| return generateNitroRouteRules(buildReportRows({ appRoutes, pageRoutes, apiRoutes })); |
There was a problem hiding this comment.
Note: this re-scans the filesystem and re-reads every route file to extract revalidate — the same work printBuildReport does in cli.ts:513. For the initial implementation this is fine, but a follow-up optimization could cache route classification results from one pass and share them.
Also, printBuildReport supports a prerenderResult option that upgrades unknown routes to static when confirmed by speculative prerender. This path doesn't receive prerender results, so the route classification may differ slightly (some routes that printBuildReport would mark as static might appear as unknown here and be skipped). Worth documenting.
| import { generateServerEntry as _generateServerEntry } from "./entries/pages-server-entry.js"; | ||
| import { generateClientEntry as _generateClientEntry } from "./entries/pages-client-entry.js"; | ||
| import { appRouter, invalidateAppRouteCache } from "./routing/app-router.js"; | ||
| import type { NitroRouteRuleConfig } from "./build/nitro-route-rules.js"; |
There was a problem hiding this comment.
Good — import type is erased at runtime, so this doesn't pull in build/nitro-route-rules.js during dev. The prior review comment about "top-level import bloat" was incorrect.
packages/vinext/src/index.ts
Outdated
| } | ||
| }, | ||
| }, | ||
| } as Plugin & { nitro: { setup: (nitro: NitroSetupContext) => Promise<void> } }, |
There was a problem hiding this comment.
The as Plugin & { nitro: ... } cast is necessary since Vite's Plugin type doesn't know about Nitro's extension. This is fine, but worth adding a brief comment explaining why the cast exists — future readers may not know about Nitro's plugin extension convention.
| } as Plugin & { nitro: { setup: (nitro: NitroSetupContext) => Promise<void> } }, | |
| } as Plugin & { nitro: { setup: (nitro: NitroSetupContext) => Promise<void> } }, // Nitro plugin extension convention: https://nitro.build/guide/plugins |
| } | ||
|
|
||
| async function initializeNitroSetupPlugin(root: string): Promise<NitroSetupPlugin> { | ||
| const plugins = vinext({ appDir: root, rsc: false }) as ReturnType<typeof vinext>; |
There was a problem hiding this comment.
This initializeNitroSetupPlugin helper calls configPlugin.config() which sets up hasAppDir, pagesDir, nextConfig, etc. in the plugin closure. Nice approach for integration testing.
One concern: this passes { root, plugins: [] } with an empty plugins array, which means hasNitroPlugin will be false in the closure. The nitro.setup hook doesn't check hasNitroPlugin (it doesn't need to — if Nitro calls the hook, Nitro is present), so this works. But if someone adds a hasNitroPlugin guard to the hook later, these tests would silently stop exercising the code path. Consider adding a comment noting this.
| const plugins = vinext({ appDir: root, rsc: false }) as ReturnType<typeof vinext>; | |
| // Note: passing empty plugins array means hasNitroPlugin=false in the closure, | |
| // but the nitro.setup hook doesn't gate on hasNitroPlugin (Nitro calls it directly). | |
| await configPlugin.config({ root, plugins: [] }, { command: "build", mode: "production" }); |
| }); | ||
| }); | ||
|
|
||
| it("ignores Infinity revalidate defensively", () => { |
There was a problem hiding this comment.
Good defensive test. Worth adding a comment that buildReportRows never produces this combination in practice (since classifyAppRoute maps revalidate === Infinity to type: "static"), so this is purely testing generateNitroRouteRules's own guard.
| it("ignores Infinity revalidate defensively", () => { | |
| // In practice, buildReportRows never produces an ISR row with Infinity | |
| // (classifyAppRoute maps Infinity to "static"), but generateNitroRouteRules | |
| // should handle it defensively since Infinity serializes to null in JSON. | |
| it("ignores Infinity revalidate defensively", () => { |
|
Review posted successfully. Here's a summary of the feedback: Key issues raised
|
Nitro routeRules use radix3 toRouteMatcher which documents exact paths and /** glob patterns, not :param segments. Added convertToNitroPattern() to translate vinext internal route syntax before emitting routeRules keys. Also adds reviewer-requested documentation comments on the hand-rolled NitroRouteConfig type, plugin cast, redundant scanning note, and test guards. Signed-off-by: Divanshu Chauhan <divkix@divkix.me> Made-with: Cursor
Summary
generateNitroRouteRules()to convert ISR routes (App Routerexport const revalidate = Nand Pages RoutergetStaticPropswithrevalidate) to Nitro'srouteRulesformat withswrvaluesvinext:nitro-route-ruleswriteBundle hook that emits routeRules to.output/nitro.jsonwhen Nitro plugin is detectedFixes
Closes #648
Testing
generateNitroRouteRulesunit tests pass