diff --git a/packages/frontend-app-api/report.api.md b/packages/frontend-app-api/report.api.md index fca32ed38c..bd0709813b 100644 --- a/packages/frontend-app-api/report.api.md +++ b/packages/frontend-app-api/report.api.md @@ -109,6 +109,21 @@ export type AppErrorTypes = { node: AppNode; }; }; + ROUTE_DUPLICATE: { + context: { + routeId: string; + }; + }; + ROUTE_BINDING_INVALID_VALUE: { + context: { + routeId: string; + }; + }; + ROUTE_NOT_FOUND: { + context: { + routeId: string; + }; + }; }; // @public diff --git a/packages/frontend-app-api/src/routing/collectRouteIds.test.ts b/packages/frontend-app-api/src/routing/collectRouteIds.test.ts index 395886f982..b1bab3fbbe 100644 --- a/packages/frontend-app-api/src/routing/collectRouteIds.test.ts +++ b/packages/frontend-app-api/src/routing/collectRouteIds.test.ts @@ -20,6 +20,18 @@ import { createFrontendPlugin, } from '@backstage/frontend-plugin-api'; import { collectRouteIds } from './collectRouteIds'; +import { createErrorCollector } from '../wiring/createErrorCollector'; + +const collector = createErrorCollector(); + +afterEach(() => { + const errors = collector.collectErrors(); + if (errors) { + throw new Error( + `Unexpected errors: ${errors.map(e => e.message).join(', ')}`, + ); + } +}); describe('collectRouteIds', () => { it('should assign IDs to routes', () => { @@ -33,13 +45,16 @@ describe('collectRouteIds', () => { /^ExternalRouteRef\{created at '.*collectRouteIds\.test\.ts.*'\}$/, ); - const collected = collectRouteIds([ - createFrontendPlugin({ - pluginId: 'test', - routes: { ref }, - externalRoutes: { extRef }, - }), - ]); + const collected = collectRouteIds( + [ + createFrontendPlugin({ + pluginId: 'test', + routes: { ref }, + externalRoutes: { extRef }, + }), + ], + collector, + ); expect(Object.fromEntries(collected.routes)).toEqual({ 'test.ref': ref, }); @@ -50,4 +65,40 @@ describe('collectRouteIds', () => { expect(String(ref)).toBe('RouteRef{test.ref}'); expect(String(extRef)).toBe('ExternalRouteRef{test.extRef}'); }); + + it('should report duplicate route IDs', () => { + const ref = createRouteRef(); + const extRef = createExternalRouteRef(); + + collectRouteIds( + [ + createFrontendPlugin({ + pluginId: 'test', + routes: { 'mid.ref': ref }, + externalRoutes: { 'mid.extRef': extRef }, + }), + createFrontendPlugin({ + pluginId: 'test.mid', + routes: { ref }, + externalRoutes: { extRef }, + }), + ], + collector, + ); + + expect(collector.collectErrors()).toEqual([ + { + code: 'ROUTE_DUPLICATE', + message: + "Duplicate route id 'test.mid.ref' encountered while collecting routes", + context: { routeId: 'test.mid.ref' }, + }, + { + code: 'ROUTE_DUPLICATE', + message: + "Duplicate external route id 'test.mid.extRef' encountered while collecting routes", + context: { routeId: 'test.mid.extRef' }, + }, + ]); + }); }); diff --git a/packages/frontend-app-api/src/routing/collectRouteIds.ts b/packages/frontend-app-api/src/routing/collectRouteIds.ts index 8f189bda81..79afb07928 100644 --- a/packages/frontend-app-api/src/routing/collectRouteIds.ts +++ b/packages/frontend-app-api/src/routing/collectRouteIds.ts @@ -30,6 +30,7 @@ import { toInternalExternalRouteRef } from '../../../frontend-plugin-api/src/rou // eslint-disable-next-line @backstage/no-relative-monorepo-imports import { toInternalSubRouteRef } from '../../../frontend-plugin-api/src/routing/SubRouteRef'; import { OpaqueFrontendPlugin } from '@internal/frontend'; +import { ErrorCollector } from '../wiring/createErrorCollector'; /** @internal */ export interface RouteRefsById { @@ -38,7 +39,10 @@ export interface RouteRefsById { } /** @internal */ -export function collectRouteIds(features: FrontendFeature[]): RouteRefsById { +export function collectRouteIds( + features: FrontendFeature[], + collector: ErrorCollector, +): RouteRefsById { const routesById = new Map(); const externalRoutesById = new Map(); @@ -50,7 +54,12 @@ export function collectRouteIds(features: FrontendFeature[]): RouteRefsById { for (const [name, ref] of Object.entries(feature.routes)) { const refId = `${feature.id}.${name}`; if (routesById.has(refId)) { - throw new Error(`Unexpected duplicate route '${refId}'`); + collector.report({ + code: 'ROUTE_DUPLICATE', + message: `Duplicate route id '${refId}' encountered while collecting routes`, + context: { routeId: refId }, + }); + continue; } if (isRouteRef(ref)) { @@ -65,7 +74,12 @@ export function collectRouteIds(features: FrontendFeature[]): RouteRefsById { for (const [name, ref] of Object.entries(feature.externalRoutes)) { const refId = `${feature.id}.${name}`; if (externalRoutesById.has(refId)) { - throw new Error(`Unexpected duplicate external route '${refId}'`); + collector.report({ + code: 'ROUTE_DUPLICATE', + message: `Duplicate external route id '${refId}' encountered while collecting routes`, + context: { routeId: refId }, + }); + continue; } const internalRef = toInternalExternalRouteRef(ref); diff --git a/packages/frontend-app-api/src/routing/resolveRouteBindings.test.ts b/packages/frontend-app-api/src/routing/resolveRouteBindings.test.ts index 6618bef152..003980e617 100644 --- a/packages/frontend-app-api/src/routing/resolveRouteBindings.test.ts +++ b/packages/frontend-app-api/src/routing/resolveRouteBindings.test.ts @@ -20,6 +20,18 @@ import { } from '@backstage/frontend-plugin-api'; import { resolveRouteBindings } from './resolveRouteBindings'; import { ConfigReader } from '@backstage/config'; +import { createErrorCollector } from '../wiring/createErrorCollector'; + +const collector = createErrorCollector(); + +afterEach(() => { + const errors = collector.collectErrors(); + if (errors) { + throw new Error( + `Unexpected errors: ${errors.map(e => e.message).join(', ')}`, + ); + } +}); const emptyIds = { routes: new Map(), externalRoutes: new Map() }; @@ -33,23 +45,30 @@ describe('resolveRouteBindings', () => { }, new ConfigReader({}), emptyIds, + collector, ); expect(result.get(external.myRoute)).toBe(ref); }); - it('throws on unknown keys', () => { + it('reports error on unknown keys', () => { const external = { myRoute: createExternalRouteRef() }; const ref = createRouteRef(); - expect(() => - resolveRouteBindings( - ({ bind }) => { - bind(external, { someOtherRoute: ref } as any); - }, - new ConfigReader({}), - emptyIds, - ), - ).toThrow('Key someOtherRoute is not an existing external route'); + resolveRouteBindings( + ({ bind }) => { + bind(external, { someOtherRoute: ref } as any); + }, + new ConfigReader({}), + emptyIds, + collector, + ); + expect(collector.collectErrors()).toEqual([ + { + code: 'ROUTE_NOT_FOUND', + message: 'Key someOtherRoute is not an existing external route', + context: { routeId: 'someOtherRoute' }, + }, + ]); }); it('reads bindings from config', () => { @@ -64,6 +83,7 @@ describe('resolveRouteBindings', () => { routes: new Map([['myTarget', myTarget]]), externalRoutes: new Map([['mySource', mySource]]), }, + collector, ); expect(result.get(mySource)).toBe(myTarget); @@ -85,6 +105,7 @@ describe('resolveRouteBindings', () => { routes: new Map([['myTarget', myTarget]]), externalRoutes: new Map([['mySource', mySource]]), }, + collector, ).get(mySource), ).toBe(undefined); @@ -100,57 +121,74 @@ describe('resolveRouteBindings', () => { routes: new Map([['myTarget', myTarget]]), externalRoutes: new Map([['mySource', mySource]]), }, + collector, ).get(mySource), ).toBe(myTarget); }); - it('throws on invalid config', () => { + it('reports errors on invalid config', () => { expect(() => resolveRouteBindings( () => {}, new ConfigReader({ app: { routes: { bindings: 'derp' } } }), emptyIds, + collector, ), ).toThrow( "Invalid type in config for key 'app.routes.bindings' in 'mock-config', got string, wanted object", ); - expect(() => - resolveRouteBindings( - () => {}, - new ConfigReader({ app: { routes: { bindings: { mySource: true } } } }), - emptyIds, - ), - ).toThrow( - "Invalid config at app.routes.bindings['mySource'], value must be a non-empty string", + resolveRouteBindings( + () => {}, + new ConfigReader({ app: { routes: { bindings: { mySource: true } } } }), + emptyIds, + collector, ); + expect(collector.collectErrors()).toEqual([ + { + code: 'ROUTE_BINDING_INVALID_VALUE', + message: + "Invalid config at app.routes.bindings['mySource'], value must be a non-empty string or false", + context: { routeId: 'mySource' }, + }, + ]); - expect(() => - resolveRouteBindings( - () => {}, - new ConfigReader({ - app: { routes: { bindings: { mySource: 'myTarget' } } }, - }), - emptyIds, - ), - ).toThrow( - "Invalid config at app.routes.bindings, 'mySource' is not a valid external route", + resolveRouteBindings( + () => {}, + new ConfigReader({ + app: { routes: { bindings: { mySource: 'myTarget' } } }, + }), + emptyIds, + collector, ); + expect(collector.collectErrors()).toEqual([ + { + code: 'ROUTE_NOT_FOUND', + message: + "Invalid config at app.routes.bindings, 'mySource' is not a valid external route", + context: { routeId: 'mySource' }, + }, + ]); - expect(() => - resolveRouteBindings( - () => {}, - new ConfigReader({ - app: { routes: { bindings: { mySource: 'myTarget' } } }, - }), - { - ...emptyIds, - externalRoutes: new Map([['mySource', createExternalRouteRef()]]), - }, - ), - ).toThrow( - "Invalid config at app.routes.bindings['mySource'], 'myTarget' is not a valid route", + resolveRouteBindings( + () => {}, + new ConfigReader({ + app: { routes: { bindings: { mySource: 'myTarget' } } }, + }), + { + ...emptyIds, + externalRoutes: new Map([['mySource', createExternalRouteRef()]]), + }, + collector, ); + expect(collector.collectErrors()).toEqual([ + { + code: 'ROUTE_NOT_FOUND', + message: + "Invalid config at app.routes.bindings['mySource'], 'myTarget' is not a valid route", + context: { routeId: 'myTarget' }, + }, + ]); }); it('can have default targets, but at the lowest priority', () => { @@ -170,6 +208,7 @@ describe('resolveRouteBindings', () => { () => {}, new ConfigReader({}), routesById, + collector, ); expect(result.get(source)).toBe(target1); @@ -181,6 +220,7 @@ describe('resolveRouteBindings', () => { app: { routes: { bindings: { source: 'target2' } } }, }), routesById, + collector, ); expect(result.get(source)).toBe(target2); @@ -192,6 +232,7 @@ describe('resolveRouteBindings', () => { }, new ConfigReader({}), routesById, + collector, ); expect(result.get(source)).toBe(target2); @@ -210,6 +251,7 @@ describe('resolveRouteBindings', () => { () => {}, new ConfigReader({}), routesById, + collector, ); expect(result.get(source)).toBe(target1); @@ -219,6 +261,7 @@ describe('resolveRouteBindings', () => { () => {}, new ConfigReader({ app: { routes: { bindings: { source: false } } } }), routesById, + collector, ); expect(result.get(source)).toBe(undefined); diff --git a/packages/frontend-app-api/src/routing/resolveRouteBindings.ts b/packages/frontend-app-api/src/routing/resolveRouteBindings.ts index 76d620c20c..f9836d7107 100644 --- a/packages/frontend-app-api/src/routing/resolveRouteBindings.ts +++ b/packages/frontend-app-api/src/routing/resolveRouteBindings.ts @@ -20,6 +20,7 @@ import { ExternalRouteRef, } from '@backstage/frontend-plugin-api'; import { RouteRefsById } from './collectRouteIds'; +import { ErrorCollector } from '../wiring/createErrorCollector'; import { Config } from '@backstage/config'; import { JsonObject } from '@backstage/types'; // eslint-disable-next-line @backstage/no-relative-monorepo-imports @@ -80,6 +81,7 @@ export function resolveRouteBindings( bindRoutes: ((context: { bind: CreateAppRouteBinder }) => void) | undefined, config: Config, routesById: RouteRefsById, + collector: ErrorCollector, ): Map { const result = new Map(); const disabledExternalRefs = new Set(); @@ -93,7 +95,12 @@ export function resolveRouteBindings( for (const [key, value] of Object.entries(targetRoutes)) { const externalRoute = externalRoutes[key]; if (!externalRoute) { - throw new Error(`Key ${key} is not an existing external route`); + collector.report({ + code: 'ROUTE_NOT_FOUND', + message: `Key ${key} is not an existing external route`, + context: { routeId: String(key) }, + }); + continue; } if (value) { result.set(externalRoute, value); @@ -112,16 +119,22 @@ export function resolveRouteBindings( if (bindings) { for (const [externalRefId, targetRefId] of Object.entries(bindings)) { if (!isValidTargetRefId(targetRefId)) { - throw new Error( - `Invalid config at app.routes.bindings['${externalRefId}'], value must be a non-empty string or false`, - ); + collector.report({ + code: 'ROUTE_BINDING_INVALID_VALUE', + message: `Invalid config at app.routes.bindings['${externalRefId}'], value must be a non-empty string or false`, + context: { routeId: externalRefId }, + }); + continue; } const externalRef = routesById.externalRoutes.get(externalRefId); if (!externalRef) { - throw new Error( - `Invalid config at app.routes.bindings, '${externalRefId}' is not a valid external route`, - ); + collector.report({ + code: 'ROUTE_NOT_FOUND', + message: `Invalid config at app.routes.bindings, '${externalRefId}' is not a valid external route`, + context: { routeId: externalRefId }, + }); + continue; } // Skip if binding was already defined in code @@ -134,9 +147,14 @@ export function resolveRouteBindings( } else { const targetRef = routesById.routes.get(targetRefId); if (!targetRef) { - throw new Error( - `Invalid config at app.routes.bindings['${externalRefId}'], '${targetRefId}' is not a valid route`, - ); + collector.report({ + code: 'ROUTE_NOT_FOUND', + message: `Invalid config at app.routes.bindings['${externalRefId}'], '${String( + targetRefId, + )}' is not a valid route`, + context: { routeId: String(targetRefId) }, + }); + continue; } result.set(externalRef, targetRef); diff --git a/packages/frontend-app-api/src/wiring/createErrorCollector.ts b/packages/frontend-app-api/src/wiring/createErrorCollector.ts index 93540cec63..c20e92d0f0 100644 --- a/packages/frontend-app-api/src/wiring/createErrorCollector.ts +++ b/packages/frontend-app-api/src/wiring/createErrorCollector.ts @@ -66,6 +66,16 @@ export type AppErrorTypes = { API_EXTENSION_INVALID: { context: { node: AppNode }; }; + // routing + ROUTE_DUPLICATE: { + context: { routeId: string }; + }; + ROUTE_BINDING_INVALID_VALUE: { + context: { routeId: string }; + }; + ROUTE_NOT_FOUND: { + context: { routeId: string }; + }; }; /** diff --git a/packages/frontend-app-api/src/wiring/createSpecializedApp.tsx b/packages/frontend-app-api/src/wiring/createSpecializedApp.tsx index a9b7e6864a..6c6d75415b 100644 --- a/packages/frontend-app-api/src/wiring/createSpecializedApp.tsx +++ b/packages/frontend-app-api/src/wiring/createSpecializedApp.tsx @@ -316,9 +316,9 @@ export function createSpecializedApp(options?: CreateSpecializedAppOptions): { const appBasePath = getBasePath(config); const appTreeApi = new AppTreeApiProxy(tree, appBasePath); - const routeRefsById = collectRouteIds(features); + const routeRefsById = collectRouteIds(features, collector); const routeResolutionApi = new RouteResolutionApiProxy( - resolveRouteBindings(options?.bindRoutes, config, routeRefsById), + resolveRouteBindings(options?.bindRoutes, config, routeRefsById, collector), appBasePath, ); diff --git a/packages/frontend-defaults/src/maybeCreateErrorPage.tsx b/packages/frontend-defaults/src/maybeCreateErrorPage.tsx index 40da64e987..6bad97f666 100644 --- a/packages/frontend-defaults/src/maybeCreateErrorPage.tsx +++ b/packages/frontend-defaults/src/maybeCreateErrorPage.tsx @@ -28,9 +28,10 @@ const DEFAULT_WARNING_CODES: Array = [ function AppErrorItem(props: { error: AppError }): JSX.Element { const { context } = props.error; - const extensionId = - 'extensionId' in context ? context.extensionId : context.node.spec.id; const node = 'node' in context ? context.node : undefined; + const extensionId = + 'extensionId' in context ? context.extensionId : node?.spec.id; + const routeId = 'routeId' in context ? context.routeId : undefined; const plugin = 'plugin' in context ? context.plugin : node?.spec.plugin; const pluginId = plugin?.id ?? 'N/A'; @@ -46,7 +47,8 @@ function AppErrorItem(props: { error: AppError }): JSX.Element {
{props.error.code}: {props.error.message}
-        
extensionId: {extensionId}
+ {extensionId &&
extensionId: {extensionId}
} + {routeId &&
routeId: {routeId}
} {pluginId &&
pluginId: {pluginId}
} {info && (