From 3c191993f3f200070b7ccf76efa41150bc46ecf7 Mon Sep 17 00:00:00 2001 From: Zhe Sun <31067185+ZheSun88@users.noreply.github.com> Date: Tue, 10 Dec 2024 15:25:15 +0200 Subject: [PATCH] fix(file-router): allow routes with the same path (#2971) (CP: 24.6) (#2979) fix(file-router): allow routes with the same path (#2971) * refactor(file-router): attempt to consider all routes with the same path * refactor(file-router): allow elements with same path in the route list * style(file-router): cleanup * chore: update package-lock.json * test(file-router): fix index fallback assertion * chore: revert package-lock.json * chore(file-router): remove extra dependency * chore(generator-utils): remove unnecessary export * chore: revert package-lock.json * chore(file-router): do not bring old tests back --------- Co-authored-by: Vlad Rindevich Co-authored-by: Anton Platonov --- .../src/runtime/RouterConfigurationBuilder.ts | 132 ++++++++++++++---- .../RouterConfigurationBuilder.spec.tsx | 86 +++++++++++- 2 files changed, 184 insertions(+), 34 deletions(-) diff --git a/packages/ts/file-router/src/runtime/RouterConfigurationBuilder.ts b/packages/ts/file-router/src/runtime/RouterConfigurationBuilder.ts index 0180c653b..9752c01c6 100644 --- a/packages/ts/file-router/src/runtime/RouterConfigurationBuilder.ts +++ b/packages/ts/file-router/src/runtime/RouterConfigurationBuilder.ts @@ -37,13 +37,14 @@ function isReactRouteModule(module?: Module): module is RouteModule = ( - original: RouteObject | undefined, - overriding: T | undefined, - children?: RouteList, -) => RouteObject | undefined; +export type RouteTransformerOptions = Readonly<{ + children?: RouteList; + original?: RouteObject; + overriding?: T; + dupe: boolean; +}>; -export type RouteListSplittingRule = (route: RouteObject) => boolean; +export type RouteTransformer = (opts: RouteTransformerOptions) => RouteObject | undefined; type RoutesModifier = (routes: RouteList | undefined) => RouteList | undefined; @@ -90,7 +91,7 @@ export class RouterConfigurationBuilder { * @param routes - A list of routes to add to the current list. */ withFileRoutes(routes: readonly AgnosticRoute[]): this { - return this.update(routes, (original, added, children) => { + return this.update(routes, ({ original, overriding: added, children }) => { if (added) { const { module, path, flowLayout } = added; if (!isReactRouteModule(module)) { @@ -146,8 +147,8 @@ export class RouterConfigurationBuilder { { index: true, element: createElement(component), handle: config }, ]; - this.update(fallbackRoutes, (original, added, children) => { - if (original && !getRouteHandleFlag(original, RouteHandleFlags.IGNORE_FALLBACK)) { + this.update(fallbackRoutes, ({ original, overriding: added, children, dupe }) => { + if (original && !getRouteHandleFlag(original, RouteHandleFlags.IGNORE_FALLBACK) && !dupe) { if (!children) { return original; } @@ -282,7 +283,7 @@ export class RouterConfigurationBuilder { * and the user is not authenticated. */ protect(redirectPath?: string): this { - this.update(undefined, (route, _, children) => { + this.update(undefined, ({ original: route, children }) => { const finalRoute = protectRoute(route!, redirectPath); finalRoute.children = children as RouteObject[] | undefined; return finalRoute; @@ -291,11 +292,18 @@ export class RouterConfigurationBuilder { return this; } + /** + * Deeply updates the current list of routes with the given routes merging + * them in process. + * + * @param routes - A list of routes to merge with the current list. + * @param callback - A callback to transform the routes during the merge. + */ update(routes: undefined, callback: RouteTransformer): this; update(routes: readonly T[], callback?: RouteTransformer): this; update( routes: readonly T[] | undefined, - callback: RouteTransformer = (original, overriding, children) => + callback: RouteTransformer = ({ original, overriding, children }) => ({ ...original, ...overriding, @@ -303,42 +311,104 @@ export class RouterConfigurationBuilder { }) as RouteObject, ): this { this.#modifiers.push((existingRoutes) => - transformTree<[RouteList | undefined, readonly T[] | undefined], RouteList | undefined>( + // Going through the existing and added list of routes. + transformTree( [existingRoutes, routes], null, ([original, added], next) => { if (original && added) { - const originalMap = new Map(original.map((route) => createRouteEntry(route))); - const addedMap = new Map(added.map((route) => createRouteEntry(route))); - - const paths = new Set([...originalMap.keys(), ...addedMap.keys()]); + // If we have both original and added routes, we have to merge them. + const final: Array = []; + const paths = new Set([...original.map(({ path }) => path), ...added.map(({ path }) => path)]); for (const path of paths) { - const originalRoute = originalMap.get(path); - const addedRoute = addedMap.get(path); - - let route: RouteObject | undefined; - if (originalRoute && addedRoute) { - route = callback(originalRoute, addedRoute, next([originalRoute.children, addedRoute.children])); - } else if (originalRoute) { - route = callback(originalRoute, undefined, next([originalRoute.children, undefined])); - } else { - route = callback(undefined, addedRoute, next([undefined, addedRoute!.children])); + // We can have multiple routes with the same path, so we have to + // consider all of them. + const originalRoutes = original.filter((r) => r.path === path); + // We can have only one route with the same path in the added list. + const addedRoutes = added.filter((r) => r.path === path); + + if (addedRoutes.length > 1) { + throw new Error('Adding multiple routes with the same path is not allowed'); } - if (route) { - originalMap.set(path, route); + const addedRoute = addedRoutes[0] as T | undefined; + + if (originalRoutes.length > 0 && addedRoute) { + // In case we have both original and added routes, we run + // the callback for each original route in pair with the added + // route. To make the difference, we flag all the routes except + // the last one as `dupe`. + // + // Why the last one is not `dupe`? According to the + // `react-router` logic, the last route is the fallback for all + // routes with the same path. So, if we apply callback to it, + // we implicitly apply it to all other routes with the same + // path. + // + // In case this logic doesn't work, the user can apply the + // callback without considering the `dupe` flag. + for (let i = 0; i < originalRoutes.length; i++) { + final.push( + callback({ + original: originalRoutes[i], + overriding: addedRoute, + children: next([originalRoutes[i].children, addedRoute.children]), + dupe: i < originalRoutes.length - 1, + }) ?? originalRoutes[i], + ); + } + } else if (originalRoutes.length > 0) { + // In case we don't have the added route with the path being + // processed, we run the callback for each original route. + for (let i = 0; i < originalRoutes.length; i++) { + final.push( + callback({ + original: originalRoutes[i], + children: next([originalRoutes[i].children, undefined]), + dupe: i < originalRoutes.length - 1, + }) ?? originalRoutes[i], + ); + } + } else { + // In case we don't have the original route with the path being + // processed, we run the callback for only the added route. + const result = callback({ + overriding: addedRoute, + children: next([undefined, addedRoute!.children]), + dupe: false, + }); + + if (result) { + final.push(result); + } } } - return [...originalMap.values()]; + return final.filter((r) => r != null); } else if (original) { + // If we have only original routes, we run the callback for each + // original route. return original - .map((route) => callback(route, undefined, next([route.children, undefined]))) + .map((route) => + callback({ + original: route, + children: next([route.children, undefined]), + dupe: false, + }), + ) .filter((r) => r != null); } else if (added) { + // If we have only added routes, we run the callback for each added + // route. return added - .map((route) => callback(undefined, route, next([undefined, route.children]))) + .map((route) => + callback({ + overriding: route, + children: next([undefined, route.children]), + dupe: false, + }), + ) .filter((r) => r != null); } diff --git a/packages/ts/file-router/test/runtime/RouterConfigurationBuilder.spec.tsx b/packages/ts/file-router/test/runtime/RouterConfigurationBuilder.spec.tsx index 08898a864..8caba49ad 100644 --- a/packages/ts/file-router/test/runtime/RouterConfigurationBuilder.spec.tsx +++ b/packages/ts/file-router/test/runtime/RouterConfigurationBuilder.spec.tsx @@ -43,6 +43,17 @@ describe('RouterBuilder', () => { }, ]); reset = mockDocumentBaseURI('https://example.com/foo'); + globalThis.window = { + // @ts-expect-error Fake just enough so tests pass + history: { + replaceState: () => {}, + }, + // @ts-expect-error Fake just enough so tests pass + location: '', + addEventListener: () => {}, + }; + // @ts-expect-error Fake just enough so tests pass + globalThis.document.defaultView = globalThis.window; }); afterEach(() => { @@ -168,9 +179,6 @@ describe('RouterBuilder', () => { { path: '/test', element:
Test
, - }, - { - path: '/test', children: [ { path: '/child-test', @@ -830,4 +838,76 @@ describe('RouterBuilder', () => { reset(); }); }); + describe('issues', () => { + it('#2954', () => { + const { routes } = new RouterConfigurationBuilder() + .withFileRoutes([ + { + path: '', + children: [ + { + path: '', + module: { + config: { + menu: { order: 0 }, + title: 'Public view', + }, + default: NextTest, + }, + }, + { + path: 'login', + module: { + config: { + menu: { exclude: true }, + flowLayout: false, + }, + }, + }, + ], + }, + ]) + .withFallback(Server) + .protect() + .build(); + + expect(routes).to.be.like([ + { + path: '', + children: [ + { + path: 'login', + handle: { + menu: { + exclude: true, + }, + flowLayout: false, + title: 'undefined', + }, + }, + ], + handle: { + title: 'undefined', + }, + }, + { + path: '', + children: [ + { + element: , + handle: { + menu: { order: 0 }, + title: 'Public view', + }, + index: true, + }, + { path: '*', element: }, + ], + handle: { title: 'undefined' }, + }, + { path: '*', element: }, + { index: true, element: }, + ]); + }); + }); });