From e2d86cf4b985841e30f23439869da428711e8baa Mon Sep 17 00:00:00 2001 From: Mikael Grankvist Date: Fri, 18 Oct 2024 15:19:38 +0300 Subject: [PATCH 1/2] fix: handle child layouts Handle child layouts depending on if they have flowLayout true or false. Fixes all layouts having flowLayout if the parent layout or one child has flowLayout true. Fixes #20261 --- .../src/runtime/RouterConfigurationBuilder.ts | 87 ++++++++++++++++--- .../RouterConfigurationBuilder.spec.tsx | 46 ++++++++-- 2 files changed, 114 insertions(+), 19 deletions(-) diff --git a/packages/ts/file-router/src/runtime/RouterConfigurationBuilder.ts b/packages/ts/file-router/src/runtime/RouterConfigurationBuilder.ts index e49fef96b9..a1589b6e39 100644 --- a/packages/ts/file-router/src/runtime/RouterConfigurationBuilder.ts +++ b/packages/ts/file-router/src/runtime/RouterConfigurationBuilder.ts @@ -1,4 +1,5 @@ /* eslint-disable @typescript-eslint/consistent-type-assertions */ +import * as Console from 'node:console'; import { protectRoute } from '@vaadin/hilla-react-auth'; import { type ComponentType, createElement } from 'react'; import { @@ -153,15 +154,87 @@ export class RouterConfigurationBuilder { * @param layoutComponent - layout component to use, usually Flow */ withLayout(layoutComponent: ComponentType): this { + function checkFlowLayout(route: RouteObject): boolean { + // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access + let flowLayout = typeof route.handle === 'object' && 'flowLayout' in route.handle && route.handle.flowLayout; + // Check children if they have layout. If yes then parent should have layout also. + if (!flowLayout && route.children) { + flowLayout = route.children.filter((child) => checkFlowLayout(child)).length > 0; + } + return flowLayout; + } + + function removeNonFlowLayoutChildren(route: RouteObject, copy: boolean) { + const routeCopy: RouteObject = copy ? JSON.parse(JSON.stringify(route)) : route; + if (route.children) { + const children: RouteObject[] = []; + route.children.forEach((child) => + children.push(removeNonFlowLayoutChildren(child, child.children !== undefined)), + ); + routeCopy.children = []; + while (children.length > 0) { + const child = children.pop(); + if (child && (checkFlowLayout(child) || child.children)) { + routeCopy.children.push(child); + } + } + } + return routeCopy; + } + + function removeFlowLayoutChildren(route: RouteObject, copy: boolean) { + const routeCopy: RouteObject = copy ? JSON.parse(JSON.stringify(route)) : route; + let flowLayout = true; + if (route.children) { + const children: RouteObject[] = []; + route.children.forEach((child) => children.push(removeFlowLayoutChildren(child, child.children !== undefined))); + routeCopy.children = []; + while (children.length > 0) { + const child = children.pop(); + if (child && (!checkFlowLayout(child) || child.children)) { + routeCopy.children.push(child); + flowLayout = false; + } + } + } + return routeCopy; + } + function applyLayouts(routes: readonly RouteObject[]): readonly RouteObject[] { if (routes.length === 0) { return routes; } - const nestedRoutes = routes.map((route) => route); + + // Remove all flowLayout: false children + const serverLayoutRoutes = routes.map((route) => + removeNonFlowLayoutChildren(route, route.children !== undefined), + ); + // Collect all flowLayout: false children to add as normal routes + const clientRoutes = routes + .map((route) => removeFlowLayoutChildren(route, route.children !== undefined)) + .filter( + (route) => + !( + (route.children && route.children.length === 0 && checkFlowLayout(route)) ?? + (!route.children && checkFlowLayout(route)) + ), + ); + if (clientRoutes.length > 0) { + return [ + { + element: createElement(layoutComponent), + children: serverLayoutRoutes, + handle: { + ignoreFallback: true, + }, + }, + ...clientRoutes, + ]; + } return [ { element: createElement(layoutComponent), - children: nestedRoutes, + children: serverLayoutRoutes, handle: { ignoreFallback: true, }, @@ -169,16 +242,6 @@ export class RouterConfigurationBuilder { ]; } - function checkFlowLayout(route: RouteObject): boolean { - // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access - let flowLayout = typeof route.handle === 'object' && 'flowLayout' in route.handle && route.handle.flowLayout; - // Check children if they have layout. If yes then parent should have layout also. - if (!flowLayout && route.children) { - flowLayout = route.children.filter((child) => checkFlowLayout(child)).length > 0; - } - return flowLayout; - } - this.#modifiers.push((routes: readonly RouteObject[] | undefined) => { if (!routes) { return routes; diff --git a/packages/ts/file-router/test/runtime/RouterConfigurationBuilder.spec.tsx b/packages/ts/file-router/test/runtime/RouterConfigurationBuilder.spec.tsx index e751c500e4..3892f80d6e 100644 --- a/packages/ts/file-router/test/runtime/RouterConfigurationBuilder.spec.tsx +++ b/packages/ts/file-router/test/runtime/RouterConfigurationBuilder.spec.tsx @@ -99,6 +99,9 @@ describe('RouterBuilder', () => { children: [ { path: '/child', + handle: { + flowLayout: true, + }, }, ], }, @@ -119,6 +122,9 @@ describe('RouterBuilder', () => { children: [ { path: '/child', + handle: { + flowLayout: true, + }, }, ], path: '/test', @@ -128,6 +134,9 @@ describe('RouterBuilder', () => { }, ], element: createElement(Server), + handle: { + ignoreFallback: true, + }, }, { children: [ @@ -176,6 +185,12 @@ describe('RouterBuilder', () => { flowLayout: true, }, }, + { + path: '/outside', + handle: { + flowLayout: false, + }, + }, ], }, { @@ -199,26 +214,22 @@ describe('RouterBuilder', () => { { children: [ { - path: '', handle: { flowLayout: true, }, + path: '/nested', }, { - path: '/nested', handle: { flowLayout: true, }, + path: '', }, ], path: 'nest', }, { - children: [ - { - path: '/child', - }, - ], + children: [], path: '/test', handle: { flowLayout: true, @@ -226,6 +237,27 @@ describe('RouterBuilder', () => { }, ], element: createElement(Server), + handle: { + ignoreFallback: true, + }, + }, + { + children: [ + { + path: '/outside', + handle: { + flowLayout: false, + }, + }, + ], + path: 'nest', + }, + { + children: [ + { + path: '/child', + }, + ], }, { children: [ From 67061e887402aa8b7284b5112e39c646e8faa68f Mon Sep 17 00:00:00 2001 From: Anton Platonov Date: Fri, 15 Nov 2024 13:31:16 +0200 Subject: [PATCH 2/2] refactor(file-router): flowLayout subtree implementation cleanup --- .../src/runtime/RouterConfigurationBuilder.ts | 177 ++++++++---------- .../RouterConfigurationBuilder.spec.tsx | 53 ++++-- 2 files changed, 116 insertions(+), 114 deletions(-) diff --git a/packages/ts/file-router/src/runtime/RouterConfigurationBuilder.ts b/packages/ts/file-router/src/runtime/RouterConfigurationBuilder.ts index a1589b6e39..d62a762560 100644 --- a/packages/ts/file-router/src/runtime/RouterConfigurationBuilder.ts +++ b/packages/ts/file-router/src/runtime/RouterConfigurationBuilder.ts @@ -1,5 +1,4 @@ /* eslint-disable @typescript-eslint/consistent-type-assertions */ -import * as Console from 'node:console'; import { protectRoute } from '@vaadin/hilla-react-auth'; import { type ComponentType, createElement } from 'react'; import { @@ -40,6 +39,18 @@ function createRouteEntry(route: T): readonly [key: string, return [`${route.path ?? ''}-${route.children ? 'n' : 'i'}`, route]; } +enum RouteHandleFlags { + FLOW_LAYOUT = 'flowLayout', + IGNORE_FALLBACK = 'ignoreFallback', +} + +function hasRouteHandleFlag( + route: RouteObject, + flag: T, +): route is { readonly handle: Readonly> } { + return typeof route.handle === 'object' && flag in route.handle && (route.handle as Record)[flag]; +} + /** * A builder for creating a Vaadin-specific router for React with * authentication and server routes support. @@ -120,8 +131,7 @@ export class RouterConfigurationBuilder { ]; this.update(fallbackRoutes, (original, added, children) => { - // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access - if (original && !original.handle?.ignoreFallback) { + if (original && !hasRouteHandleFlag(original, RouteHandleFlags.IGNORE_FALLBACK)) { if (!children) { return original; } @@ -154,107 +164,78 @@ export class RouterConfigurationBuilder { * @param layoutComponent - layout component to use, usually Flow */ withLayout(layoutComponent: ComponentType): this { - function checkFlowLayout(route: RouteObject): boolean { - // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access - let flowLayout = typeof route.handle === 'object' && 'flowLayout' in route.handle && route.handle.flowLayout; - // Check children if they have layout. If yes then parent should have layout also. - if (!flowLayout && route.children) { - flowLayout = route.children.filter((child) => checkFlowLayout(child)).length > 0; - } - return flowLayout; - } - - function removeNonFlowLayoutChildren(route: RouteObject, copy: boolean) { - const routeCopy: RouteObject = copy ? JSON.parse(JSON.stringify(route)) : route; - if (route.children) { - const children: RouteObject[] = []; - route.children.forEach((child) => - children.push(removeNonFlowLayoutChildren(child, child.children !== undefined)), - ); - routeCopy.children = []; - while (children.length > 0) { - const child = children.pop(); - if (child && (checkFlowLayout(child) || child.children)) { - routeCopy.children.push(child); - } - } - } - return routeCopy; - } - - function removeFlowLayoutChildren(route: RouteObject, copy: boolean) { - const routeCopy: RouteObject = copy ? JSON.parse(JSON.stringify(route)) : route; - let flowLayout = true; - if (route.children) { - const children: RouteObject[] = []; - route.children.forEach((child) => children.push(removeFlowLayoutChildren(child, child.children !== undefined))); - routeCopy.children = []; - while (children.length > 0) { - const child = children.pop(); - if (child && (!checkFlowLayout(child) || child.children)) { - routeCopy.children.push(child); - flowLayout = false; - } - } + this.#modifiers.push((originalRoutes: readonly RouteObject[] | undefined) => { + if (originalRoutes === undefined) { + return originalRoutes; } - return routeCopy; - } - - function applyLayouts(routes: readonly RouteObject[]): readonly RouteObject[] { - if (routes.length === 0) { - return routes; - } - - // Remove all flowLayout: false children - const serverLayoutRoutes = routes.map((route) => - removeNonFlowLayoutChildren(route, route.children !== undefined), - ); - // Collect all flowLayout: false children to add as normal routes - const clientRoutes = routes - .map((route) => removeFlowLayoutChildren(route, route.children !== undefined)) - .filter( - (route) => - !( - (route.children && route.children.length === 0 && checkFlowLayout(route)) ?? - (!route.children && checkFlowLayout(route)) + // Split the routes tree onto two subtrees with and without + // a server layout. + const [serverRoutesTree, clientRoutesTree]: [RouteObject[] | undefined, RouteObject[] | undefined] = + transformTree( + originalRoutes, + ( + routes: readonly RouteObject[], + next: (...nodes: readonly RouteObject[]) => [RouteObject[] | undefined, RouteObject[] | undefined], + ) => + // Split single routes list onto two filtered lists + routes.reduce<[RouteObject[] | undefined, RouteObject[] | undefined]>( + ([serverRoutesList, clientRoutesList], route) => { + if (hasRouteHandleFlag(route, RouteHandleFlags.FLOW_LAYOUT)) { + // Server layout is explicitly declared: move to the entire + // route to the server list, taking also all the children. + return [[...(serverRoutesList ?? []), route], clientRoutesList]; + } + if (!route.children?.length) { + // Leaf routes and empty layouts: move to the client list. + return [serverRoutesList, [...(clientRoutesList ?? []), route]]; + } + // Nested children: collect server and client subtrees, and + // copy the current route to either or both the server and + // the client lists with the respective subtree as children. + const [serverRouteSubtree, clientRouteSubtree] = next(...route.children); + return [ + serverRouteSubtree + ? [ + ...(serverRoutesList ?? []), + { + ...route, + children: serverRouteSubtree, + }, + ] + : serverRoutesList, + clientRouteSubtree + ? [ + ...(clientRoutesList ?? []), + { + ...route, + children: clientRouteSubtree, + }, + ] + : clientRoutesList, + ]; + }, + [undefined, undefined], ), ); - if (clientRoutes.length > 0) { - return [ - { - element: createElement(layoutComponent), - children: serverLayoutRoutes, - handle: { - ignoreFallback: true, - }, - }, - ...clientRoutes, - ]; - } + return [ - { - element: createElement(layoutComponent), - children: serverLayoutRoutes, - handle: { - ignoreFallback: true, - }, - }, + // The server subtree is wrapped with the server layout component, + // which applies the top-level server layout to all matches. + ...(serverRoutesTree + ? [ + { + element: createElement(layoutComponent), + children: serverRoutesTree, + handle: { + [RouteHandleFlags.IGNORE_FALLBACK]: true, + }, + }, + ] + : []), + // The client route subtree is preserved without wrapping. + ...(clientRoutesTree ?? []), ]; - } - - this.#modifiers.push((routes: readonly RouteObject[] | undefined) => { - if (!routes) { - return routes; - } - const withLayout = routes.filter((route) => checkFlowLayout(route)); - const allRoutes = routes.filter((route) => !withLayout.includes(route)); - const catchAll = [routes.find((route) => route.path === '*')].filter((route) => route !== undefined); - withLayout.push(...catchAll); // Add * fallback to all child routes - - allRoutes.unshift(...applyLayouts(withLayout)); - return allRoutes; }); - return this; } diff --git a/packages/ts/file-router/test/runtime/RouterConfigurationBuilder.spec.tsx b/packages/ts/file-router/test/runtime/RouterConfigurationBuilder.spec.tsx index 3892f80d6e..1c4d7eff33 100644 --- a/packages/ts/file-router/test/runtime/RouterConfigurationBuilder.spec.tsx +++ b/packages/ts/file-router/test/runtime/RouterConfigurationBuilder.spec.tsx @@ -191,6 +191,10 @@ describe('RouterBuilder', () => { flowLayout: false, }, }, + { + path: '/nested-empty-layout', + children: [], + }, ], }, { @@ -202,8 +206,16 @@ describe('RouterBuilder', () => { { path: '/child', }, + { + path: '/empty-layout', + children: [], + }, ], }, + { + path: '/empty-layout-outside', + children: [], + }, ]) .withLayout(Server) .build(); @@ -217,19 +229,27 @@ describe('RouterBuilder', () => { handle: { flowLayout: true, }, - path: '/nested', + path: '', }, { handle: { flowLayout: true, }, - path: '', + path: '/nested', }, ], path: 'nest', }, { - children: [], + children: [ + { + path: '/child', + }, + { + path: '/empty-layout', + children: [], + }, + ], path: '/test', handle: { flowLayout: true, @@ -244,29 +264,30 @@ describe('RouterBuilder', () => { { children: [ { - path: '/outside', - handle: { - flowLayout: false, - }, + path: '/test', + element:
Test
, }, ], - path: 'nest', + path: '', }, { children: [ { - path: '/child', + path: '/outside', + handle: { + flowLayout: false, + }, }, - ], - }, - { - children: [ { - path: '/test', - element:
Test
, + path: '/nested-empty-layout', + children: [], }, ], - path: '', + path: 'nest', + }, + { + path: '/empty-layout-outside', + children: [], }, ]); });