Skip to content

Commit

Permalink
fix(file-router): allow routes with the same path (#2971)
Browse files Browse the repository at this point in the history 
* 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: Anton Platonov <[email protected]>
  • Loading branch information
@Lodin @platosha
Lodin and platosha authored Dec 10, 2024
1 parent 35aaded commit 0a72e18
Show file tree
Hide file tree
Showing 2 changed files with 183 additions and 36 deletions.
132 changes: 101 additions & 31 deletions packages/ts/file-router/src/runtime/RouterConfigurationBuilder.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,13 +32,14 @@ function isReactRouteModule(module?: Module): module is RouteModule<ComponentTyp
export type RouteList = readonly RouteObject[];
export type WritableRouteList = RouteObject[];

export type RouteTransformer<T> = (
original: RouteObject | undefined,
overriding: T | undefined,
children?: RouteList,
) => RouteObject | undefined;
export type RouteTransformerOptions<T> = Readonly<{
children?: RouteList;
original?: RouteObject;
overriding?: T;
dupe: boolean;
}>;

export type RouteListSplittingRule = (route: RouteObject) => boolean;
export type RouteTransformer<T> = (opts: RouteTransformerOptions<T>) => RouteObject | undefined;

type RoutesModifier = (routes: RouteList | undefined) => RouteList | undefined;

Expand Down Expand Up @@ -85,7 +86,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)) {
Expand Down Expand Up @@ -141,8 +142,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;
}
Expand Down Expand Up @@ -277,7 +278,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;
Expand All @@ -286,54 +287,123 @@ 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<T extends RouteBase>(routes: undefined, callback: RouteTransformer<undefined>): this;
update<T extends RouteBase>(routes: readonly T[], callback?: RouteTransformer<T>): this;
update<T extends RouteBase>(
routes: readonly T[] | undefined,
callback: RouteTransformer<T | undefined> = (original, overriding, children) =>
callback: RouteTransformer<T | undefined> = ({ original, overriding, children }) =>
({
...original,
...overriding,
children,
}) 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<readonly [RouteList | undefined, readonly T[] | undefined], RouteList | undefined>(
[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<RouteObject | undefined> = [];
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);
}

Expand Down
87 changes: 82 additions & 5 deletions packages/ts/file-router/test/runtime/RouterConfigurationBuilder.spec.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,15 @@ describe('RouterBuilder', () => {
},
]);
reset = mockDocumentBaseURI('https://example.com/foo');
// @ts-expect-error Fake just enough so tests pass
globalThis.window = { history: { replaceState: () => {} }, location: '', addEventListener: () => {} };
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;
});
Expand Down Expand Up @@ -171,9 +178,6 @@ describe('RouterBuilder', () => {
{
path: '/test',
element: <div>Test</div>,
},
{
path: '/test',
children: [
{
path: '/child-test',
Expand Down Expand Up @@ -803,4 +807,77 @@ describe('RouterBuilder', () => {
expect(protectRoute).to.have.been.calledWith(test, '/login');
});
});

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: <NextTest />,
handle: {
menu: { order: 0 },
title: 'Public view',
},
index: true,
},
{ path: '*', element: <Server /> },
],
handle: { title: 'undefined' },
},
{ path: '*', element: <Server /> },
{ index: true, element: <Server /> },
]);
});
});
});

0 comments on commit 0a72e18

Please sign in to comment.