diff --git a/.changeset/cuddly-jobs-kick.md b/.changeset/cuddly-jobs-kick.md new file mode 100644 index 0000000000..aa62efb88b --- /dev/null +++ b/.changeset/cuddly-jobs-kick.md @@ -0,0 +1,5 @@ +--- +'@backstage/core-compat-api': patch +--- + +Add support for forwarding default target from legacy external route refs. diff --git a/.changeset/honest-pandas-chew.md b/.changeset/honest-pandas-chew.md new file mode 100644 index 0000000000..d5ae9e8cea --- /dev/null +++ b/.changeset/honest-pandas-chew.md @@ -0,0 +1,5 @@ +--- +'@backstage/core-plugin-api': patch +--- + +Added a new `defaultTarget` option to `createExternalRouteRef`. I lets you specify a default target of the route by name, for example `'catalog.catalogIndex'`, which will be used if the target route is present in the app and there is no explicit route binding. diff --git a/.changeset/serious-yaks-sit.md b/.changeset/serious-yaks-sit.md new file mode 100644 index 0000000000..5afc74f955 --- /dev/null +++ b/.changeset/serious-yaks-sit.md @@ -0,0 +1,29 @@ +--- +'@backstage/core-app-api': patch +--- + +Added support for configuration of route bindings through static configuration, and default targets for external route refs. + +In addition to configuring route bindings through code, it is now also possible to configure route bindings under the `app.routes.bindings` key, for example: + +```yaml +app: + routes: + bindings: + catalog.createComponent: catalog-import.importPage +``` + +Each key in the route binding object is of the form `.`, where the route name is key used in the `externalRoutes` object passed to `createPlugin`. The value is of the same form, but with the name taken from the plugin `routes` option instead. + +The equivalent of the above configuration in code is the following: + +```ts +const app = createApp({ + // ... + bindRoutes({ bind }) { + bind(catalogPlugin.externalRoutes, { + createComponent: catalogImportPlugin.routes.importPage, + }); + }, +}); +``` diff --git a/packages/core-app-api/src/app/AppManager.test.tsx b/packages/core-app-api/src/app/AppManager.test.tsx index 1345ccb13e..85b6bcd684 100644 --- a/packages/core-app-api/src/app/AppManager.test.tsx +++ b/packages/core-app-api/src/app/AppManager.test.tsx @@ -20,7 +20,7 @@ import { renderWithEffects, withLogCollector, } from '@backstage/test-utils'; -import { render, screen, waitFor, act } from '@testing-library/react'; +import { screen, waitFor, act } from '@testing-library/react'; import userEvent from '@testing-library/user-event'; import React, { PropsWithChildren, ReactNode } from 'react'; import { BrowserRouter, Navigate, Route, Routes } from 'react-router-dom'; @@ -624,7 +624,7 @@ describe('Integration Test', () => { expect(capturedEvents).toHaveLength(2); }); - it('should throw some error when the route has duplicate params', () => { + it('should throw some error when the route has duplicate params', async () => { const app = new AppManager({ apis: [], defaultApis: [], @@ -641,31 +641,43 @@ describe('Integration Test', () => { }, }); + const expectedMessage = + 'Parameter :thing is duplicated in path test/:thing/some/:thing'; + const Provider = app.getProvider(); const Router = app.getRouter(); - const { error: errorLogs } = withLogCollector(() => { - render( - - - - }> - } /> - - - - , - ); + const { error: errorLogs } = await withLogCollector(async () => { + await expect(() => + renderWithEffects( + + + + }> + } /> + + + + , + ), + ).rejects.toThrow(expectedMessage); }); + expect(errorLogs).toEqual([ expect.objectContaining({ - message: expect.stringContaining( - 'Parameter :thing is duplicated in path test/:thing/some/:thing', - ), + detail: new Error(expectedMessage), + type: 'unhandled exception', }), + expect.objectContaining({ + detail: new Error(expectedMessage), + type: 'unhandled exception', + }), + expect.stringContaining( + 'The above error occurred in the component:', + ), ]); }); - it('should throw an error when required external plugin routes are not bound', () => { + it('should throw an error when required external plugin routes are not bound', async () => { const app = new AppManager({ apis: [], defaultApis: [], @@ -676,25 +688,36 @@ describe('Integration Test', () => { configLoader: async () => [], }); + const expectedMessage = + "External route 'extRouteRef1' of the 'blob' plugin must be bound to a target route. See https://backstage.io/link?bind-routes for details."; + const Provider = app.getProvider(); const Router = app.getRouter(); - const { error: errorLogs } = withLogCollector(() => { - render( - - - - } /> - - - , - ); + const { error: errorLogs } = await withLogCollector(async () => { + await expect(() => + renderWithEffects( + + + + } /> + + + , + ), + ).rejects.toThrow(expectedMessage); }); expect(errorLogs).toEqual([ expect.objectContaining({ - message: expect.stringMatching( - /^External route 'extRouteRef1' of the 'blob' plugin must be bound to a target route/, - ), + detail: new Error(expectedMessage), + type: 'unhandled exception', }), + expect.objectContaining({ + detail: new Error(expectedMessage), + type: 'unhandled exception', + }), + expect.stringContaining( + 'The above error occurred in the component:', + ), ]); }); diff --git a/packages/core-app-api/src/app/AppManager.tsx b/packages/core-app-api/src/app/AppManager.tsx index 64736470d2..48501b52de 100644 --- a/packages/core-app-api/src/app/AppManager.tsx +++ b/packages/core-app-api/src/app/AppManager.tsx @@ -233,8 +233,10 @@ export class AppManager implements BackstageApp { const appContext = new AppContextImpl(this); - // We only validate routes once - let routesHaveBeenValidated = false; + // We only bind and validate routes once + let routeBindings: ReturnType; + // Store and keep throwing the same error if we encounter one + let routeValidationError: Error | undefined = undefined; const Provider = ({ children }: PropsWithChildren<{}>) => { const needsFeatureFlagRegistrationRef = useRef(true); @@ -243,7 +245,7 @@ export class AppManager implements BackstageApp { [], ); - const { routing, featureFlags, routeBindings } = useMemo(() => { + const { routing, featureFlags } = useMemo(() => { const usesReactRouterBeta = isReactRouterBeta(); if (usesReactRouterBeta) { // eslint-disable-next-line no-console @@ -275,21 +277,9 @@ DEPRECATION WARNING: React Router Beta is deprecated and support for it will be // Initialize APIs once all plugins are available this.getApiHolder(); - return { - ...result, - routeBindings: resolveRouteBindings(this.bindRoutes), - }; + return result; }, [children]); - if (!routesHaveBeenValidated) { - routesHaveBeenValidated = true; - validateRouteParameters(routing.paths, routing.parents); - validateRouteBindings( - routeBindings, - this.plugins as Iterable, - ); - } - const loadedConfig = useConfigLoader( this.configLoader, this.components, @@ -307,6 +297,24 @@ DEPRECATION WARNING: React Router Beta is deprecated and support for it will be return loadedConfig.node; } + if (!routeBindings) { + routeBindings = resolveRouteBindings( + this.bindRoutes, + loadedConfig.api, + this.plugins, + ); + + try { + validateRouteParameters(routing.paths, routing.parents); + validateRouteBindings(routeBindings, this.plugins); + } catch (error) { + routeValidationError = error; + throw error; + } + } else if (routeValidationError) { + throw routeValidationError; + } + // We can't register feature flags just after the element traversal, because the // config API isn't available yet and implementations frequently depend on it. // Instead we make it happen immediately, to make sure all flags are available diff --git a/packages/core-app-api/src/app/resolveRouteBindings.test.ts b/packages/core-app-api/src/app/resolveRouteBindings.test.ts index 5c9fde53b7..200be4f367 100644 --- a/packages/core-app-api/src/app/resolveRouteBindings.test.ts +++ b/packages/core-app-api/src/app/resolveRouteBindings.test.ts @@ -16,17 +16,23 @@ import { createExternalRouteRef, + createPlugin, createRouteRef, } from '@backstage/core-plugin-api'; -import { resolveRouteBindings } from './resolveRouteBindings'; +import { collectRouteIds, resolveRouteBindings } from './resolveRouteBindings'; +import { MockConfigApi } from '@backstage/test-utils'; describe('resolveRouteBindings', () => { it('runs happy path', () => { const external = { myRoute: createExternalRouteRef({ id: '1' }) }; const ref = createRouteRef({ id: 'ref-1' }); - const result = resolveRouteBindings(({ bind }) => { - bind(external, { myRoute: ref }); - }); + const result = resolveRouteBindings( + ({ bind }) => { + bind(external, { myRoute: ref }); + }, + new MockConfigApi({}), + [], + ); expect(result.get(external.myRoute)).toBe(ref); }); @@ -35,9 +41,30 @@ describe('resolveRouteBindings', () => { const external = { myRoute: createExternalRouteRef({ id: '2' }) }; const ref = createRouteRef({ id: 'ref-2' }); expect(() => - resolveRouteBindings(({ bind }) => { - bind(external, { someOtherRoute: ref } as any); - }), + resolveRouteBindings( + ({ bind }) => { + bind(external, { someOtherRoute: ref } as any); + }, + new MockConfigApi({}), + [], + ), ).toThrow('Key someOtherRoute is not an existing external route'); }); }); + +describe('collectRouteIds', () => { + it('should assign IDs to routes', () => { + const ref = createRouteRef({ id: 'ignored' }); + const extRef = createExternalRouteRef({ id: 'ignored' }); + + const collected = collectRouteIds([ + createPlugin({ id: 'test', routes: { ref }, externalRoutes: { extRef } }), + ]); + expect(Object.fromEntries(collected.routes)).toEqual({ + 'test.ref': ref, + }); + expect(Object.fromEntries(collected.externalRoutes)).toEqual({ + 'test.extRef': extRef, + }); + }); +}); diff --git a/packages/core-app-api/src/app/resolveRouteBindings.ts b/packages/core-app-api/src/app/resolveRouteBindings.ts index 63119b204b..55e1139b29 100644 --- a/packages/core-app-api/src/app/resolveRouteBindings.ts +++ b/packages/core-app-api/src/app/resolveRouteBindings.ts @@ -18,12 +18,63 @@ import { RouteRef, SubRouteRef, ExternalRouteRef, + BackstagePlugin, + AnyRoutes, + AnyExternalRoutes, } from '@backstage/core-plugin-api'; import { AppOptions, AppRouteBinder } from './types'; +import { Config } from '@backstage/config'; +import { JsonObject } from '@backstage/types'; -export function resolveRouteBindings(bindRoutes: AppOptions['bindRoutes']) { +/** @internal */ +export function collectRouteIds( + plugins: Iterable< + Pick< + BackstagePlugin, + 'getId' | 'routes' | 'externalRoutes' + > + >, +) { + const routesById = new Map(); + const externalRoutesById = new Map(); + + for (const plugin of plugins) { + for (const [name, ref] of Object.entries(plugin.routes ?? {})) { + const refId = `${plugin.getId()}.${name}`; + if (routesById.has(refId)) { + throw new Error(`Unexpected duplicate route '${refId}'`); + } + + routesById.set(refId, ref); + } + for (const [name, ref] of Object.entries(plugin.externalRoutes ?? {})) { + const refId = `${plugin.getId()}.${name}`; + if (externalRoutesById.has(refId)) { + throw new Error(`Unexpected duplicate external route '${refId}'`); + } + + externalRoutesById.set(refId, ref); + } + } + + return { routes: routesById, externalRoutes: externalRoutesById }; +} + +/** @internal */ +export function resolveRouteBindings( + bindRoutes: AppOptions['bindRoutes'], + config: Config, + plugins: Iterable< + Pick< + BackstagePlugin, + 'getId' | 'routes' | 'externalRoutes' + > + >, +) { + const routesById = collectRouteIds(plugins); const result = new Map(); + // Perform callback bindings first with highest priority if (bindRoutes) { const bind: AppRouteBinder = ( externalRoutes, @@ -47,5 +98,53 @@ export function resolveRouteBindings(bindRoutes: AppOptions['bindRoutes']) { bindRoutes({ bind }); } + // Then perform config based bindings with lower priority + const bindings = config + .getOptionalConfig('app.routes.bindings') + ?.get(); + if (bindings) { + for (const [externalRefId, targetRefId] of Object.entries(bindings)) { + if (typeof targetRefId !== 'string' || targetRefId === '') { + throw new Error( + `Invalid config at app.routes.bindings['${externalRefId}'], value must be a non-empty string`, + ); + } + + const externalRef = routesById.externalRoutes.get(externalRefId); + if (!externalRef) { + throw new Error( + `Invalid config at app.routes.bindings, '${externalRefId}' is not a valid external route`, + ); + } + if (result.has(externalRef)) { + continue; + } + const targetRef = routesById.routes.get(targetRefId); + if (!targetRef) { + throw new Error( + `Invalid config at app.routes.bindings['${externalRefId}'], '${targetRefId}' is not a valid route`, + ); + } + + result.set(externalRef, targetRef); + } + } + + // Finally fall back to attempting to map defaults, at lowest priority + for (const externalRef of routesById.externalRoutes.values()) { + if (!result.has(externalRef)) { + const defaultRefId = + 'getDefaultTarget' in externalRef + ? (externalRef.getDefaultTarget as () => string | undefined)() + : undefined; + if (defaultRefId) { + const defaultRef = routesById.routes.get(defaultRefId); + if (defaultRef) { + result.set(externalRef, defaultRef); + } + } + } + } + return result; } diff --git a/packages/core-app-api/src/routing/validation.ts b/packages/core-app-api/src/routing/validation.ts index 63950b9fdc..ab0fc42af4 100644 --- a/packages/core-app-api/src/routing/validation.ts +++ b/packages/core-app-api/src/routing/validation.ts @@ -66,7 +66,12 @@ export function validateRouteParameters( // Validates that all non-optional external routes have been bound export function validateRouteBindings( routeBindings: Map, - plugins: Iterable>>, + plugins: Iterable< + Pick< + BackstagePlugin<{}, Record>, + 'getId' | 'externalRoutes' + > + >, ) { for (const plugin of plugins) { if (!plugin.externalRoutes) { diff --git a/packages/core-compat-api/src/convertLegacyRouteRef.ts b/packages/core-compat-api/src/convertLegacyRouteRef.ts index a5375e0692..37277fc422 100644 --- a/packages/core-compat-api/src/convertLegacyRouteRef.ts +++ b/packages/core-compat-api/src/convertLegacyRouteRef.ts @@ -186,6 +186,10 @@ export function convertLegacyRouteRef( createExternalRouteRef<{ [key in string]: string }>({ params: legacyRef.params as string[], optional: legacyRef.optional, + defaultTarget: + 'getDefaultTarget' in legacyRef + ? (legacyRef.getDefaultTarget as () => string | undefined)() + : undefined, }), ); return Object.assign(legacyRef, { @@ -199,10 +203,8 @@ export function convertLegacyRouteRef( getDescription() { return legacyRefStr; }, - getDefaultTarget() { - // TODO(freben): These are not yet supported in the old system; just returning undefined for now - return undefined; - }, + // This might already be implemented in the legacy ref, but we override it just to be sure + getDefaultTarget: newRef.getDefaultTarget, setId(id: string) { newRef.setId(id); }, diff --git a/packages/core-plugin-api/api-report.md b/packages/core-plugin-api/api-report.md index 2a993687b3..134b338c92 100644 --- a/packages/core-plugin-api/api-report.md +++ b/packages/core-plugin-api/api-report.md @@ -314,6 +314,7 @@ export function createExternalRouteRef< id: string; params?: ParamKey[]; optional?: Optional; + defaultTarget?: string; }): ExternalRouteRef, Optional>; // @public diff --git a/packages/core-plugin-api/src/routing/ExternalRouteRef.ts b/packages/core-plugin-api/src/routing/ExternalRouteRef.ts index 2ebb81dbfb..d61001aa10 100644 --- a/packages/core-plugin-api/src/routing/ExternalRouteRef.ts +++ b/packages/core-plugin-api/src/routing/ExternalRouteRef.ts @@ -38,11 +38,16 @@ export class ExternalRouteRefImpl< private readonly id: string, readonly params: ParamKeys, readonly optional: Optional, + readonly defaultTarget: string | undefined, ) {} toString() { return `routeRef{type=external,id=${this.id}}`; } + + getDefaultTarget() { + return this.defaultTarget; + } } /** @@ -77,10 +82,19 @@ export function createExternalRouteRef< * if they aren't, `useRouteRef` will return `undefined`. */ optional?: Optional; + + /** + * The route (typically in another plugin) that this should map to by default. + * + * The string is expected to be on the standard `.` form, + * for example `techdocs.docRoot`. + */ + defaultTarget?: string; }): ExternalRouteRef, Optional> { return new ExternalRouteRefImpl( options.id, (options.params ?? []) as ParamKeys>, Boolean(options.optional) as Optional, + options?.defaultTarget, ); }