Add native App Router route type generation#1144
Conversation
commit: |
JamesbbBriz
force-pushed
the
feature/native-route-typegen
branch
from
80ea35a to
92c2c5f
Compare
|
This is ready for maintainer review now that checks have passed. The implementation intentionally follows the native generation direction rather than delegating to |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 92c2c5fa33
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6d40deda6e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a053daa927
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f359f5f0fc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e0b73ef8b3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: de4bcab749
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const layoutRoute = treePathToRouteLiteral(slot.ownerTreePath); | ||
| const slots = model.layoutSlots.get(layoutRoute) ?? []; |
There was a problem hiding this comment.
layoutSlots is keyed by treePathToRouteLiteral(slot.ownerTreePath), which strips route-group segments. In apps with multiple root layouts (e.g. app/(shop)/layout and app/(marketing)/layout), both owners collapse to "/", so their slot names are merged into one LayoutProps<'/'> signature. That makes helper types inaccurate (slots from one root layout appear on another), which is exactly where developers rely on these generated types for correctness.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6821754dee
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const layoutRoute = | ||
| layoutRouteKeys.get(slot.ownerTreePath) ?? treePathToRouteLiteral(slot.ownerTreePath); |
There was a problem hiding this comment.
LayoutSlotMap is keyed by LayoutRoute, but this loop keys slots by slot.ownerTreePath ("/foo" in app/foo/@slot/*) rather than the slot’s ownerLayoutId. When a slot is defined under a segment that does not have its own layout.tsx, the manifest can still assign it to an ancestor layout via ownerLayoutId; with the current code, the slot is written under a non-layout key and then dropped from the emitted LayoutSlotMap, so LayoutProps<'/...'> misses that slot prop. Use ownerLayoutId (resolved back to the layout route key) as the source of truth for slot ownership.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7006ccb0cf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const segments = treePath | ||
| .split("/") | ||
| .filter(Boolean) | ||
| .filter((segment) => !isSlotSegment(segment) && segment !== ".") |
There was a problem hiding this comment.
When two layouts resolve to the same URL route, this fallback key path is supposed to disambiguate them, but treePathToScopedLayoutRouteLiteral removes @slot segments, so layouts like app/layout.tsx and app/@modal/layout.tsx both collapse to "/". That causes LayoutRoute de-duplication and merges slot ownership in LayoutSlotMap, producing incorrect LayoutProps typings for slot-local layouts (missing or extra slot props).
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bcc3a36b17
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| await fs.mkdir(path.dirname(outPath), { recursive: true }); | ||
| await fs.writeFile(outPath, content, "utf-8"); |
There was a problem hiding this comment.
generateRouteTypes only writes .next/types/routes.d.ts, but vinext typegen is intended to be a standalone pre-typecheck step and many Next-style TS configs include next-env.d.ts (usually gitignored). In a fresh checkout where that file is absent, running vinext typegen && tsc --noEmit still fails because the env declaration file is never regenerated, so the command does not fully satisfy the expected typegen workflow.
Useful? React with 👍 / 👎.
|
Hi @james-elicx, this is ready for human review when you have a moment.
Happy to split this into smaller PRs (e.g. CLI command vs dev-mode integration vs layout slot wiring) if that would make review easier. |
|
/bigbonk review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Add native App Router route type generation
Overall this is solid work — the type model design is clean, the layout disambiguation via createLayoutRouteKeyMap is well thought out, the serialized dev regeneration handles rapid watcher bursts correctly, and the test coverage is thorough (slot ownership, route groups, escaped segments, dev-mode invalidation). The Codex review rounds addressed all the major issues I would have flagged.
A few things I'd like to see addressed before merge:
Issues
-
addRouteusesArray.includesfor dedup — O(n) per insertion makes overall route collection O(n²). For typical apps this is fine, but it's a latent scaling issue. ASetalongside the array would be cleaner. -
Duplicate
isInvisibleSegment—typegen.tsre-implementsisInvisibleSegmentidentically to the one inapp-route-graph.ts. This is a maintenance risk — if the canonical definition changes (e.g. intercepting route segments become invisible), the typegen copy drifts silently. Consider exporting it from the routing module. -
next-env.d.tsreferencesnexttypes — The generated stub includes/// <reference types="next" />and/// <reference types="next/image-types/global" />. For vinext projects that don't havenextinstalled as a dependency, this will cause TS errors. Should this reference vinext's own type declarations instead, or should the stub be conditional on whethernextis actually installed? -
Race in
drainAppRouteTypeGenerationfinally block — Thefinallycallback setsappRouteTypeGeneration = nullthen checksappRouteTypeGenerationPendingand re-invokesregenerateAppRouteTypes(). But betweennullassignment and theifcheck, there's no actual race (single-threaded), so this is fine. However, the pattern is a bit subtle — a brief comment explaining why thefinallyre-check is needed would help future readers.
See inline comments for specifics.
| if (!routes.includes(route)) { | ||
| routes.push(route); | ||
| routes.sort(compareStrings); | ||
| } |
There was a problem hiding this comment.
O(n) includes on every insertion makes this O(n²) overall. For typical apps with <100 routes this doesn't matter, but a Set alongside the sorted array would be cleaner and avoid the latent scaling issue:
| } | |
| function addRoute( | |
| routes: string[], | |
| params: Map<string, ParamShape>, | |
| route: string, | |
| paramShape: ParamShape, | |
| ): void { | |
| if (!params.has(route)) { | |
| routes.push(route); | |
| routes.sort(compareStrings); | |
| } | |
| params.set(route, paramShape); | |
| } |
Since params is already keyed by route, you can use it as the dedup check and avoid the linear scan entirely. (The sort-on-every-push is also O(n² log n) total — could do a single sort at the end of collectRouteTypeModel — but that's a much smaller concern.)
| return ( | ||
| segment === "." || (segment.startsWith("(") && segment.endsWith(")")) || segment.startsWith("@") | ||
| ); | ||
| } |
There was a problem hiding this comment.
This is an exact copy of the isInvisibleSegment in app-route-graph.ts:1860. Duplicating this definition is a maintenance risk — if the canonical definition in the routing module changes (e.g., intercepting route prefixes become invisible), this copy won't be updated.
Consider exporting isInvisibleSegment from app-route-graph.ts (or a shared routing util) and importing it here.
|
|
||
| const NEXT_ENV_FILE_CONTENT = `/// <reference types="next" /> | ||
| /// <reference types="next/image-types/global" /> | ||
| import "./.next/types/routes.d.ts"; |
There was a problem hiding this comment.
The import "./.next/types/routes.d.ts" line uses a relative import from the project root, which means next-env.d.ts must live at the project root for this to resolve. That's the standard location, so it works, but it's worth a comment explaining the coupling.
More importantly: /// <reference types="next" /> will fail in vinext projects that don't have next installed. Is the intent that next is always a peer dep? If not, this stub should either reference vinext's own type declarations or be conditional on whether next is resolvable.
This is a nit for now since migrated projects will have next installed, but it'll bite fresh vinext-only projects.
| appRouteTypeGeneration = drainAppRouteTypeGeneration().finally(() => { | ||
| appRouteTypeGeneration = null; | ||
| if (appRouteTypeGenerationPending) regenerateAppRouteTypes(); | ||
| }); |
There was a problem hiding this comment.
The finally block sets appRouteTypeGeneration = null then re-checks appRouteTypeGenerationPending. Since JS is single-threaded, there's no actual race here, but the pattern is subtle. A brief comment explaining why the re-check is needed would help:
| }); | |
| appRouteTypeGeneration = drainAppRouteTypeGeneration().finally(() => { | |
| appRouteTypeGeneration = null; | |
| // An event may have arrived after drain finished but before we cleared | |
| // the in-flight promise. Re-check and restart if so. | |
| if (appRouteTypeGenerationPending) regenerateAppRouteTypes(); | |
| }); |
| params: Promise<VinextRouteTypes.ParamMap[Route]>; | ||
| children: React.ReactNode; | ||
| } & { | ||
| [K in VinextRouteTypes.LayoutSlotMap[Route]]: React.ReactNode; |
There was a problem hiding this comment.
Nit: The { mapped type brace is on the same line as the & { intersection. This is correct but dense. More importantly, when LayoutSlotMap[Route] resolves to never (no slots), the intersection & { [K in never]: ReactNode } produces & {} which is fine — just verifying this is intentional and tested (it is, via the "/@modal": never assertion).
|
|
||
| for (const slot of route.parallelSlots) { | ||
| const ownerLayoutId = findSlotOwnerLayoutId(route, slot); | ||
| if (slot.layoutPath) { |
There was a problem hiding this comment.
Nice — materializing slot-local layouts in the segment graph is the right call. This means app/@modal/layout.tsx gets its own layout entry with a treePath of /@modal, which keeps it distinct from the root layout in typegen's LayoutRoute union.
One question: if a slot has a layoutPath but its ownerLayoutId points to an ancestor, the slot-local layout gets registered here but the slot itself (line 465) still references the ancestor via ownerLayoutId. This is correct for typegen (slot props go to the ancestor), but could it confuse other consumers of the segment graph that iterate layouts and expect every layout to have at least one slot? Worth a brief comment documenting that slot-local layouts may have zero slots in the manifest.
| root, | ||
| mode: "production", | ||
| }); | ||
| const resolvedNextConfig = await resolveNextConfig( |
There was a problem hiding this comment.
Nit: resolvedNextConfig loads both loadNextConfig and resolveNextConfig, which reads the config file and resolves paths. If the project has no next.config.{js,ts,mjs}, does loadNextConfig throw or return defaults? If it throws, the error message won't be very helpful for a typegen command. Consider catching and providing a friendlier message like "No next.config found; using defaults".
|
|
||
| let server: ViteDevServer | null = null; | ||
| try { | ||
| server = await createServer({ |
There was a problem hiding this comment.
The dev-mode test passes { appDir: root } to vinext(), which sets appDir to the project root rather than root + '/app'. This works because the plugin detects the app/ subdirectory, but it's a bit confusing — at first glance it looks like the app dir is the root. Consider passing path.join(root, 'app') for clarity, or add a comment explaining why root works here.
|
Review posted. Here's a summary of what I found: The PR is well-structured — it adds a clean Key feedback items:
|
|
Thanks for the thorough review @james-elicx / @ask-bonk. Addressed in
Deferred (happy to follow up if you'd prefer them in this PR):
The |
|
/bigbonk review |
There was a problem hiding this comment.
Review: Add native App Router route type generation
This is well-designed work. The type model is clean, the layout disambiguation via createLayoutRouteKeyMap handles the tricky route-group collision cases correctly, the serialized drain loop for dev regeneration is sound, and the test coverage is thorough across slot ownership, escaped segments, route groups, and dev-mode invalidation. The iterative Codex rounds addressed all the major structural issues (slot ownership, layout params from manifest, scoped route groups, serialized writes).
I have a few remaining items — two that should be fixed before merge, and several minor observations.
Should fix
-
addRoutesorts on every insertion —routes.sort(compareStrings)runs after every push, making the total cost O(n² log n) across all routes. Since the sort only matters for the final output, deferring it to after collection completes (or using insertion sort for O(n²) but better constants) would be cleaner. Not a correctness issue, but it's unnecessary work and the pattern is easy to misread as intentional. See inline comment. -
Slot name dedup uses
Array.includes— Same pattern at line 116. Slot lists per layout are small so this is even less of a practical concern, but the inconsistency with the route dedup (which usesSet) stands out. Consider using aSethere too for consistency.
Minor / nits
-
treePathToScopedLayoutRouteLiteralonly filters.— The disambiguation function keeps@slotand(group)segments, which is correct for distinguishingapp/layout.tsxfromapp/@modal/layout.tsx. But it also keeps route-group segments like(marketing), which means two layouts under different groups at the same path disambiguate correctly too. Just want to confirm this is the intentional design — the"/(marketing)"test assertion suggests it is. -
writeRouteTypesduringconfigResolved— This runsawait writeRouteTypes()at plugin config time (line 875), which means the initial typegen runs synchronously in the config resolution path. For large apps this could slow down Vite startup noticeably. Not blocking, but worth noting — if it becomes a problem, it could be deferred toconfigureServerwith the first write happening asynchronously. -
next-env.d.tsreferencesnexttypes — Acknowledged in the PR thread as a known gap for vinext-only projects. The deferral reasoning (matching existing examples that useskipLibCheck: true) is reasonable. Tracking this as a follow-up is fine.
| if (!seen.has(route)) { | ||
| seen.add(route); | ||
| routes.push(route); | ||
| routes.sort(compareStrings); |
There was a problem hiding this comment.
Sorting after every push makes addRoute O(n log n) per call and O(n² log n) total across all routes. Since the sorted order only matters for the rendered output, consider deferring the sort to after all routes are collected:
| routes.sort(compareStrings); | |
| if (!seen.has(route)) { | |
| seen.add(route); | |
| routes.push(route); | |
| } |
Then add a single routes.sort(compareStrings) call at the end of collectRouteTypeModel for each route list (or sort in renderRouteTypes). The current approach is correct but does unnecessary work.
| if (!layoutRoute) continue; | ||
|
|
||
| const slots = model.layoutSlots.get(layoutRoute) ?? []; | ||
| if (!slots.includes(slot.name)) { |
There was a problem hiding this comment.
Nit: this uses Array.includes for slot name dedup while route dedup (line 308) uses a Set. For consistency, consider using a Set<string> here too, even though slot lists per layout are small enough that it doesn't matter for performance:
| if (!slots.includes(slot.name)) { | |
| const slots = model.layoutSlots.get(layoutRoute) ?? []; | |
| if (!slots.includes(slot.name)) { | |
| slots.push(slot.name); | |
| } |
Or better yet, track a Set alongside the array (same pattern as pageRouteSet/layoutRouteSet) and defer the sort to after the loop.
| return segments.length === 0 ? "/" : `/${segments.join("/")}`; | ||
| } | ||
|
|
||
| function treePathToScopedLayoutRouteLiteral(treePath: string): string { |
There was a problem hiding this comment.
This is a subtle but important distinction from treePathToRouteLiteral — it keeps @slot and (group) segments for disambiguation. The asymmetry between the two functions (one filters isInvisibleSegment, the other only filters .) is correct but easy to misread.
Consider a doc-comment explaining when this variant is used and why it preserves route-group/slot segments:
| function treePathToScopedLayoutRouteLiteral(treePath: string): string { | |
| /** | |
| * Convert a layout tree path to a scoped route literal that preserves | |
| * route-group and @slot segments. Used only for disambiguation when | |
| * multiple layouts collapse to the same URL route literal. | |
| */ | |
| function treePathToScopedLayoutRouteLiteral(treePath: string): string { |
| return layoutRouteKeys.get(layout.treePath) ?? treePathToRouteLiteral(layout.treePath); | ||
| } | ||
|
|
||
| function treePathToRouteLiteral(treePath: string): string { |
There was a problem hiding this comment.
Same suggestion — a brief doc-comment here would help readers understand the contract:
| function treePathToRouteLiteral(treePath: string): string { | |
| /** Convert a layout tree path to its URL route literal, stripping invisible segments. */ | |
| function treePathToRouteLiteral(treePath: string): string { |
| return outPath; | ||
| } | ||
|
|
||
| async function ensureNextEnvFile(root: string): Promise<void> { |
There was a problem hiding this comment.
The ensureNextEnvFile check uses fs.access to test existence and silently catches to create. This is correct, but there's a TOCTOU window — if two concurrent generateRouteTypes calls race (e.g., during the dev drain loop), both could see the file as absent and write. In practice the content is idempotent so this is harmless, but fs.writeFile with { flag: 'wx' } (exclusive create) would be more precise:
| async function ensureNextEnvFile(root: string): Promise<void> { | |
| async function ensureNextEnvFile(root: string): Promise<void> { | |
| const envPath = path.join(root, "next-env.d.ts"); | |
| try { | |
| await fs.writeFile(envPath, NEXT_ENV_FILE_CONTENT, { flag: "wx" }); | |
| } catch (error: unknown) { | |
| // File already exists — leave it untouched. | |
| if ((error as NodeJS.ErrnoException).code !== "EEXIST") throw error; | |
| } | |
| } |
This is a nit — the current approach works fine.
| instrumentationPath = findInstrumentationFile(root, fileMatcher); | ||
| instrumentationClientPath = findInstrumentationClientFile(root, fileMatcher); | ||
| middlewarePath = findMiddlewareFile(root, fileMatcher); | ||
| await writeRouteTypes(); |
There was a problem hiding this comment.
This await writeRouteTypes() runs in the configResolved hook, which means it blocks Vite startup. For most apps this is fast, but for large route trees the FS scan + write could add noticeable latency to vite dev startup.
Not blocking — just flagging that if startup perf becomes a concern, this could be moved to fire-and-forget in configureServer (since the types aren't needed for Vite to start serving).
|
Review posted successfully. Here's a summary of what I found: Overall assessment: This is solid, well-iterated work. The type model design is clean, layout disambiguation via Key feedback:
|
|
Thanks @james-elicx / @ask-bonk. Addressed in
Skipped per your earlier framing:
|
|
Hey @james-elicx, quick ping from my side. The last review blocker is fixed now and CI is green on I can’t trigger |
|
/bigbonk review |
There was a problem hiding this comment.
Review: Add native App Router route type generation
This is well-designed, well-iterated work. The type model is clean, the layout disambiguation via createLayoutRouteKeyMap handles route-group collisions correctly, the serialized drain loop for dev regeneration is sound, and the test coverage is thorough (slot ownership, escaped segments, route groups, dev-mode invalidation, next-env.d.ts preservation). The paramShapesEqual assertion on duplicate routes is a nice defensive measure.
The prior review rounds have addressed all the major structural issues. I have a few remaining items — one that I think should be fixed, and several observations.
Should fix
addRoutestill does redundantparams.seton known-equal shapes — After theparamShapesEqualcheck confirms equality, the function unconditionally callsparams.set(route, paramShape)even though the existing shape is identical. This is a minor clarity issue — the early return whenseen.has(route)already skips the push, but theparams.setstill runs. Skipping it when the shape already exists would make the intent clearer and avoid an unnecessary Map write on every duplicate.
Observations / nits
-
next-env.d.tsimportvs/// <reference path>— The current implementation matches Next.js canary (importfor ESLint compat) and the existing vinext examples. Good. -
export {}in generated output — Redundant given theimport type * as Reactalready makes the file a module, but reasonable as defensive codegen practice. Fine to keep. -
await writeRouteTypes()inconfigResolved— Blocks Vite startup. For typical apps this is fast, but worth noting as a potential optimization target for very large route trees. Not blocking. -
Empty
ParamMapfallback — When there are no routes,renderParamMapemits[route: string]: {}which is an open index signature. Only matters in the degenerate no-routes case, so not blocking, but worth noting. -
CI reliability improvements — The Node HTTP probe with explicit
127.0.0.1,--max-time-equivalent timeout, and per-attempt logging is a solid improvement over the prior curl-based approach. Thetrapfor cleanup is good practice. -
Link navigation test fix — Sequentializing the
importcalls to avoid a mock-order race is the right fix. The priorPromise.allcould allow React to load before the mock was applied.
No blocking issues. This is ready to merge.
| if (existingParamShape && !paramShapesEqual(existingParamShape, paramShape)) { | ||
| throw new Error(`[vinext] Conflicting route param shapes generated for ${route}`); | ||
| } | ||
| params.set(route, paramShape); |
There was a problem hiding this comment.
Nit: after the paramShapesEqual check confirms the shapes match, this still unconditionally overwrites the map entry with an identical value. Skipping the write when the key already exists would make the contract clearer:
| params.set(route, paramShape); | |
| if (!existingParamShape) { | |
| params.set(route, paramShape); | |
| } |
Not blocking — the current code is correct.
| ]); | ||
|
|
||
| return `// This file is generated by vinext. Do not edit. | ||
| import type * as React from "react"; |
There was a problem hiding this comment.
Very minor nit: since React is only used for React.ReactNode in the generated output, you could use a named import type and reference ReactNode directly:
| import type * as React from "react"; | |
| import type { ReactNode } from "react"; |
Then React.ReactNode → ReactNode in the template strings below. Slightly more precise for a declaration file. Not blocking.
| routes: readonly string[], | ||
| params: ReadonlyMap<string, ParamShape>, | ||
| ): string { | ||
| if (routes.length === 0) return " [route: string]: {};\n"; |
There was a problem hiding this comment.
When routes is empty, this emits an open index signature [route: string]: {} that accepts any string key. The route unions above correctly resolve to never, so PageProps etc. are already unusable (good). But a consumer could write VinextRouteTypes.ParamMap["anything"] and get {} without a type error.
Consider emitting an empty interface body or a // no routes comment instead, so ParamMap stays closed in the no-routes case. Very low priority — only affects the degenerate empty project scenario.
| // Sort all collected route lists once after collection. addRoute() and the | ||
| // slot loop above intentionally skip per-insertion sorts to keep collection | ||
| // O(n) — the rendered output relies on stable sorted order, so the single | ||
| // pass here is enough. | ||
| model.pageRoutes.sort(compareStrings); | ||
| model.layoutRoutes.sort(compareStrings); | ||
| model.routeHandlerRoutes.sort(compareStrings); | ||
| for (const slotNames of model.layoutSlots.values()) slotNames.sort(compareStrings); |
There was a problem hiding this comment.
Nice — the single deferred sort pass is the right call. Clean comment explaining the design decision.
| function createLayoutRouteKeyMap(layouts: Iterable<{ treePath: string }>): Map<string, string> { | ||
| const treePathsByRoute = new Map<string, string[]>(); | ||
| for (const { treePath } of layouts) { | ||
| const route = treePathToRouteLiteral(treePath); | ||
| const treePaths = treePathsByRoute.get(route) ?? []; | ||
| treePaths.push(treePath); | ||
| treePathsByRoute.set(route, treePaths); | ||
| } | ||
|
|
||
| const keys = new Map<string, string>(); | ||
| for (const [route, treePaths] of treePathsByRoute) { | ||
| for (const treePath of treePaths) { | ||
| keys.set( | ||
| treePath, | ||
| treePaths.length === 1 ? route : treePathToScopedLayoutRouteLiteral(treePath), | ||
| ); | ||
| } | ||
| } | ||
| return keys; | ||
| } |
There was a problem hiding this comment.
Good design. The two-pass approach (group by URL route literal, then disambiguate collisions) cleanly separates the common case from the edge case. The treePaths.length === 1 guard ensures disambiguation only fires when necessary.
| if (slot.layoutPath) { | ||
| // Materialize the slot-local layout as its own entry so consumers | ||
| // (e.g. typegen) can distinguish it from the owning layout. Note | ||
| // that this layout may have zero entries in `slots`: the slot | ||
| // itself is registered below against `ownerLayoutId`, which points | ||
| // to the ancestor layout that owns the slot prop. |
There was a problem hiding this comment.
Good documentation of a non-obvious invariant. This comment will prevent future readers from incorrectly assuming every layout has at least one slot entry.
| appRouteTypeGeneration = drainAppRouteTypeGeneration().finally(() => { | ||
| appRouteTypeGeneration = null; | ||
| // A watcher event may have arrived after the drain loop's final | ||
| // check but before this finally runs; restart the loop if so. | ||
| if (appRouteTypeGenerationPending) regenerateAppRouteTypes(); | ||
| }); | ||
| } |
There was a problem hiding this comment.
The drain + finally re-check design is clean and correct. The comment explaining the edge case is helpful. Verified: if writeRouteTypes() throws persistently, the drain loop exits after one iteration and no infinite retry occurs — new watcher events are needed to restart. This is the right behavior.
| * hasRemainingVisibleSegments — keep this the single source of truth. | ||
| */ | ||
| function isInvisibleSegment(segment: string): boolean { | ||
| export function isInvisibleSegment(segment: string): boolean { |
There was a problem hiding this comment.
Good — exporting isInvisibleSegment here and importing it in typegen.ts eliminates the prior duplication. Single source of truth.
| it("updates generated route helper types when App Router files are added in dev", async () => { | ||
| await withTempProject(async (root) => { | ||
| await writeProjectFile(root, "app/layout.tsx", EMPTY_LAYOUT); | ||
| await writeProjectFile(root, "app/page.tsx", EMPTY_PAGE); | ||
|
|
||
| let server: ViteDevServer | null = null; | ||
| try { | ||
| // `appDir` in the vinext plugin options names the project root, not | ||
| // the App Router directory; the plugin auto-detects `app/` (or | ||
| // `src/app/`) under it. Pass the project root explicitly here so | ||
| // the dev server uses the same root path for both Vite and vinext. | ||
| server = await createServer({ | ||
| root, | ||
| logLevel: "silent", | ||
| plugins: [vinext({ appDir: root })], | ||
| }); | ||
|
|
||
| const generatedPath = path.join(root, ".next", "types", "routes.d.ts"); | ||
| await eventually(async () => { | ||
| expect(await readFile(generatedPath, "utf-8")).toContain('type PageRoute = "/";'); | ||
| }); | ||
|
|
||
| const aboutPage = path.join(root, "app/about/page.tsx"); | ||
| await writeProjectFile(root, "app/about/page.tsx", EMPTY_PAGE); | ||
| server.watcher.emit("add", aboutPage); | ||
|
|
||
| await eventually(async () => { | ||
| expect(await readFile(generatedPath, "utf-8")).toContain( | ||
| 'type PageRoute = "/" | "/about";', | ||
| ); | ||
| }); | ||
|
|
||
| const blogPage = path.join(root, "app/blog/page.tsx"); | ||
| const docsPage = path.join(root, "app/docs/page.tsx"); | ||
| await writeProjectFile(root, "app/blog/page.tsx", EMPTY_PAGE); | ||
| await writeProjectFile(root, "app/docs/page.tsx", EMPTY_PAGE); | ||
| server.watcher.emit("add", blogPage); | ||
| server.watcher.emit("add", docsPage); | ||
|
|
||
| await eventually(async () => { | ||
| expect(await readFile(generatedPath, "utf-8")).toContain( | ||
| 'type PageRoute = "/" | "/about" | "/blog" | "/docs";', | ||
| ); | ||
| }); | ||
| } finally { | ||
| await server?.close(); | ||
| } | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Thorough dev-mode test — covers single file addition, rapid burst of multiple files, and verifies the serialized drain produces the correct final state. The eventually helper with retry is a clean pattern for async watcher assertions.
| it("preserves an existing next-env.d.ts", async () => { | ||
| await withTempProject(async (root) => { | ||
| await writeProjectFile(root, "app/layout.tsx", EMPTY_LAYOUT); | ||
| await writeProjectFile(root, "app/page.tsx", EMPTY_PAGE); | ||
| const customContent = '/// <reference types="custom" />\n'; | ||
| await writeProjectFile(root, "next-env.d.ts", customContent); | ||
|
|
||
| await generateRouteTypes({ root }); | ||
| const preserved = await readFile(path.join(root, "next-env.d.ts"), "utf-8"); | ||
|
|
||
| expect(preserved).toBe(customContent); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Good test — verifies ensureNextEnvFile with { flag: "wx" } preserves existing content. This is important for projects with customized next-env.d.ts files.
|
Review posted. Here's a summary: Overall assessment: This is solid, well-iterated work that's ready to merge. The type model design is clean, layout disambiguation handles route-group collisions correctly, the serialized drain loop for dev regeneration is sound, and test coverage is thorough across all the important edge cases. Key feedback:
No blocking issues found. |
JamesbbBriz
force-pushed
the
feature/native-route-typegen
branch
from
dcc0c0f to
ac6a012
Compare
|
@james-elicx rebased onto latest main, conflict resolved, and CI is green now. I think ask-bonk’s latest review is non-blocking. Could you take another look when you get a chance? |
|
/bigbonk review |
There was a problem hiding this comment.
Review: Add native App Router route type generation
This is solid, well-structured work that has clearly been improved through multiple review iterations. The type model design is clean, the layout disambiguation logic via createLayoutRouteKeyMap is correct, the serialized drain loop for dev regeneration handles rapid watcher bursts properly, and the test coverage is thorough across all the important edge cases.
I have two minor nits — neither is blocking.
Architecture observations
Good decisions:
- Reusing
isInvisibleSegmentfrom the routing module (single source of truth) instead of the earlier duplicated copy. - Adding
patternParts/paramNamestoRouteManifestLayoutso typegen derives layout params from structured manifest data rather than regexing rendered route literals. This makes the%5Bslug%5D(escaped bracket) case correct by construction. - The
paramShapesEqualassertion on duplicate routes — catches real bugs if page/layout/handler shapes ever diverge. ensureNextEnvFilewith{ flag: "wx" }— correct and race-free.- The deferred single-pass sort after collection — O(n log n) total instead of O(n² log n).
- Using
importinnext-env.d.tsmatching Next.js canary's ESLint-compat rationale.
Drain loop correctness: Verified. The while loop clears pending before await, so events during the write set pending and trigger another iteration. The finally re-check covers the narrow window between the last loop-condition check and microtask scheduling. If writeRouteTypes() throws persistently, the loop exits after one iteration without infinite retry — new watcher events are needed to restart. All correct.
See inline comments for the two nits.
| } | ||
| return; | ||
| } | ||
| params.set(route, paramShape); |
There was a problem hiding this comment.
Nit: after the paramShapesEqual check confirms the shapes are identical (line 335), this still unconditionally writes the identical value back into the map. Skipping the write when the key already exists would make the intent clearer — if the shapes are proven equal, there's nothing to update:
| params.set(route, paramShape); | |
| const existingParamShape = params.get(route); | |
| if (existingParamShape) { | |
| if (!paramShapesEqual(existingParamShape, paramShape)) { | |
| throw new Error(`[vinext] Conflicting route param shapes generated for ${route}`); | |
| } | |
| return; | |
| } | |
| params.set(route, paramShape); |
Wait — re-reading the code, this is exactly what's already here. My mistake, the early return on line 338 already skips the set. This is correct as-is. (Leaving the comment for transparency.)
| routes: readonly string[], | ||
| params: ReadonlyMap<string, ParamShape>, | ||
| ): string { | ||
| if (routes.length === 0) return " [route: string]: {};\n"; |
There was a problem hiding this comment.
Nit: when routes is empty (no App Router routes at all), this emits [route: string]: {}; — an open index signature that lets VinextRouteTypes.ParamMap["anything"] resolve to {} without a type error. The route unions above correctly resolve to never, so the generic constraints on PageProps/LayoutProps/RouteContext already prevent misuse. But if you want the ParamMap interface itself to be closed in the empty case, you could emit an empty body or a // no routes comment instead.
Very low priority — only affects the degenerate empty-project scenario.
|
Review posted and approved. Here's a summary: Approved — this PR is ready to merge. The type model design is clean, the layout disambiguation handles all the tricky edge cases (route-group collisions, slot-local layouts, escaped bracket segments), the serialized drain loop for dev regeneration is correct, and test coverage is thorough. Key things I verified:
Two minor nits posted inline (neither blocking): the open |
JamesbbBriz
force-pushed
the
feature/native-route-typegen
branch
from
f7a061e to
8edcb50
Compare
|
@james-elicx rebased onto latest main again, resolved the CLI import conflict, and CI is green. Looks ready to merge when you have a moment. |
|
@james-elicx gentle ping on this one. It has an approving review, all checks are green, and it’s clean with main now. Do you think this is good to merge, or is there anything else you’d like me to adjust? |
|
I'll have a look on Monday - currently prioritising bug fixes. |
2 participants
Summary
PageProps,LayoutProps, andRouteContext..next/types/routes.d.tsfrom Vinext's existing App Router route graph.Closes #664
Test Plan
NODE_OPTIONS=--experimental-strip-types pnpm exec vp check --fixpnpm test tests/typegen.test.ts tests/check.test.tspnpm exec tsc --noEmit --pretty falsefrompackages/vinextpnpm --filter vinext run buildgit diff --checkvinext typegenagainst a temp App Router project