-
Notifications
You must be signed in to change notification settings - Fork 218
fix: App Router route-group and slot collision #447
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
4f222c6
c86efe8
af29375
25db7f7
91be782
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -37,6 +37,8 @@ export interface InterceptingRoute { | |
| export interface ParallelSlot { | ||
| /** Slot name (e.g. "team" from @team) */ | ||
| name: string; | ||
| /** Absolute path to the @slot directory that owns this slot. Internal routing metadata. */ | ||
| ownerDir: string; | ||
| /** Absolute path to the slot's page component */ | ||
| pagePath: string | null; | ||
| /** Absolute path to the slot's default.tsx fallback */ | ||
|
|
@@ -211,7 +213,19 @@ function discoverSlotSubRoutes( | |
| matcher: ValidFileMatcher, | ||
| ): AppRoute[] { | ||
| const syntheticRoutes: AppRoute[] = []; | ||
| const existingPatterns = new Set(routes.map((r) => r.pattern)); | ||
|
|
||
| // O(1) lookup for existing routes by pattern — avoids O(n) routes.find() per sub-path per parent. | ||
| // Updated as new synthetic routes are pushed so that later parents can see earlier synthetic entries. | ||
| const routesByPattern = new Map<string, AppRoute>(routes.map((r) => [r.pattern, r])); | ||
|
|
||
| const slotKey = (slotName: string, ownerDir: string): string => `${slotName}\u0000${ownerDir}`; | ||
|
|
||
| const applySlotSubPages = (route: AppRoute, slotPages: Map<string, string>): void => { | ||
| route.parallelSlots = route.parallelSlots.map((slot) => ({ | ||
| ...slot, | ||
| pagePath: slotPages.get(slotKey(slot.name, slot.ownerDir)) ?? slot.pagePath, | ||
| })); | ||
| }; | ||
|
|
||
| for (const parentRoute of routes) { | ||
| if (parentRoute.parallelSlots.length === 0) continue; | ||
|
|
@@ -220,19 +234,53 @@ function discoverSlotSubRoutes( | |
| const parentPageDir = path.dirname(parentRoute.pagePath); | ||
|
|
||
| // Collect sub-paths from all slots. | ||
| // Map: relative sub-path (e.g., "demographics") -> Map<slotName, pagePath> | ||
| const subPathMap = new Map<string, Map<string, string>>(); | ||
| // Map: normalized visible sub-path -> slot pages, raw filesystem segments (for routeSegments), | ||
| // and the pre-computed convertedSubRoute (to avoid a redundant re-conversion in the merge loop). | ||
| const subPathMap = new Map< | ||
| string, | ||
| { | ||
| // Raw filesystem segments (with route groups, @slots, etc.) used for routeSegments so | ||
| // that useSelectedLayoutSegments() sees the correct segment list at runtime. | ||
| rawSegments: string[]; | ||
| // Pre-computed URL parts, params, isDynamic from convertSegmentsToRouteParts. | ||
| converted: { urlSegments: string[]; params: string[]; isDynamic: boolean }; | ||
| slotPages: Map<string, string>; | ||
| } | ||
| >(); | ||
|
|
||
| for (const slot of parentRoute.parallelSlots) { | ||
| const slotDir = path.join(parentPageDir, `@${slot.name}`); | ||
| if (!fs.existsSync(slotDir)) continue; | ||
|
|
||
| const subPages = findSlotSubPages(slotDir, matcher); | ||
| for (const { relativePath, pagePath } of subPages) { | ||
| if (!subPathMap.has(relativePath)) { | ||
| subPathMap.set(relativePath, new Map()); | ||
| const subSegments = relativePath.split(path.sep); | ||
| const convertedSubRoute = convertSegmentsToRouteParts(subSegments); | ||
| if (!convertedSubRoute) continue; | ||
|
|
||
| const { urlSegments } = convertedSubRoute; | ||
| const normalizedSubPath = urlSegments.join("/"); | ||
| let subPathEntry = subPathMap.get(normalizedSubPath); | ||
|
|
||
| if (!subPathEntry) { | ||
| subPathEntry = { | ||
| rawSegments: subSegments, | ||
| converted: convertedSubRoute, | ||
| slotPages: new Map(), | ||
| }; | ||
| subPathMap.set(normalizedSubPath, subPathEntry); | ||
| } | ||
| subPathMap.get(relativePath)!.set(slot.name, pagePath); | ||
|
|
||
| const slotId = slotKey(slot.name, slot.ownerDir); | ||
| const existingSlotPage = subPathEntry.slotPages.get(slotId); | ||
| if (existingSlotPage) { | ||
| const pattern = joinRoutePattern(parentRoute.pattern, normalizedSubPath); | ||
| throw new Error( | ||
| `You cannot have two routes that resolve to the same path ("${pattern}").`, | ||
| ); | ||
| } | ||
|
|
||
| subPathEntry.slotPages.set(slotId, pagePath); | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -241,34 +289,35 @@ function discoverSlotSubRoutes( | |
| // Find the default.tsx for the children slot at the parent directory | ||
| const childrenDefault = findFile(parentPageDir, "default", matcher); | ||
|
|
||
| for (const [subPath, slotPages] of subPathMap) { | ||
| // Convert sub-path segments to URL pattern parts | ||
| const subSegments = subPath.split(path.sep); | ||
| const convertedSubRoute = convertSegmentsToRouteParts(subSegments); | ||
| if (!convertedSubRoute) continue; | ||
|
|
||
| for (const { rawSegments, converted: convertedSubRoute, slotPages } of subPathMap.values()) { | ||
| const { | ||
| urlSegments: urlParts, | ||
| params: subParams, | ||
| isDynamic: subIsDynamic, | ||
| } = convertedSubRoute; | ||
|
|
||
| const subUrlPath = urlParts.join("/"); | ||
| const pattern = | ||
| parentRoute.pattern === "/" ? "/" + subUrlPath : parentRoute.pattern + "/" + subUrlPath; | ||
|
|
||
| // Skip if this pattern already exists as a regular route | ||
| if (existingPatterns.has(pattern)) continue; | ||
| if (syntheticRoutes.some((r) => r.pattern === pattern)) continue; | ||
| const pattern = joinRoutePattern(parentRoute.pattern, subUrlPath); | ||
|
|
||
| const existingRoute = routesByPattern.get(pattern); | ||
| if (existingRoute) { | ||
| if (existingRoute.routePath && !existingRoute.pagePath) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This collision check catches the case where a slot sub-page targets a pattern that already has a Worth noting: the inverse case — a |
||
| throw new Error( | ||
| `You cannot have two routes that resolve to the same path ("${pattern}").`, | ||
| ); | ||
| } | ||
| applySlotSubPages(existingRoute, slotPages); | ||
| continue; | ||
| } | ||
|
|
||
| // Build parallel slots for this sub-route: matching slots get the sub-page, | ||
| // non-matching slots get null pagePath (rendering falls back to defaultPath) | ||
| const subSlots: ParallelSlot[] = parentRoute.parallelSlots.map((slot) => ({ | ||
| ...slot, | ||
| pagePath: slotPages.get(slot.name) || null, | ||
| pagePath: slotPages.get(slotKey(slot.name, slot.ownerDir)) || null, | ||
| })); | ||
|
|
||
| syntheticRoutes.push({ | ||
| const newRoute: AppRoute = { | ||
| pattern, | ||
| pagePath: childrenDefault, // children slot uses parent's default.tsx as page | ||
| routePath: null, | ||
|
|
@@ -282,12 +331,14 @@ function discoverSlotSubRoutes( | |
| notFoundPaths: parentRoute.notFoundPaths, | ||
| forbiddenPath: parentRoute.forbiddenPath, | ||
| unauthorizedPath: parentRoute.unauthorizedPath, | ||
| routeSegments: [...parentRoute.routeSegments, ...subSegments], | ||
| routeSegments: [...parentRoute.routeSegments, ...rawSegments], | ||
| layoutTreePositions: parentRoute.layoutTreePositions, | ||
| isDynamic: parentRoute.isDynamic || subIsDynamic, | ||
| params: [...parentRoute.params, ...subParams], | ||
| patternParts: [...parentRoute.patternParts, ...urlParts], | ||
| }); | ||
| }; | ||
| syntheticRoutes.push(newRoute); | ||
| routesByPattern.set(pattern, newRoute); | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -652,6 +703,7 @@ function discoverParallelSlots( | |
|
|
||
| slots.push({ | ||
| name: slotName, | ||
| ownerDir: slotDir, | ||
| pagePath, | ||
| defaultPath, | ||
| layoutPath: findFile(slotDir, "layout", matcher), | ||
|
|
@@ -881,6 +933,15 @@ function findFile(dir: string, name: string, matcher: ValidFileMatcher): string | |
| return null; | ||
| } | ||
|
|
||
| /** | ||
| * Convert filesystem path segments to URL route parts, skipping invisible segments | ||
| * (route groups, @slots, ".") and converting dynamic segment syntax to Express-style | ||
| * patterns (e.g. "[id]" → ":id", "[...slug]" → ":slug+"). | ||
| * | ||
| * Note: the invisible-segment filtering logic here is also applied manually in | ||
| * discoverSlotSubRoutes when building the dedup key from urlSegments. If a new | ||
| * invisible segment type is added, both locations need updating. | ||
| */ | ||
| function convertSegmentsToRouteParts( | ||
| segments: string[], | ||
| ): { urlSegments: string[]; params: string[]; isDynamic: boolean } | null { | ||
|
|
@@ -947,6 +1008,11 @@ function hasRemainingVisibleSegments(segments: string[], startIndex: number): bo | |
| return false; | ||
| } | ||
|
|
||
| function joinRoutePattern(basePattern: string, subPath: string): string { | ||
| if (!subPath) return basePattern; | ||
| return basePattern === "/" ? `/${subPath}` : `${basePattern}/${subPath}`; | ||
| } | ||
|
|
||
| /** | ||
| * Match a URL against App Router routes. | ||
| */ | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -154,10 +154,24 @@ export function patternToNextFormat(pattern: string): string { | |||||||||||||
| .replace(/:([\w-]+)/g, "[$1]"); | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| function normalizeRoutePattern(pattern: string): string { | ||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor:
Suggested change
|
||||||||||||||
| if (pattern === "/") return "/"; | ||||||||||||||
| const normalized = pattern.replace(/\/+$/, ""); | ||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ultra-minor: the
Suggested change
|
||||||||||||||
| return normalized === "" ? "/" : normalized; | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| export function validateRoutePatterns(patterns: readonly string[]): void { | ||||||||||||||
| const root = new UrlNode(); | ||||||||||||||
| const seenPatterns = new Set<string>(); | ||||||||||||||
| for (const pattern of patterns) { | ||||||||||||||
| root.insert(patternToNextFormat(pattern)); | ||||||||||||||
| const normalizedPattern = normalizeRoutePattern(pattern); | ||||||||||||||
| if (seenPatterns.has(normalizedPattern)) { | ||||||||||||||
| throw new Error( | ||||||||||||||
| `You cannot have two routes that resolve to the same path ("${normalizedPattern}").`, | ||||||||||||||
| ); | ||||||||||||||
| } | ||||||||||||||
| seenPatterns.add(normalizedPattern); | ||||||||||||||
| root.insert(patternToNextFormat(normalizedPattern)); | ||||||||||||||
| } | ||||||||||||||
| root.assertOptionalCatchAllSpecificity(); | ||||||||||||||
| } | ||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: When two different slots contribute sub-pages to the same normalized path (e.g.,
@team/(a)/membersand@analytics/(b)/members),rawSegmentsis set from whichever slot is processed first. This meansrouteSegmentsfor the synthesized route will contain route-group names from an arbitrary slot.In practice this is harmless — route groups are invisible in URLs and
__resolveChildSegmentspasses them through as-is — but it's worth a brief comment noting the first-writer-wins behavior so a future reader doesn't think it's a bug.