Skip to content

Commit 0d2acbb

Browse files
alan-agius4alan-agius4
committed
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
1 parent f5dc745 commit 0d2acbb

File tree

4 files changed

+220
-159
lines changed

4 files changed

+220
-159
lines changed

‎packages/angular/ssr/src/routes/ng-routes.ts‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -458,6 +458,13 @@ function buildServerConfigRouteTree({ routes, appShellRoute }: ServerRoutesConfi
458458
continue;
459459
}
460460

461+
if (path.includes('**') && 'getPrerenderParams' in metadata) {
462+
errors.push(
463+
`Invalid '${path}' route configuration: 'getPrerenderParams' cannot be used with a '**' route.`,
464+
);
465+
continue;
466+
}
467+
461468
serverConfigRouteTree.insert(path, metadata);
462469
}
463470

‎packages/angular/ssr/src/routes/route-tree.ts‎

Lines changed: 35 additions & 55 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@
66
* found in the LICENSE file at https://angular.dev/license
77
*/
88

9-
import { addLeadingSlash, stripTrailingSlash } from '../utils/url';
9+
import { addLeadingSlash } from '../utils/url';
1010
import { RenderMode } from './route-config';
1111

1212
/**
@@ -222,7 +222,7 @@ export class RouteTree<AdditionalMetadata extends Record<string, unknown> = {}>
222222
* @returns An array of path segments.
223223
*/
224224
private getPathSegments(route: string): string[] {
225-
return stripTrailingSlash(route).split('/');
225+
return route.split('/').filter(Boolean);
226226
}
227227

