From 4258fa0cb9a1fb459e8ea5ac9daec02f4e7dc9c9 Mon Sep 17 00:00:00 2001 From: Alan Agius Date: Fri, 24 Jan 2025 08:19:28 +0000 Subject: [PATCH] fix(@angular/ssr): enhance dynamic route matching for better performance and accuracy Updated route matching logic to prioritize closest matches, improving the accuracy of dynamic route resolution. Also we optimized performance by eliminating unnecessary recursive checks, reducing overhead during route matching. Closes #29452 --- packages/angular/ssr/src/routes/ng-routes.ts | 23 +++ packages/angular/ssr/src/routes/route-tree.ts | 102 +++------- .../angular/ssr/test/routes/ng-routes_spec.ts | 188 +++++++++++------- .../ssr/test/routes/route-tree_spec.ts | 63 +++--- 4 files changed, 200 insertions(+), 176 deletions(-) diff --git a/packages/angular/ssr/src/routes/ng-routes.ts b/packages/angular/ssr/src/routes/ng-routes.ts index b6554610dd9a..2e104054a814 100644 --- a/packages/angular/ssr/src/routes/ng-routes.ts +++ b/packages/angular/ssr/src/routes/ng-routes.ts @@ -199,6 +199,7 @@ async function* traverseRoutesConfig(options: { if (metadata.renderMode === RenderMode.Prerender) { // Handle SSG routes yield* handleSSGRoute( + serverConfigRouteTree, typeof redirectTo === 'string' ? redirectTo : undefined, metadata, parentInjector, @@ -301,6 +302,7 @@ function appendPreloadToMetadata( * Handles SSG (Static Site Generation) routes by invoking `getPrerenderParams` and yielding * all parameterized paths, returning any errors encountered. * + * @param serverConfigRouteTree - The tree representing the server's routing setup. * @param redirectTo - Optional path to redirect to, if specified. * @param metadata - The metadata associated with the route tree node. * @param parentInjector - The dependency injection container for the parent route. @@ -309,6 +311,7 @@ function appendPreloadToMetadata( * @returns An async iterable iterator that yields route tree node metadata for each SSG path or errors. */ async function* handleSSGRoute( + serverConfigRouteTree: RouteTree | undefined, redirectTo: string | undefined, metadata: ServerConfigRouteTreeNodeMetadata, parentInjector: Injector, @@ -354,6 +357,19 @@ async function* handleSSGRoute( return; } + if (serverConfigRouteTree) { + // Automatically resolve dynamic parameters for nested routes. + const catchAllRoutePath = joinUrlParts(currentRoutePath, '**'); + const match = serverConfigRouteTree.match(catchAllRoutePath); + if (match && match.renderMode === RenderMode.Prerender && !('getPrerenderParams' in match)) { + serverConfigRouteTree.insert(catchAllRoutePath, { + ...match, + presentInClientRouter: true, + getPrerenderParams, + }); + } + } + const parameters = await runInInjectionContext(parentInjector, () => getPrerenderParams()); try { for (const params of parameters) { @@ -458,6 +474,13 @@ function buildServerConfigRouteTree({ routes, appShellRoute }: ServerRoutesConfi continue; } + if (path.includes('*') && 'getPrerenderParams' in metadata) { + errors.push( + `Invalid '${path}' route configuration: 'getPrerenderParams' cannot be used with a '*' or '**' route.`, + ); + continue; + } + serverConfigRouteTree.insert(path, metadata); } diff --git a/packages/angular/ssr/src/routes/route-tree.ts b/packages/angular/ssr/src/routes/route-tree.ts index 6a0360679a0d..64d9a0fdfef2 100644 --- a/packages/angular/ssr/src/routes/route-tree.ts +++ b/packages/angular/ssr/src/routes/route-tree.ts @@ -6,7 +6,7 @@ * found in the LICENSE file at https://angular.dev/license */ -import { addLeadingSlash, stripTrailingSlash } from '../utils/url'; +import { addLeadingSlash } from '../utils/url'; import { RenderMode } from './route-config'; /** @@ -78,13 +78,6 @@ export interface RouteTreeNodeMetadata { * The `AdditionalMetadata` type parameter allows for extending the node metadata with custom data. */ interface RouteTreeNode> { - /** - * The index indicating the order in which the route was inserted into the tree. - * This index helps determine the priority of routes during matching, with lower indexes - * indicating earlier inserted routes. - */ - insertionIndex: number; - /** * A map of child nodes, keyed by their corresponding route segment or wildcard. */ @@ -110,13 +103,6 @@ export class RouteTree = {}> */ private readonly root = this.createEmptyRouteTreeNode(); - /** - * A counter that tracks the order of route insertion. - * This ensures that routes are matched in the order they were defined, - * with earlier routes taking precedence. - */ - private insertionIndexCounter = 0; - /** * Inserts a new route into the route tree. * The route is broken down into segments, and each segment is added to the tree. @@ -134,7 +120,6 @@ export class RouteTree = {}> // Replace parameterized segments (e.g., :id) with a wildcard (*) for matching const normalizedSegment = segment[0] === ':' ? '*' : segment; let childNode = node.children.get(normalizedSegment); - if (!childNode) { childNode = this.createEmptyRouteTreeNode(); node.children.set(normalizedSegment, childNode); @@ -149,8 +134,6 @@ export class RouteTree = {}> ...metadata, route: addLeadingSlash(normalizedSegments.join('/')), }; - - node.insertionIndex = this.insertionIndexCounter++; } /** @@ -222,7 +205,7 @@ export class RouteTree = {}> * @returns An array of path segments. */ private getPathSegments(route: string): string[] { - return stripTrailingSlash(route).split('/'); + return route.split('/').filter(Boolean); } /** @@ -232,74 +215,48 @@ export class RouteTree = {}> * This function prioritizes exact segment matches first, followed by wildcard matches (`*`), * and finally deep wildcard matches (`**`) that consume all segments. * - * @param remainingSegments - The remaining segments of the route path to match. - * @param node - The current node in the route tree to start traversal from. + * @param segments - The array of route path segments to match against the route tree. + * @param node - The current node in the route tree to start traversal from. Defaults to the root node. + * @param currentIndex - The index of the segment in `remainingSegments` currently being matched. + * Defaults to `0` (the first segment). * * @returns The node that best matches the remaining segments or `undefined` if no match is found. */ private traverseBySegments( - remainingSegments: string[], + segments: string[], node = this.root, + currentIndex = 0, ): RouteTreeNode | undefined { - const { metadata, children } = node; - - // If there are no remaining segments and the node has metadata, return this node - if (!remainingSegments.length) { - return metadata ? node : node.children.get('**'); + if (currentIndex >= segments.length) { + return node.metadata ? node : node.children.get('**'); } - // If the node has no children, end the traversal - if (!children.size) { - return; + if (!node.children.size) { + return undefined; } - const [segment, ...restSegments] = remainingSegments; - let currentBestMatchNode: RouteTreeNode | undefined; - - // 1. Exact segment match - const exactMatchNode = node.children.get(segment); - currentBestMatchNode = this.getHigherPriorityNode( - currentBestMatchNode, - this.traverseBySegments(restSegments, exactMatchNode), - ); + const segment = segments[currentIndex]; - // 2. Wildcard segment match (`*`) - const wildcardNode = node.children.get('*'); - currentBestMatchNode = this.getHigherPriorityNode( - currentBestMatchNode, - this.traverseBySegments(restSegments, wildcardNode), - ); - - // 3. Deep wildcard segment match (`**`) - const deepWildcardNode = node.children.get('**'); - currentBestMatchNode = this.getHigherPriorityNode(currentBestMatchNode, deepWildcardNode); - - return currentBestMatchNode; - } - - /** - * Compares two nodes and returns the node with higher priority based on insertion index. - * A node with a lower insertion index is prioritized as it was defined earlier. - * - * @param currentBestMatchNode - The current best match node. - * @param candidateNode - The node being evaluated for higher priority based on insertion index. - * @returns The node with higher priority (i.e., lower insertion index). If one of the nodes is `undefined`, the other node is returned. - */ - private getHigherPriorityNode( - currentBestMatchNode: RouteTreeNode | undefined, - candidateNode: RouteTreeNode | undefined, - ): RouteTreeNode | undefined { - if (!candidateNode) { - return currentBestMatchNode; + // 1. Attempt exact match with the current segment. + const exactMatch = node.children.get(segment); + if (exactMatch) { + const match = this.traverseBySegments(segments, exactMatch, currentIndex + 1); + if (match) { + return match; + } } - if (!currentBestMatchNode) { - return candidateNode; + // 2. Attempt wildcard match ('*'). + const wildcardMatch = node.children.get('*'); + if (wildcardMatch) { + const match = this.traverseBySegments(segments, wildcardMatch, currentIndex + 1); + if (match) { + return match; + } } - return candidateNode.insertionIndex < currentBestMatchNode.insertionIndex - ? candidateNode - : currentBestMatchNode; + // 3. Attempt double wildcard match ('**'). + return node.children.get('**'); } /** @@ -310,7 +267,6 @@ export class RouteTree = {}> */ private createEmptyRouteTreeNode(): RouteTreeNode { return { - insertionIndex: -1, children: new Map(), }; } diff --git a/packages/angular/ssr/test/routes/ng-routes_spec.ts b/packages/angular/ssr/test/routes/ng-routes_spec.ts index 81b949cd098e..44d246e60a2f 100644 --- a/packages/angular/ssr/test/routes/ng-routes_spec.ts +++ b/packages/angular/ssr/test/routes/ng-routes_spec.ts @@ -52,19 +52,121 @@ describe('extractRoutesAndCreateRouteTree', () => { ]); }); - it('should handle invalid route configuration path', async () => { - setAngularAppTestingManifest( - [{ path: 'home', component: DummyComponent }], - [ - // This path starts with a slash, which should trigger an error - { path: '/invalid', renderMode: RenderMode.Client }, - ], - ); + describe('route configuration validation', () => { + it(`should error when a route starts with a '/'`, async () => { + setAngularAppTestingManifest( + [{ path: 'home', component: DummyComponent }], + [ + // This path starts with a slash, which should trigger an error + { path: '/invalid', renderMode: RenderMode.Client }, + ], + ); - const { errors } = await extractRoutesAndCreateRouteTree({ url }); - expect(errors[0]).toContain( - `Invalid '/invalid' route configuration: the path cannot start with a slash.`, - ); + const { errors } = await extractRoutesAndCreateRouteTree({ url }); + expect(errors[0]).toContain( + `Invalid '/invalid' route configuration: the path cannot start with a slash.`, + ); + }); + + it("should error when 'getPrerenderParams' is used with a '**' route", async () => { + setAngularAppTestingManifest( + [{ path: 'home', component: DummyComponent }], + [ + { + path: '**', + renderMode: RenderMode.Prerender, + getPrerenderParams() { + return Promise.resolve([]); + }, + }, + ], + ); + + const { errors } = await extractRoutesAndCreateRouteTree({ url }); + expect(errors[0]).toContain( + "Invalid '**' route configuration: 'getPrerenderParams' cannot be used with a '*' or '**' route.", + ); + }); + + it("should error when 'getPrerenderParams' is used with a '*' route", async () => { + setAngularAppTestingManifest( + [{ path: 'invalid/:id', component: DummyComponent }], + [ + { + path: 'invalid/*', + renderMode: RenderMode.Prerender, + getPrerenderParams() { + return Promise.resolve([]); + }, + }, + ], + ); + + const { errors } = await extractRoutesAndCreateRouteTree({ url }); + expect(errors[0]).toContain( + "Invalid 'invalid/*' route configuration: 'getPrerenderParams' cannot be used with a '*' or '**' route.", + ); + }); + + it(`should not error when a catch-all route didn't match any Angular route.`, async () => { + setAngularAppTestingManifest( + [{ path: 'home', component: DummyComponent }], + [ + { path: 'home', renderMode: RenderMode.Server }, + { path: '**', renderMode: RenderMode.Server }, + ], + ); + + const { errors } = await extractRoutesAndCreateRouteTree({ + url, + invokeGetPrerenderParams: false, + includePrerenderFallbackRoutes: false, + }); + + expect(errors).toHaveSize(0); + }); + + it('should error when a route is not defined in the server routing configuration', async () => { + setAngularAppTestingManifest( + [{ path: 'home', component: DummyComponent }], + [ + { path: 'home', renderMode: RenderMode.Server }, + { path: 'invalid', renderMode: RenderMode.Server }, + ], + ); + + const { errors } = await extractRoutesAndCreateRouteTree({ + url, + invokeGetPrerenderParams: false, + includePrerenderFallbackRoutes: false, + }); + + expect(errors).toHaveSize(1); + expect(errors[0]).toContain( + `The 'invalid' server route does not match any routes defined in the Angular routing configuration`, + ); + }); + + it('should error when a server route is not defined in the Angular routing configuration', async () => { + setAngularAppTestingManifest( + [ + { path: 'home', component: DummyComponent }, + { path: 'invalid', component: DummyComponent }, + ], + [{ path: 'home', renderMode: RenderMode.Server }], + ); + + const { errors } = await extractRoutesAndCreateRouteTree({ + url, + invokeGetPrerenderParams: false, + includePrerenderFallbackRoutes: false, + }); + + expect(errors).toHaveSize(1); + expect(errors[0]).toContain( + `The 'invalid' route does not match any route defined in the server routing configuration`, + ); + }); }); describe('when `invokeGetPrerenderParams` is true', () => { @@ -216,6 +318,7 @@ describe('extractRoutesAndCreateRouteTree', () => { invokeGetPrerenderParams: true, includePrerenderFallbackRoutes: true, }); + expect(errors).toHaveSize(0); expect(routeTree.toObject()).toEqual([ { route: '/', renderMode: RenderMode.Prerender, redirectTo: '/some' }, @@ -318,7 +421,6 @@ describe('extractRoutesAndCreateRouteTree', () => { const { routeTree, errors } = await extractRoutesAndCreateRouteTree({ url, - invokeGetPrerenderParams: true, includePrerenderFallbackRoutes: false, }); @@ -374,66 +476,6 @@ describe('extractRoutesAndCreateRouteTree', () => { ]); }); - it(`should not error when a catch-all route didn't match any Angular route.`, async () => { - setAngularAppTestingManifest( - [{ path: 'home', component: DummyComponent }], - [ - { path: 'home', renderMode: RenderMode.Server }, - { path: '**', renderMode: RenderMode.Server }, - ], - ); - - const { errors } = await extractRoutesAndCreateRouteTree({ - url, - invokeGetPrerenderParams: false, - includePrerenderFallbackRoutes: false, - }); - - expect(errors).toHaveSize(0); - }); - - it('should error when a route is not defined in the server routing configuration', async () => { - setAngularAppTestingManifest( - [{ path: 'home', component: DummyComponent }], - [ - { path: 'home', renderMode: RenderMode.Server }, - { path: 'invalid', renderMode: RenderMode.Server }, - ], - ); - - const { errors } = await extractRoutesAndCreateRouteTree({ - url, - invokeGetPrerenderParams: false, - includePrerenderFallbackRoutes: false, - }); - - expect(errors).toHaveSize(1); - expect(errors[0]).toContain( - `The 'invalid' server route does not match any routes defined in the Angular routing configuration`, - ); - }); - - it('should error when a server route is not defined in the Angular routing configuration', async () => { - setAngularAppTestingManifest( - [ - { path: 'home', component: DummyComponent }, - { path: 'invalid', component: DummyComponent }, - ], - [{ path: 'home', renderMode: RenderMode.Server }], - ); - - const { errors } = await extractRoutesAndCreateRouteTree({ - url, - invokeGetPrerenderParams: false, - includePrerenderFallbackRoutes: false, - }); - - expect(errors).toHaveSize(1); - expect(errors[0]).toContain( - `The 'invalid' route does not match any route defined in the server routing configuration`, - ); - }); - it('should use wildcard configuration when no Angular routes are defined', async () => { setAngularAppTestingManifest([], [{ path: '**', renderMode: RenderMode.Server, status: 201 }]); diff --git a/packages/angular/ssr/test/routes/route-tree_spec.ts b/packages/angular/ssr/test/routes/route-tree_spec.ts index e800edf51792..fa4e120f6c56 100644 --- a/packages/angular/ssr/test/routes/route-tree_spec.ts +++ b/packages/angular/ssr/test/routes/route-tree_spec.ts @@ -7,7 +7,7 @@ */ import { RenderMode } from '../../src/routes/route-config'; -import { RouteTree } from '../../src/routes/route-tree'; +import { RouteTree, RouteTreeNodeMetadataWithoutRoute } from '../../src/routes/route-tree'; describe('RouteTree', () => { let routeTree: RouteTree; @@ -118,33 +118,6 @@ describe('RouteTree', () => { const newRouteTree = RouteTree.fromObject(routeTreeObj); expect(newRouteTree.match('/any-path')).toBeUndefined(); }); - - it('should preserve insertion order when converting to and from object', () => { - routeTree.insert('/first', { renderMode: RenderMode.Server }); - routeTree.insert('/:id', { renderMode: RenderMode.Server }); - routeTree.insert('/second', { renderMode: RenderMode.Server }); - - const routeTreeObj = routeTree.toObject(); - expect(routeTreeObj).toEqual([ - { route: '/first', renderMode: RenderMode.Server }, - { route: '/*', renderMode: RenderMode.Server }, - { route: '/second', renderMode: RenderMode.Server }, - ]); - - const newRouteTree = RouteTree.fromObject(routeTreeObj); - expect(newRouteTree.match('/first')).toEqual({ - route: '/first', - renderMode: RenderMode.Server, - }); - expect(newRouteTree.match('/second')).toEqual({ - route: '/*', - renderMode: RenderMode.Server, - }); - expect(newRouteTree.match('/third')).toEqual({ - route: '/*', - renderMode: RenderMode.Server, - }); - }); }); describe('match', () => { @@ -204,12 +177,42 @@ describe('RouteTree', () => { }); }); - it('should prioritize earlier insertions in case of conflicts', () => { + it('matches routes correctly with exact, wildcard, and double wildcard patterns', () => { + const meta: RouteTreeNodeMetadataWithoutRoute = { renderMode: RenderMode.Client }; + + // Set up the route tree with various route configurations + routeTree.insert('/', meta); + routeTree.insert('/*', meta); + routeTree.insert('/*/*', meta); + routeTree.insert('/**', meta); + routeTree.insert('/blog', meta); + routeTree.insert('/blog/*', meta); + + // Test route matches for exact routes + expect(routeTree.match('/')?.route).toBe('/'); + expect(routeTree.match('/blog')?.route).toBe('/blog'); + + // Test route matches for single wildcard routes + expect(routeTree.match('/something')?.route).toBe('/*'); + expect(routeTree.match('/blog/article')?.route).toBe('/blog/*'); + + // Test route matches for multiple wildcard routes + expect(routeTree.match('/something/another')?.route).toBe('/*/*'); + + // Additional test for wildcard fallback + expect(routeTree.match('/something')?.route).toBe('/*'); + + // Test route matches for catch all double wildcard routes + expect(routeTree.match('/something/another/nested')?.route).toBe('/**'); + }); + + it('should prefer exact matches in case of conflicts', () => { routeTree.insert('/blog/*', { renderMode: RenderMode.Server }); routeTree.insert('/blog/article', { redirectTo: 'blog', renderMode: RenderMode.Server }); expect(routeTree.match('/blog/article')).toEqual({ - route: '/blog/*', + route: '/blog/article', + redirectTo: 'blog', renderMode: RenderMode.Server, }); });