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 |
…e-typegen # Conflicts: # packages/vinext/src/routing/app-route-graph.ts
|
/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:
|
|
/bigbonk review |
There was a problem hiding this comment.
Review: Add native App Router route type generation
This is well-executed work. The type model design is clean, the layout disambiguation via createLayoutRouteKeyMap handles route-group collisions correctly, the serialized drain loop is sound, and the test coverage is thorough across the important cases (slot ownership, escaped segments, route groups, dev-mode invalidation, next-env.d.ts preservation).
The iterative review rounds addressed all the major structural issues. The current state of the code is solid — I have a few remaining observations (mostly minor) below.
Summary of items
-
Unknown flags silently dropped (minor gap) —
parseArgsnow collects positionals for non-flag args, but unknown flags (e.g.--foobar) that don't matchFLAG_PATTERNare silently ignored. Not introduced by this PR (existing behavior), but worth noting sincetypegenis the first command that relies on positionals. -
paramsmap last-write-wins on duplicates (observation) —addRoutealways callsparams.set(route, paramShape)even when the route was already seen. If a page and layout both produce the same route string with different param shapes, the last writer wins silently. In practice page/layout/handler routes are collected from separate manifest entries with consistent shapes, so this is not a real concern today, but the contract is implicit. -
convertTreePathToRoutePartsfor slot paths (correctness check) — CallingconvertTreePathToRouteParts("/@modal")splits to["@modal"], thenconvertSegmentsToRoutePartsskips it as invisible, yielding{ urlSegments: [], params: [] }. This is correct — slot-local layouts have empty URL parts. Verified via the test assertion attests/app-route-graph.test.ts:330-331. -
Dev regeneration error handling —
warnRouteTypeGenerationFailurelogs and continues, which is the right call for dev. The drain loop won't propagate failures, so a transient FS error (e.g. full disk) won't crash the dev server. Good. -
No
changewatcher event — Regeneration hooks onaddandunlinkonly. Achangeevent on an existing route file (e.g. renaming frompage.tsxtoroute.ts) won't trigger regeneration. This matches the existinginvalidateAppRoutingModulespattern so it's consistent, but it's a latent gap — a user renamingpage.tsxtoroute.tswould need to save again or restart dev.
See inline comments for specifics.
| seen.add(route); | ||
| routes.push(route); | ||
| } | ||
| params.set(route, paramShape); |
There was a problem hiding this comment.
Observation: params.set(route, paramShape) is an unconditional overwrite — if a route string appears in both pageRoutes and layoutRoutes (which it will for routes like "/" or "/dashboard"), the layout's param shape silently replaces the page's. Since layouts are processed after pages, this means the layout param shape wins.
In practice these should always agree (they're derived from the same route segments), but the implicit last-writer-wins contract is easy to miss. Consider either:
- Only setting when the key is new (matching the
seen.hasguard), or - Adding a debug assertion that the shapes match when overwriting.
Not blocking — just a maintenance note.
|
|
||
| type ParamShape = Map<string, "string" | "string[]" | "string[]?">; | ||
|
|
||
| const NEXT_ENV_FILE_CONTENT = `/// <reference types="next" /> |
There was a problem hiding this comment.
The generated next-env.d.ts content has /// <reference types="next" /> and /// <reference types="next/image-types/global" />. I understand the decision to defer (matching existing examples that use skipLibCheck: true), and the PR thread documents this clearly.
One thought for the follow-up: when vinext ships its own type declarations, the stub could conditionally reference vinext types when next isn't resolvable. This would make vinext typegen && tsc --noEmit work out-of-the-box for greenfield vinext projects.
| ); | ||
| break; | ||
| } | ||
| if (!FLAG_PATTERN.test(arg)) { |
There was a problem hiding this comment.
Nit: this guard correctly skips flag-looking strings (so --unknown isn't collected as a positional), but it also means unknown flags are silently swallowed. For typegen this is fine — the only expected positional is the directory — but as more commands adopt positionals, a warning for unrecognized flags would improve DX.
Not blocking for this PR.
| ); | ||
| } | ||
|
|
||
| async function drainAppRouteTypeGeneration() { |
There was a problem hiding this comment.
The drain loop design is clean. One subtle detail worth calling out: the while loop clears pending at the top, then awaits the write. If an event arrives during the write, pending gets set to true by regenerateAppRouteTypes, and the loop runs another iteration — which is correct.
The finally block at line 2095 covers the edge case where an event arrives after the loop exits but before finally runs (i.e. during microtask scheduling of the .finally callback). Since both the event handler and finally run on the same microtask queue, this window is extremely narrow, but the guard is still correct to have. Good attention to detail.
| } | ||
| if (hasAppDir && shouldInvalidateAppRouteFile(appDir, filePath, fileMatcher)) { | ||
| invalidateAppRoutingModules(); | ||
| regenerateAppRouteTypes(); |
There was a problem hiding this comment.
Typegen regeneration hooks on add and unlink but not change. This matches the existing invalidateAppRoutingModules pattern, so it's consistent.
However, if a user renames page.tsx to route.ts in the same directory (editor "save as" rather than move), the watcher fires unlink for the old file + add for the new one, so that case is covered. The gap is an in-place edit that changes exports (e.g. adding/removing generateStaticParams) — but since typegen only cares about route presence, not exports, this is fine.
Just documenting the reasoning for future readers.
|
|
||
| for (const slot of route.parallelSlots) { | ||
| const ownerLayoutId = findSlotOwnerLayoutId(route, slot); | ||
| if (slot.layoutPath) { |
There was a problem hiding this comment.
The comment at lines 463-467 is well-written and explains a non-obvious invariant. This is exactly the kind of documentation that prevents future confusion.
One edge case to consider for a follow-up: if a slot directory has a layout.tsx and nested sub-routes with their own slots, those nested slots would have ownerLayoutId pointing to the slot-local layout. The current code handles this correctly because the materialized slot-local layout gets its own entry in layouts, so layoutRouteKeyForSlot in typegen can resolve it. Just confirming the chain works end-to-end.
| return params; | ||
| } | ||
|
|
||
| function createLayoutRouteKeyMap(layouts: Iterable<{ treePath: string }>): Map<string, string> { |
There was a problem hiding this comment.
Nice design. createLayoutRouteKeyMap does a two-pass approach: first group tree paths by their URL route literal, then disambiguate collisions by falling back to the scoped variant. This cleanly separates the common case (no collision, use URL route) from the edge case (route-group collision, use scoped key).
The treePaths.length === 1 check at line 269 ensures disambiguation only fires when necessary, avoiding unnecessarily noisy route keys like "/(marketing)" when there's no ambiguity.
| }); | ||
| }); | ||
|
|
||
| it("keeps layout slots scoped to their root route group", async () => { |
There was a problem hiding this comment.
This test is a good regression guard for the route-group slot scoping bug that Codex caught. The negative assertion at line 100 (not.toContain) is important — it verifies that slot names from different route groups don't leak into a merged "/" entry.
One gap: there's no test for a project with both (marketing) and (shop) route groups plus a root layout at app/layout.tsx. In that case, three layouts collapse to "/" and all three need disambiguation. If this is supported, it'd be worth adding coverage. If it's an error in Next.js (which I believe it is — multiple root layouts conflict), then the current test is correct.
|
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 is correct, the serialized dev regeneration handles rapid watcher bursts properly, and test coverage is thorough across all the important edge cases. Key observations:
No blocking issues found. |
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