228228
/**
@@ -232,74 +232,54 @@ export class RouteTree<AdditionalMetadata extends Record<string, unknown> = {}>
232232
* This function prioritizes exact segment matches first, followed by wildcard matches (`*`),
233233
* and finally deep wildcard matches (`**`) that consume all segments.
234234
*
235-
* @param remainingSegments - The remaining segments of the route path to match.
236-
* @param node - The current node in the route tree to start traversal from.
235+
* @param segments - The array of route path segments to match against the route tree.
236+
* @param node - The current node in the route tree to start traversal from. Defaults to the root node.
237+
* @param currentIndex - The index of the segment in `remainingSegments` currently being matched.
238+
* Defaults to `0` (the first segment).
237239
*
238240
* @returns The node that best matches the remaining segments or `undefined` if no match is found.
239241
*/
240242
private traverseBySegments(
241-
remainingSegments: string[],
243+
segments: string[],
242244
node = this.root,
245+
currentIndex = 0,
243246
): RouteTreeNode<AdditionalMetadata> | undefined {
244-
const { metadata, children } = node;
245-
246-
// If there are no remaining segments and the node has metadata, return this node
247-
if (!remainingSegments.length) {
248-
return metadata ? node : node.children.get('**');
247+
if (currentIndex >= segments.length) {
248+
if (node.metadata) {
249+
return node;
250+
} else if (node.children.size) {
251+
return this.traverseBySegments([''], node);
252+
} else {
253+
return undefined;
254+
}
249255
}
250256

251-
// If the node has no children, end the traversal
252-
if (!children.size) {
253-
return;
257+
if (!node.children.size) {
258+
return undefined;
254259
}
255260

256-
const [segment, ...restSegments] = remainingSegments;
257-
let currentBestMatchNode: RouteTreeNode<AdditionalMetadata> | undefined;
258-
259-
// 1. Exact segment match
260-
const exactMatchNode = node.children.get(segment);
261-
currentBestMatchNode = this.getHigherPriorityNode(
262-
currentBestMatchNode,
263-
this.traverseBySegments(restSegments, exactMatchNode),
264-
);
265-
266-
// 2. Wildcard segment match (`*`)
267-
const wildcardNode = node.children.get('*');
268-
currentBestMatchNode = this.getHigherPriorityNode(
269-
currentBestMatchNode,
270-
this.traverseBySegments(restSegments, wildcardNode),
271-
);
272-
273-
// 3. Deep wildcard segment match (`**`)
274-
const deepWildcardNode = node.children.get('**');
275-
currentBestMatchNode = this.getHigherPriorityNode(currentBestMatchNode, deepWildcardNode);
276-
277-
return currentBestMatchNode;
278-
}
261+
const segment = segments[currentIndex];
279262

280-
/**
281-
* Compares two nodes and returns the node with higher priority based on insertion index.
282-
* A node with a lower insertion index is prioritized as it was defined earlier.
283-
*
284-
* @param currentBestMatchNode - The current best match node.
285-
* @param candidateNode - The node being evaluated for higher priority based on insertion index.
286-
* @returns The node with higher priority (i.e., lower insertion index). If one of the nodes is `undefined`, the other node is returned.
287-
*/
288-
private getHigherPriorityNode(
289-
currentBestMatchNode: RouteTreeNode<AdditionalMetadata> | undefined,
290-
candidateNode: RouteTreeNode<AdditionalMetadata> | undefined,
291-
): RouteTreeNode<AdditionalMetadata> | undefined {
292-
if (!candidateNode) {
293-
return currentBestMatchNode;
263+
// 1. Attempt exact match with the current segment.
264+
const exactMatch = node.children.get(segment);
265+
if (exactMatch) {
266+
const match = this.traverseBySegments(segments, exactMatch, currentIndex + 1);
267+
if (match) {
268+
return match;
269+
}
294270
}
295271

296-
if (!currentBestMatchNode) {
297-
return candidateNode;
272+
// 2. Attempt wildcard match ('*').
273+
const wildcardMatch = node.children.get('*');
274+
if (wildcardMatch) {
275+
const match = this.traverseBySegments(segments, wildcardMatch, currentIndex + 1);
276+
if (match) {
277+
return match;
278+
}
298279
}
299280

300-
return candidateNode.insertionIndex < currentBestMatchNode.insertionIndex
301-
? candidateNode
302-
: currentBestMatchNode;
281+
// 3. Attempt double wildcard match ('**').
282+
return node.children.get('**');
303283
}
304284

305285
/**

‎packages/angular/ssr/test/routes/ng-routes_spec.ts‎

Lines changed: 96 additions & 74 deletions
Original file line numberDiff line numberDiff line change
@@ -52,19 +52,101 @@ describe('extractRoutesAndCreateRouteTree', () => {
5252
]);
5353
});
5454

55-
it('should handle invalid route configuration path', async () => {
56-
setAngularAppTestingManifest(
57-
[{ path: 'home', component: DummyComponent }],
58-
[
59-
// This path starts with a slash, which should trigger an error
60-
{ path: '/invalid', renderMode: RenderMode.Client },
61-
],
62-
);
55+
describe('route configuration validation', () => {
56+
it(`should error when a route starts with a '/'`, async () => {
57+
setAngularAppTestingManifest(
58+
[{ path: 'home', component: DummyComponent }],
59+
[
60+
// This path starts with a slash, which should trigger an error
61+
{ path: '/invalid', renderMode: RenderMode.Client },
62+
],
63+
);
6364

64-
const { errors } = await extractRoutesAndCreateRouteTree({ url });
65-
expect(errors[0]).toContain(
66-
`Invalid '/invalid' route configuration: the path cannot start with a slash.`,
67-
);
65+
const { errors } = await extractRoutesAndCreateRouteTree({ url });
66+
expect(errors[0]).toContain(
67+
`Invalid '/invalid' route configuration: the path cannot start with a slash.`,
68+
);
69+
});
70+
71+
it("should error when 'getPrerenderParams' is used with a '**' route", async () => {
72+
setAngularAppTestingManifest(
73+
[{ path: 'home', component: DummyComponent }],
74+
[
75+
{
76+
path: '**',
77+
renderMode: RenderMode.Prerender,
78+
getPrerenderParams() {
79+
return Promise.resolve([]);
80+
},
81+
},
82+
],
83+
);
84+
85+
const { errors } = await extractRoutesAndCreateRouteTree({ url });
86+
expect(errors[0]).toContain(
87+
"Invalid '**' route configuration: 'getPrerenderParams' cannot be used with a '**' route.",
88+
);
89+
});
90+
91+
it(`should not error when a catch-all route didn't match any Angular route.`, async () => {
92+
setAngularAppTestingManifest(
93+
[{ path: 'home', component: DummyComponent }],
94+
[
95+
{ path: 'home', renderMode: RenderMode.Server },
96+
{ path: '**', renderMode: RenderMode.Server },
97+
],
98+
);
99+
100+
const { errors } = await extractRoutesAndCreateRouteTree({
101+
url,
102+
invokeGetPrerenderParams: false,
103+
includePrerenderFallbackRoutes: false,
104+
});
105+
106+
expect(errors).toHaveSize(0);
107+
});
108+
109+
it('should error when a route is not defined in the server routing configuration', async () => {
110+
setAngularAppTestingManifest(
111+
[{ path: 'home', component: DummyComponent }],
112+
[
113+
{ path: 'home', renderMode: RenderMode.Server },
114+
{ path: 'invalid', renderMode: RenderMode.Server },
115+
],
116+
);
117+
118+
const { errors } = await extractRoutesAndCreateRouteTree({
119+
url,
120+
invokeGetPrerenderParams: false,
121+
includePrerenderFallbackRoutes: false,
122+
});
123+
124+
expect(errors).toHaveSize(1);
125+
expect(errors[0]).toContain(
126+
`The 'invalid' server route does not match any routes defined in the Angular routing configuration`,
127+
);
128+
});
129+
130+
it('should error when a server route is not defined in the Angular routing configuration', async () => {
131+
setAngularAppTestingManifest(
132+
[
133+
{ path: 'home', component: DummyComponent },
134+
{ path: 'invalid', component: DummyComponent },
135+
],
136+
[{ path: 'home', renderMode: RenderMode.Server }],
137+
);
138+
139+
const { errors } = await extractRoutesAndCreateRouteTree({
140+
url,
141+
invokeGetPrerenderParams: false,
142+
includePrerenderFallbackRoutes: false,
143+
});
144+
145+
expect(errors).toHaveSize(1);
146+
expect(errors[0]).toContain(
147+
`The 'invalid' route does not match any route defined in the server routing configuration`,
148+
);
149+
});
68150
});
69151

70152
describe('when `invokeGetPrerenderParams` is true', () => {
@@ -201,7 +283,7 @@ describe('extractRoutesAndCreateRouteTree', () => {
201283
],
202284
[
203285
{
204-
path: ':param',
286+
path: ':param/*',
205287
renderMode: RenderMode.Prerender,
206288
async getPrerenderParams() {
207289
return [{ param: 'some' }];
@@ -216,6 +298,7 @@ describe('extractRoutesAndCreateRouteTree', () => {
216298
invokeGetPrerenderParams: true,
217299
includePrerenderFallbackRoutes: true,
218300
});
301+
219302
expect(errors).toHaveSize(0);
220303
expect(routeTree.toObject()).toEqual([
221304
{ route: '/', renderMode: RenderMode.Prerender, redirectTo: '/some' },
@@ -318,7 +401,6 @@ describe('extractRoutesAndCreateRouteTree', () => {
318401

319402
const { routeTree, errors } = await extractRoutesAndCreateRouteTree({
320403
url,
321-
322404
invokeGetPrerenderParams: true,
323405
includePrerenderFallbackRoutes: false,
324406
});
@@ -374,66 +456,6 @@ describe('extractRoutesAndCreateRouteTree', () => {
374456
]);
375457
});
376458

377-
it(`should not error when a catch-all route didn't match any Angular route.`, async () => {
378-
setAngularAppTestingManifest(
379-
[{ path: 'home', component: DummyComponent }],
380-
[
381-
{ path: 'home', renderMode: RenderMode.Server },
382-
{ path: '**', renderMode: RenderMode.Server },
383-
],
384-
);
385-
386-
const { errors } = await extractRoutesAndCreateRouteTree({
387-
url,
388-
invokeGetPrerenderParams: false,
389-
includePrerenderFallbackRoutes: false,
390-
});
391-
392-
expect(errors).toHaveSize(0);
393-
});
394-
395-
it('should error when a route is not defined in the server routing configuration', async () => {
396-
setAngularAppTestingManifest(
397-
[{ path: 'home', component: DummyComponent }],
398-
[
399-
{ path: 'home', renderMode: RenderMode.Server },
400-
{ path: 'invalid', renderMode: RenderMode.Server },
401-
],
402-
);
403-
404-
const { errors } = await extractRoutesAndCreateRouteTree({
405-
url,
406-
invokeGetPrerenderParams: false,
407-
includePrerenderFallbackRoutes: false,
408-
});
409-
410-
expect(errors).toHaveSize(1);
411-
expect(errors[0]).toContain(
412-
`The 'invalid' server route does not match any routes defined in the Angular routing configuration`,
413-
);
414-
});
415-
416-
it('should error when a server route is not defined in the Angular routing configuration', async () => {
417-
setAngularAppTestingManifest(
418-
[
419-
{ path: 'home', component: DummyComponent },
420-
{ path: 'invalid', component: DummyComponent },
421-
],
422-
[{ path: 'home', renderMode: RenderMode.Server }],
423-
);
424-
425-
const { errors } = await extractRoutesAndCreateRouteTree({
426-
url,
427-
invokeGetPrerenderParams: false,
428-
includePrerenderFallbackRoutes: false,
429-
});
430-
431-
expect(errors).toHaveSize(1);
432-
expect(errors[0]).toContain(
433-
`The 'invalid' route does not match any route defined in the server routing configuration`,
434-
);
435-
});
436-
437459
it('should use wildcard configuration when no Angular routes are defined', async () => {
438460
setAngularAppTestingManifest([], [{ path: '**', renderMode: RenderMode.Server, status: 201 }]);
439461

0 commit comments

Comments
 (0)