Skip to content

Commit

Permalink
refactor(file-router): allow elements with same path in the route list
Browse files Browse the repository at this point in the history
  • Loading branch information
@Lodin
Lodin committed Dec 9, 2024
1 parent 3b536b8 commit 127e663
Show file tree
Hide file tree
Showing 6 changed files with 178 additions and 58 deletions.
2 changes: 1 addition & 1 deletion package-lock.json
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion packages/ts/file-router/package.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@
"@types/mocha": "^10.0.6",
"@types/sinon": "^10.0.17",
"@types/sinon-chai": "^3.2.12",
"@vaadin/hilla-generator-utils": "^24.6.0-beta5",
"chai-as-promised": "^7.1.1",
"chai-deep-equal-ignore-undefined": "^1.1.1",
"chai-fs": "^2.0.0",
Expand All @@ -80,7 +81,6 @@
"type-fest": "^4.9.0"
},
"dependencies": {
"@vaadin/hilla-generator-utils": "24.6.0-beta5",
"@vaadin/hilla-react-auth": "24.6.0-beta5",
"@vaadin/hilla-react-signals": "24.6.0-beta5",
"react": "^18.2.0",
Expand Down
116 changes: 96 additions & 20 deletions packages/ts/file-router/src/runtime/RouterConfigurationBuilder.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,11 +37,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 RouteTransformer<T> = (opts: RouteTransformerOptions<T>) => RouteObject | undefined;

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

Expand Down Expand Up @@ -88,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)) {
Expand Down Expand Up @@ -144,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;
}
Expand Down Expand Up @@ -280,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;
Expand All @@ -289,50 +292,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 final: Array<RouteObject | undefined> = [...original];
// 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) {
// 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);
const addedRoute = added.find((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');
}

const addedRoute = addedRoutes[0] as T | undefined;

if (originalRoutes.length > 0 && addedRoute) {
for (const originalRoute of originalRoutes) {
final.push(callback(originalRoute, addedRoute, next([originalRoute.children, addedRoute.children])));
// 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) {
for (const originalRoute of originalRoutes) {
final.push(callback(originalRoute, undefined, next([originalRoute.children, undefined])));
// 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 {
final.push(callback(undefined, addedRoute, next([undefined, addedRoute!.children])));
// 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 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
76 changes: 40 additions & 36 deletions packages/ts/file-router/test/runtime/RouterConfigurationBuilder.spec.tsx
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { expect, use } from '@esm-bundle/chai';
import chaiDeepEqualIgnoreUndefined from 'chai-deep-equal-ignore-undefined';
import chaiLooseDeepEqual from '@vaadin/hilla-generator-utils/testing/looseDeepEqual.js';
import chaiLike from 'chai-like';
import { createElement } from 'react';
import sinonChai from 'sinon-chai';
Expand All @@ -8,7 +8,8 @@ import { mockDocumentBaseURI } from '../mocks/dom.js';
import { browserRouter, createBrowserRouter } from '../mocks/react-router-dom.js';
import { protectRoute } from '../mocks/vaadin-hilla-react-auth.js';

use(chaiDeepEqualIgnoreUndefined);
use(chaiLooseDeepEqual);
use(chaiLike);
use(sinonChai);

describe('RouterBuilder', () => {
Expand Down Expand Up @@ -70,7 +71,7 @@ describe('RouterBuilder', () => {
])
.build();

expect(routes).to.be.deepEqualIgnoreUndefined([
expect(routes).to.be.to.looseDeepEqual([
{
path: '',
children: [
Expand Down Expand Up @@ -162,7 +163,7 @@ describe('RouterBuilder', () => {

const serverRoutes = [serverWildcard, serverIndex];

expect(routes).to.be.deepEqualIgnoreUndefined([
expect(routes).to.looseDeepEqual([
{
path: '',
children: [
Expand Down Expand Up @@ -271,7 +272,7 @@ describe('RouterBuilder', () => {
])
.build();

expect(routes).to.be.deepEqualIgnoreUndefined([
expect(routes).to.be.to.looseDeepEqual([
{
path: '',
children: [
Expand Down Expand Up @@ -346,7 +347,7 @@ describe('RouterBuilder', () => {
.withLayout(Server)
.build();

expect(routes).to.be.deepEqualIgnoreUndefined([
expect(routes).to.be.to.looseDeepEqual([
{
element: createElement(Server),
handle: {
Expand Down Expand Up @@ -405,7 +406,7 @@ describe('RouterBuilder', () => {
.withLayout(Server)
.build();

expect(routes).to.be.deepEqualIgnoreUndefined([
expect(routes).to.be.to.looseDeepEqual([
{
path: '',
},
Expand Down Expand Up @@ -506,7 +507,7 @@ describe('RouterBuilder', () => {
.withLayout(Server)
.build();

expect(routes).to.be.deepEqualIgnoreUndefined([
expect(routes).to.be.to.looseDeepEqual([
{
element: createElement(Server),
handle: {
Expand Down Expand Up @@ -639,7 +640,7 @@ describe('RouterBuilder', () => {
it('should not throw when no routes', () => {
const { routes } = new RouterConfigurationBuilder().withLayout(Server).build();

expect(routes).to.be.deepEqualIgnoreUndefined([]);
expect(routes).to.be.to.looseDeepEqual([]);
});
});

Expand Down Expand Up @@ -696,7 +697,7 @@ describe('RouterBuilder', () => {
.withLayout(Server)
.build();

expect(routes).to.be.deepEqualIgnoreUndefined([
expect(routes).to.be.to.looseDeepEqual([
{
handle: {
ignoreFallback: true,
Expand Down Expand Up @@ -830,6 +831,22 @@ describe('RouterBuilder', () => {
});

describe('issues', () => {
function removeUndefined(obj: unknown): unknown {
if (Array.isArray(obj)) {
return obj.map((v) => (typeof v === 'object' ? removeUndefined(v) : v));
}

if (obj != null && typeof obj === 'object') {
return Object.fromEntries(
Object.entries(obj)
.filter(([, value]) => value !== undefined)
.map(([key, value]) => [key, removeUndefined(value)]),
);
}

return obj;
}

it('#2954', () => {
const { routes } = new RouterConfigurationBuilder()
.withFileRoutes([
Expand Down Expand Up @@ -862,55 +879,42 @@ describe('RouterBuilder', () => {
.protect()
.build();

expect(routes).to.be.deepEqualIgnoreUndefined([
expect(routes).to.looseDeepEqual([
{
path: '',
handle: {
title: 'undefined',
},
children: [
{
path: 'login',
handle: {
title: 'undefined',
menu: { exclude: true },
menu: {
exclude: true,
},
flowLayout: false,
title: 'undefined',
},
},
{
path: '*',
element: <Server />,
},
{
index: true,
element: <Index />,
},
],
},
{
path: '',
handle: {
title: 'undefined',
},
},
{
path: '',
children: [
{
path: '',
element: <NextTest />,
handle: {
menu: { order: 0 },
title: 'Public view',
element: NextTest,
},
},
{
path: '*',
element: <Server />,
},
{
index: true,
element: <Index />,
},
{ path: '*', element: <Server /> },
],
handle: { title: 'undefined' },
},
{ path: '*', element: <Server /> },
{ index: true, element: <Index /> },
]);
});
});
Expand Down
Loading

0 comments on commit 127e663

Please sign in to comment.