From c963dada1a1b511e72904ef7e3964275cfa4cd5e Mon Sep 17 00:00:00 2001 From: Alan Agius Date: Thu, 26 Sep 2024 09:59:20 +0000 Subject: [PATCH 1/2] fix(@angular/ssr): improve handling of route mismatches between Angular server routes and Angular router This commit resolves an issue where routes defined in the Angular server routing configuration did not match those in the Angular router. Previously, discrepancies between these routes went unnoticed by users. With this update, appropriate error messages are now displayed when mismatches occur, enhancing the developer experience and facilitating easier troubleshooting. --- packages/angular/ssr/src/routes/ng-routes.ts | 35 ++++++-- packages/angular/ssr/src/routes/route-tree.ts | 2 +- .../angular/ssr/test/routes/ng-routes_spec.ts | 81 ++++++++++++++----- 3 files changed, 93 insertions(+), 25 deletions(-) diff --git a/packages/angular/ssr/src/routes/ng-routes.ts b/packages/angular/ssr/src/routes/ng-routes.ts index e832be72a7c5..dd66ee6ba723 100644 --- a/packages/angular/ssr/src/routes/ng-routes.ts +++ b/packages/angular/ssr/src/routes/ng-routes.ts @@ -26,7 +26,7 @@ import { ServerAssets } from '../assets'; import { Console } from '../console'; import { AngularAppManifest, getAngularAppManifest } from '../manifest'; import { AngularBootstrap, isNgModule } from '../utils/ng'; -import { joinUrlParts } from '../utils/url'; +import { joinUrlParts, stripLeadingSlash } from '../utils/url'; import { PrerenderFallback, RenderMode, SERVER_ROUTES_CONFIG, ServerRoute } from './route-config'; import { RouteTree, RouteTreeNodeMetadata } from './route-tree'; @@ -43,7 +43,10 @@ const VALID_REDIRECT_RESPONSE_CODES = new Set([301, 302, 303, 307, 308]); /** * Additional metadata for a server configuration route tree. */ -type ServerConfigRouteTreeAdditionalMetadata = Partial; +type ServerConfigRouteTreeAdditionalMetadata = Partial & { + /** Indicates if the route has been matched with the Angular router routes. */ + matched?: boolean; +}; /** * Metadata for a server configuration route tree node. @@ -124,12 +127,14 @@ async function* traverseRoutesConfig(options: { if (!matchedMetaData) { yield { error: - `The '${currentRoutePath}' route does not match any route defined in the server routing configuration. ` + + `The '${stripLeadingSlash(currentRoutePath)}' route does not match any route defined in the server routing configuration. ` + 'Please ensure this route is added to the server routing configuration.', }; continue; } + + matchedMetaData.matched = true; } const metadata: ServerConfigRouteTreeNodeMetadata = { @@ -137,6 +142,8 @@ async function* traverseRoutesConfig(options: { route: currentRoutePath, }; + delete metadata.matched; + // Handle redirects if (typeof redirectTo === 'string') { const redirectToResolved = resolveRedirectTo(currentRoutePath, redirectTo); @@ -189,7 +196,9 @@ async function* traverseRoutesConfig(options: { } } } catch (error) { - yield { error: `Error processing route '${route.path}': ${(error as Error).message}` }; + yield { + error: `Error processing route '${stripLeadingSlash(route.path ?? '')}': ${(error as Error).message}`, + }; } } } @@ -237,7 +246,7 @@ async function* handleSSGRoute( if (!getPrerenderParams) { yield { error: - `The '${currentRoutePath}' route uses prerendering and includes parameters, but 'getPrerenderParams' is missing. ` + + `The '${stripLeadingSlash(currentRoutePath)}' route uses prerendering and includes parameters, but 'getPrerenderParams' is missing. ` + `Please define 'getPrerenderParams' function for this route in your server routing configuration ` + `or specify a different 'renderMode'.`, }; @@ -253,7 +262,7 @@ async function* handleSSGRoute( const value = params[parameterName]; if (typeof value !== 'string') { throw new Error( - `The 'getPrerenderParams' function defined for the '${currentRoutePath}' route ` + + `The 'getPrerenderParams' function defined for the '${stripLeadingSlash(currentRoutePath)}' route ` + `returned a non-string value for parameter '${parameterName}'. ` + `Please make sure the 'getPrerenderParams' function returns values for all parameters ` + 'specified in this route.', @@ -440,6 +449,20 @@ export async function getRoutesFromAngularRouterConfig( routesResults.push(result); } } + + if (serverConfigRouteTree) { + for (const { route, matched } of serverConfigRouteTree.traverse()) { + if (matched || route === '**') { + // Skip if matched or it's the catch-all route. + continue; + } + + errors.push( + `The server route '${route}' does not match any routes defined in the Angular routing configuration. ` + + 'Please verify and if unneeded remove this route from the server configuration.', + ); + } + } } else { routesResults.push({ route: '', renderMode: RenderMode.Prerender }); } diff --git a/packages/angular/ssr/src/routes/route-tree.ts b/packages/angular/ssr/src/routes/route-tree.ts index 8b8db243b21e..a7a31b5453d1 100644 --- a/packages/angular/ssr/src/routes/route-tree.ts +++ b/packages/angular/ssr/src/routes/route-tree.ts @@ -207,7 +207,7 @@ export class RouteTree = {}> * * @param node - The current node to start the traversal from. Defaults to the root node of the tree. */ - private *traverse(node = this.root): Generator { + *traverse(node = this.root): Generator { if (node.metadata) { yield node.metadata; } diff --git a/packages/angular/ssr/test/routes/ng-routes_spec.ts b/packages/angular/ssr/test/routes/ng-routes_spec.ts index 2a98a4130852..75d79afc6534 100644 --- a/packages/angular/ssr/test/routes/ng-routes_spec.ts +++ b/packages/angular/ssr/test/routes/ng-routes_spec.ts @@ -66,24 +66,6 @@ describe('extractRoutesAndCreateRouteTree', () => { ); }); - it('should handle route not matching server routing configuration', async () => { - setAngularAppTestingManifest( - [ - { path: 'home', component: DummyComponent }, - { path: 'about', component: DummyComponent }, // This route is not in the server configuration - ], - [ - { path: 'home', renderMode: RenderMode.Client }, - // 'about' route is missing here - ], - ); - - const { errors } = await extractRoutesAndCreateRouteTree(url); - expect(errors[0]).toContain( - `The '/about' route does not match any route defined in the server routing configuration.`, - ); - }); - describe('when `invokeGetPrerenderParams` is true', () => { it('should resolve parameterized routes for SSG and add a fallback route if fallback is Server', async () => { setAngularAppTestingManifest( @@ -294,4 +276,67 @@ describe('extractRoutesAndCreateRouteTree', () => { { route: '/user/:id/role/:role', renderMode: RenderMode.Client }, ]); }); + + 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, + /** manifest */ undefined, + /** 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, + /** manifest */ undefined, + /** invokeGetPrerenderParams */ false, + /** includePrerenderFallbackRoutes */ false, + ); + + expect(errors).toHaveSize(1); + expect(errors[0]).toContain( + `The server route 'invalid' 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, + /** manifest */ undefined, + /** 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`, + ); + }); }); From 3486248915b1119e79e3c9f990cbada0bb9fa202 Mon Sep 17 00:00:00 2001 From: Alan Agius Date: Thu, 26 Sep 2024 20:35:07 +0000 Subject: [PATCH 2/2] fix(@angular/ssr): show error when multiple routes are set with `RenderMode.AppShell` This change introduces error handling to ensure that when multiple routes are configured with `RenderMode.AppShell`, an error message is displayed. This prevents misconfiguration and enhances clarity in route management. --- packages/angular/ssr/src/routes/ng-routes.ts | 32 +++++++++++++------ .../angular/ssr/test/routes/ng-routes_spec.ts | 24 +++++++++++++- 2 files changed, 46 insertions(+), 10 deletions(-) diff --git a/packages/angular/ssr/src/routes/ng-routes.ts b/packages/angular/ssr/src/routes/ng-routes.ts index dd66ee6ba723..a2680d5175d9 100644 --- a/packages/angular/ssr/src/routes/ng-routes.ts +++ b/packages/angular/ssr/src/routes/ng-routes.ts @@ -45,7 +45,7 @@ const VALID_REDIRECT_RESPONSE_CODES = new Set([301, 302, 303, 307, 308]); */ type ServerConfigRouteTreeAdditionalMetadata = Partial & { /** Indicates if the route has been matched with the Angular router routes. */ - matched?: boolean; + presentInClientRouter?: boolean; }; /** @@ -134,7 +134,7 @@ async function* traverseRoutesConfig(options: { continue; } - matchedMetaData.matched = true; + matchedMetaData.presentInClientRouter = true; } const metadata: ServerConfigRouteTreeNodeMetadata = { @@ -142,7 +142,7 @@ async function* traverseRoutesConfig(options: { route: currentRoutePath, }; - delete metadata.matched; + delete metadata.presentInClientRouter; // Handle redirects if (typeof redirectTo === 'string') { @@ -246,8 +246,8 @@ async function* handleSSGRoute( if (!getPrerenderParams) { yield { error: - `The '${stripLeadingSlash(currentRoutePath)}' route uses prerendering and includes parameters, but 'getPrerenderParams' is missing. ` + - `Please define 'getPrerenderParams' function for this route in your server routing configuration ` + + `The '${stripLeadingSlash(currentRoutePath)}' route uses prerendering and includes parameters, but 'getPrerenderParams' ` + + `is missing. Please define 'getPrerenderParams' function for this route in your server routing configuration ` + `or specify a different 'renderMode'.`, }; @@ -442,24 +442,38 @@ export async function getRoutesFromAngularRouterConfig( includePrerenderFallbackRoutes, }); + let seenAppShellRoute: string | undefined; for await (const result of traverseRoutes) { if ('error' in result) { errors.push(result.error); } else { + if (result.renderMode === RenderMode.AppShell) { + if (seenAppShellRoute !== undefined) { + errors.push( + `Error: Both '${seenAppShellRoute}' and '${stripLeadingSlash(result.route)}' routes have ` + + `their 'renderMode' set to 'AppShell'. AppShell renderMode should only be assigned to one route. ` + + `Please review your route configurations to ensure that only one route is set to 'RenderMode.AppShell'.`, + ); + } + + seenAppShellRoute = stripLeadingSlash(result.route); + } + routesResults.push(result); } } if (serverConfigRouteTree) { - for (const { route, matched } of serverConfigRouteTree.traverse()) { - if (matched || route === '**') { + for (const { route, presentInClientRouter } of serverConfigRouteTree.traverse()) { + if (presentInClientRouter || route === '**') { // Skip if matched or it's the catch-all route. continue; } errors.push( - `The server route '${route}' does not match any routes defined in the Angular routing configuration. ` + - 'Please verify and if unneeded remove this route from the server configuration.', + `The '${route}' server route does not match any routes defined in the Angular ` + + `routing configuration (typically provided as a part of the 'provideRouter' call). ` + + 'Please make sure that the mentioned server route is present in the Angular routing configuration.', ); } } diff --git a/packages/angular/ssr/test/routes/ng-routes_spec.ts b/packages/angular/ssr/test/routes/ng-routes_spec.ts index 75d79afc6534..82ac36288110 100644 --- a/packages/angular/ssr/test/routes/ng-routes_spec.ts +++ b/packages/angular/ssr/test/routes/ng-routes_spec.ts @@ -314,7 +314,7 @@ describe('extractRoutesAndCreateRouteTree', () => { expect(errors).toHaveSize(1); expect(errors[0]).toContain( - `The server route 'invalid' does not match any routes defined in the Angular routing configuration`, + `The 'invalid' server route does not match any routes defined in the Angular routing configuration`, ); }); @@ -339,4 +339,26 @@ describe('extractRoutesAndCreateRouteTree', () => { `The 'invalid' route does not match any route defined in the server routing configuration`, ); }); + + it(`should error when 'RenderMode.AppShell' is used on more than one route`, async () => { + setAngularAppTestingManifest( + [ + { path: 'home', component: DummyComponent }, + { path: 'shell', component: DummyComponent }, + ], + [{ path: '**', renderMode: RenderMode.AppShell }], + ); + + const { errors } = await extractRoutesAndCreateRouteTree( + url, + /** manifest */ undefined, + /** invokeGetPrerenderParams */ false, + /** includePrerenderFallbackRoutes */ false, + ); + + expect(errors).toHaveSize(1); + expect(errors[0]).toContain( + `Both 'home' and 'shell' routes have their 'renderMode' set to 'AppShell'.`, + ); + }); });