From 35fbe097ea2d2f60376e93c07cd740ecb1a326b3 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Wed, 8 May 2024 17:13:39 +0200 Subject: [PATCH 01/13] core-*-api: add support for default targets for external route refs and route binding through config Signed-off-by: Patrik Oldsberg --- .changeset/cuddly-jobs-kick.md | 5 + .changeset/honest-pandas-chew.md | 5 + .changeset/serious-yaks-sit.md | 29 +++++ .../core-app-api/src/app/AppManager.test.tsx | 85 +++++++++------ packages/core-app-api/src/app/AppManager.tsx | 40 ++++--- .../src/app/resolveRouteBindings.test.ts | 41 +++++-- .../src/app/resolveRouteBindings.ts | 101 +++++++++++++++++- .../core-app-api/src/routing/validation.ts | 7 +- .../src/convertLegacyRouteRef.ts | 10 +- packages/core-plugin-api/api-report.md | 1 + .../src/routing/ExternalRouteRef.ts | 14 +++ 11 files changed, 278 insertions(+), 60 deletions(-) create mode 100644 .changeset/cuddly-jobs-kick.md create mode 100644 .changeset/honest-pandas-chew.md create mode 100644 .changeset/serious-yaks-sit.md 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, ); } From b80dc81229f97d17bb50437bdf5fff8dd8f2b786 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Wed, 8 May 2024 17:14:56 +0200 Subject: [PATCH 02/13] docs/frontend-system: add docs for external route refs default targets Signed-off-by: Patrik Oldsberg --- docs/frontend-system/architecture/07-routes.md | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/docs/frontend-system/architecture/07-routes.md b/docs/frontend-system/architecture/07-routes.md index 7fb053aa56..42750fad39 100644 --- a/docs/frontend-system/architecture/07-routes.md +++ b/docs/frontend-system/architecture/07-routes.md @@ -274,6 +274,21 @@ Note that we are not importing and using the `RouteRef`s directly in the app, an Another thing to note is that this indirection in the routing is particularly useful for open source plugins that need to provide flexibility in how they are integrated. For plugins that you build internally for your own Backstage application, you can choose to use direct imports or even concrete route path strings directly. Although there can be some benefits to using the full routing system even in internal plugins: it can help you structure your routes, and as you will see further down it also helps you manage route parameters. +### Default Targets for External Route References + +It is possible to define a default target for an external route reference, potentially removing the need to bind the route in the app. This reduces the need for configuration when installing new plugins through providing a sensible default. It is of course still possible to override the route binding in the app. + +The default target uses the same syntax as the route binding configuration, and will only be used if the target plugin an route exists. For example, this is how the catalog can define a default target for the create component external route in a way that removes the need for the binding in the previous example: + +```tsx title="plugins/catalog/src/routes.ts" +import { createExternalRouteRef } from '@backstage/frontend-plugin-api'; + +export const createComponentExternalRouteRef = createExternalRouteRef({ + // highlight-next-line + defaultTarget: 'scaffolder.createComponent', +}); +``` + ### Optional External Route References It is possible to define an `ExternalRouteRef` as optional, so it is not required to bind it in the app. From ec3ca372aafc488aa5cdaaccceb6bb087f406533 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Wed, 8 May 2024 17:15:19 +0200 Subject: [PATCH 03/13] docs/plugins/composability: document default external route refs targets and route binding through config Signed-off-by: Patrik Oldsberg --- docs/plugins/composability.md | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/docs/plugins/composability.md b/docs/plugins/composability.md index b18bae31c6..1a9c0babf9 100644 --- a/docs/plugins/composability.md +++ b/docs/plugins/composability.md @@ -325,6 +325,33 @@ concrete routes directly. Although there can be some benefits to using the full routing system even in internal plugins. It can help you structure your routes, and as you will see further down it also helps you manage route parameters. +You can also use static configuration to bind routes, removing the need to make +changes to the app code. It does however mean that you won't get type safety +when binding routes and compile-time validation of the bindings. Static +configuration of route bindings is done under the `app.routes.bindings` key in +`app-config.yaml`. It works the same way as [route bindings in the new frontend system](../frontend-system/architecture/07-routes.md#binding-external-route-references), +for example: + +```yaml +app: + routes: + bindings: + bar.headerLink: foo.root +``` + +### Default Targets for External Route References + +Following the `1.28` release of Backstage you can now define default targets for +external route references. They work the same way as [default targets in the new frontend system](../frontend-system/architecture/07-routes.md#default-targets-for-external-route-references), +for example: + +```ts +export const createComponentExternalRouteRef = createExternalRouteRef({ + // highlight-next-line + defaultTarget: 'scaffolder.createComponent', +}); +``` + ### Optional External Routes When creating an `ExternalRouteRef` it is possible to mark it as optional: From 7f8403952f3e304ac1c8d12ca9506b54233717de Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Wed, 8 May 2024 17:27:16 +0200 Subject: [PATCH 04/13] api-docs: add default route target Signed-off-by: Patrik Oldsberg --- .changeset/chilled-planes-sniff.md | 5 +++++ plugins/api-docs/src/routes.ts | 1 + 2 files changed, 6 insertions(+) create mode 100644 .changeset/chilled-planes-sniff.md diff --git a/.changeset/chilled-planes-sniff.md b/.changeset/chilled-planes-sniff.md new file mode 100644 index 0000000000..e4084ea561 --- /dev/null +++ b/.changeset/chilled-planes-sniff.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-api-docs': patch +--- + +The `registerComponent` external route will now by default bind to the catalog import page if it is available. diff --git a/plugins/api-docs/src/routes.ts b/plugins/api-docs/src/routes.ts index 2103672eb0..295a263d6d 100644 --- a/plugins/api-docs/src/routes.ts +++ b/plugins/api-docs/src/routes.ts @@ -26,4 +26,5 @@ export const rootRoute = createRouteRef({ export const registerComponentRouteRef = createExternalRouteRef({ id: 'register-component', optional: true, + defaultTarget: 'catalog-import.importPage', }); From 863a800d4e1ba88a830a73aacb599db777b7e652 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Wed, 8 May 2024 17:33:52 +0200 Subject: [PATCH 05/13] catalog: add default route targets Signed-off-by: Patrik Oldsberg --- .changeset/rich-teachers-agree.md | 9 +++++++++ plugins/catalog/src/routes.ts | 3 +++ 2 files changed, 12 insertions(+) create mode 100644 .changeset/rich-teachers-agree.md diff --git a/.changeset/rich-teachers-agree.md b/.changeset/rich-teachers-agree.md new file mode 100644 index 0000000000..eed8517619 --- /dev/null +++ b/.changeset/rich-teachers-agree.md @@ -0,0 +1,9 @@ +--- +'@backstage/plugin-catalog': minor +--- + +Added the following default targets for external routes: + +- `createComponent` binds to the Scaffolder page. +- `viewTechDoc` binds to the TechDocs entity documentation page. +- `createFromTemplate` binds to the Scaffolder selected template page. diff --git a/plugins/catalog/src/routes.ts b/plugins/catalog/src/routes.ts index dcc46c7988..4f7f0427b1 100644 --- a/plugins/catalog/src/routes.ts +++ b/plugins/catalog/src/routes.ts @@ -22,18 +22,21 @@ import { export const createComponentRouteRef = createExternalRouteRef({ id: 'create-component', optional: true, + defaultTarget: 'scaffolder.createComponent', }); export const viewTechDocRouteRef = createExternalRouteRef({ id: 'view-techdoc', optional: true, params: ['namespace', 'kind', 'name'], + defaultTarget: 'techdocs.docRoot', }); export const createFromTemplateRouteRef = createExternalRouteRef({ id: 'create-from-template', optional: true, params: ['namespace', 'templateName'], + defaultTarget: 'scaffolder.selectedTemplate', }); export const unregisterRedirectRouteRef = createExternalRouteRef({ From cd6aeea6a2bcb98824ce7c588460f66bb374b78f Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Wed, 8 May 2024 17:35:17 +0200 Subject: [PATCH 06/13] catalog-graph: add default route target Signed-off-by: Patrik Oldsberg --- .changeset/proud-readers-move.md | 5 +++++ plugins/catalog-graph/src/routes.ts | 1 + 2 files changed, 6 insertions(+) create mode 100644 .changeset/proud-readers-move.md diff --git a/.changeset/proud-readers-move.md b/.changeset/proud-readers-move.md new file mode 100644 index 0000000000..70d94b2e6d --- /dev/null +++ b/.changeset/proud-readers-move.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-catalog-graph': patch +--- + +The `catalogEntity` external route will now by default bind to the catalog entity page if it is available. diff --git a/plugins/catalog-graph/src/routes.ts b/plugins/catalog-graph/src/routes.ts index 924db7fa4c..116ff27fe5 100644 --- a/plugins/catalog-graph/src/routes.ts +++ b/plugins/catalog-graph/src/routes.ts @@ -38,4 +38,5 @@ export const catalogEntityRouteRef = createExternalRouteRef({ id: 'catalog-entity', params: ['namespace', 'kind', 'name'], optional: true, + defaultTarget: 'catalog.catalogEntity', }); From d8e2f53bab9b3e99bc607379620fd654dfd0ba12 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Wed, 8 May 2024 17:36:40 +0200 Subject: [PATCH 07/13] org: add default route target and make optional Signed-off-by: Patrik Oldsberg --- .changeset/green-apricots-invite.md | 5 ++ plugins/org/api-report-alpha.md | 2 +- plugins/org/api-report.md | 2 +- .../Cards/OwnershipCard/ComponentsGrid.tsx | 55 +++++++++++-------- plugins/org/src/routes.ts | 2 + 5 files changed, 40 insertions(+), 26 deletions(-) create mode 100644 .changeset/green-apricots-invite.md diff --git a/.changeset/green-apricots-invite.md b/.changeset/green-apricots-invite.md new file mode 100644 index 0000000000..b5e92021b4 --- /dev/null +++ b/.changeset/green-apricots-invite.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-org': patch +--- + +The `catalogIndex` external route is now optional and will by default bind to the catalog index page if it is available. diff --git a/plugins/org/api-report-alpha.md b/plugins/org/api-report-alpha.md index 3cee584db0..8c5391d857 100644 --- a/plugins/org/api-report-alpha.md +++ b/plugins/org/api-report-alpha.md @@ -10,7 +10,7 @@ import { ExternalRouteRef } from '@backstage/frontend-plugin-api'; const _default: BackstagePlugin< {}, { - catalogIndex: ExternalRouteRef; + catalogIndex: ExternalRouteRef; } >; export default _default; diff --git a/plugins/org/api-report.md b/plugins/org/api-report.md index 1960ce839b..a6926d71f2 100644 --- a/plugins/org/api-report.md +++ b/plugins/org/api-report.md @@ -70,7 +70,7 @@ export const MyGroupsSidebarItem: (props: { const orgPlugin: BackstagePlugin< {}, { - catalogIndex: ExternalRouteRef; + catalogIndex: ExternalRouteRef; } >; export { orgPlugin }; diff --git a/plugins/org/src/components/Cards/OwnershipCard/ComponentsGrid.tsx b/plugins/org/src/components/Cards/OwnershipCard/ComponentsGrid.tsx index ff16a4ab2d..69ad5bf8bb 100644 --- a/plugins/org/src/components/Cards/OwnershipCard/ComponentsGrid.tsx +++ b/plugins/org/src/components/Cards/OwnershipCard/ComponentsGrid.tsx @@ -69,38 +69,45 @@ const EntityCountTile = ({ counter: number; type?: string; kind: string; - url: string; + url?: string; }) => { const classes = useStyles({ type: type ?? kind }); const rawTitle = type ?? kind; const isLongText = rawTitle.length > 10; - return ( - - - - {counter} + const tile = ( + + + {counter} + + + + - - - - - - {type && {kind}} - + {type && {kind}} + ); + + if (url) { + return ( + + {tile} + + ); + } + return tile; }; export const ComponentsGrid = ({ @@ -138,7 +145,7 @@ export const ComponentsGrid = ({ counter={c.counter} kind={c.kind} type={c.type} - url={`${catalogLink()}/?${c.queryParams}`} + url={catalogLink && `${catalogLink()}/?${c.queryParams}`} /> ))} diff --git a/plugins/org/src/routes.ts b/plugins/org/src/routes.ts index aa24c1b1cc..6ca1eb7b43 100644 --- a/plugins/org/src/routes.ts +++ b/plugins/org/src/routes.ts @@ -18,4 +18,6 @@ import { createExternalRouteRef } from '@backstage/core-plugin-api'; export const catalogIndexRouteRef = createExternalRouteRef({ id: 'catalog-index', + optional: true, + defaultTarget: 'catalog.catalogIndex', }); From 60085dd33acd40a4e37c2cfc218349529d580c15 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Wed, 8 May 2024 17:37:59 +0200 Subject: [PATCH 08/13] scaffolder: add default route targets Signed-off-by: Patrik Oldsberg --- .changeset/warm-mayflies-yell.md | 8 ++++++++ plugins/scaffolder/src/routes.ts | 2 ++ 2 files changed, 10 insertions(+) create mode 100644 .changeset/warm-mayflies-yell.md diff --git a/.changeset/warm-mayflies-yell.md b/.changeset/warm-mayflies-yell.md new file mode 100644 index 0000000000..a2068668f7 --- /dev/null +++ b/.changeset/warm-mayflies-yell.md @@ -0,0 +1,8 @@ +--- +'@backstage/plugin-scaffolder': minor +--- + +Added the following default targets for external routes: + +- `registerComponent` binds to the catalog import page. +- `viewTechDoc` binds to the TechDocs entity documentation page. diff --git a/plugins/scaffolder/src/routes.ts b/plugins/scaffolder/src/routes.ts index 54b7605eb1..8ef77e91e3 100644 --- a/plugins/scaffolder/src/routes.ts +++ b/plugins/scaffolder/src/routes.ts @@ -22,12 +22,14 @@ import { export const registerComponentRouteRef = createExternalRouteRef({ id: 'register-component', optional: true, + defaultTarget: 'catalog-import.importPage', }); export const viewTechDocRouteRef = createExternalRouteRef({ id: 'view-techdoc', optional: true, params: ['namespace', 'kind', 'name'], + defaultTarget: 'techdocs.docRoot', }); /** From f26861e30ef031ed47567280333b9ca4b2092259 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Wed, 8 May 2024 17:46:16 +0200 Subject: [PATCH 09/13] core-app-api: port over the rest of resolveRouteBindings tests Signed-off-by: Patrik Oldsberg --- .../src/app/resolveRouteBindings.test.ts | 127 ++++++++++++++++++ 1 file changed, 127 insertions(+) diff --git a/packages/core-app-api/src/app/resolveRouteBindings.test.ts b/packages/core-app-api/src/app/resolveRouteBindings.test.ts index 200be4f367..9dfbbfd452 100644 --- a/packages/core-app-api/src/app/resolveRouteBindings.test.ts +++ b/packages/core-app-api/src/app/resolveRouteBindings.test.ts @@ -50,6 +50,133 @@ describe('resolveRouteBindings', () => { ), ).toThrow('Key someOtherRoute is not an existing external route'); }); + + it('reads bindings from config', () => { + const mySource = createExternalRouteRef({ id: 'test' }); + const myTarget = createRouteRef({ id: 'test' }); + const result = resolveRouteBindings( + () => {}, + new MockConfigApi({ + app: { routes: { bindings: { 'test.mySource': 'test.myTarget' } } }, + }), + [ + createPlugin({ + id: 'test', + routes: { + myTarget, + }, + externalRoutes: { + mySource, + }, + }), + ], + ); + + expect(result.get(mySource)).toBe(myTarget); + }); + + it('throws on invalid config', () => { + expect(() => + resolveRouteBindings( + () => {}, + new MockConfigApi({ app: { routes: { bindings: 'derp' } } }), + [], + ), + ).toThrow( + "Invalid type in config for key 'app.routes.bindings' in 'mock-config', got string, wanted object", + ); + + expect(() => + resolveRouteBindings( + () => {}, + new MockConfigApi({ + app: { routes: { bindings: { 'test.mySource': true } } }, + }), + [], + ), + ).toThrow( + "Invalid config at app.routes.bindings['test.mySource'], value must be a non-empty string", + ); + + expect(() => + resolveRouteBindings( + () => {}, + new MockConfigApi({ + app: { routes: { bindings: { 'test.mySource': 'test.myTarget' } } }, + }), + [], + ), + ).toThrow( + "Invalid config at app.routes.bindings, 'test.mySource' is not a valid external route", + ); + + expect(() => + resolveRouteBindings( + () => {}, + new MockConfigApi({ + app: { routes: { bindings: { 'test.mySource': 'test.myTarget' } } }, + }), + [ + createPlugin({ + id: 'test', + externalRoutes: { + mySource: createExternalRouteRef({ id: 'test' }), + }, + }), + ], + ), + ).toThrow( + "Invalid config at app.routes.bindings['test.mySource'], 'test.myTarget' is not a valid route", + ); + }); + + it('can have default targets, but at the lowest priority', () => { + const source = createExternalRouteRef({ + id: 'test', + defaultTarget: 'test.target1', + }); + const target1 = createRouteRef({ id: 'test' }); + const target2 = createRouteRef({ id: 'test' }); + const plugin = createPlugin({ + id: 'test', + routes: { + target1, + target2, + }, + externalRoutes: { + source, + }, + }); + + // defaultTarget wins only if no bind or config matches + let result = resolveRouteBindings(() => {}, new MockConfigApi({}), [ + plugin, + ]); + + expect(result.get(source)).toBe(target1); + + // config wins over defaultTarget + result = resolveRouteBindings( + () => {}, + new MockConfigApi({ + app: { routes: { bindings: { 'test.source': 'test.target2' } } }, + }), + [plugin], + ); + + expect(result.get(source)).toBe(target2); + + // bind wins over defaultTarget + result = resolveRouteBindings( + ({ bind }) => { + bind(plugin.externalRoutes, { source: plugin.routes.target2 }); + }, + new MockConfigApi({}), + [plugin], + ); + + expect(result.get(source)).toBe(target2); + }); }); describe('collectRouteIds', () => { From a85c6f57e39a10d69fb7f3825c0c4087cf0abb2c Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Wed, 8 May 2024 17:48:57 +0200 Subject: [PATCH 10/13] app: remove unnecessary route bindings Signed-off-by: Patrik Oldsberg --- packages/app/src/App.tsx | 34 ++++------------------------------ 1 file changed, 4 insertions(+), 30 deletions(-) diff --git a/packages/app/src/App.tsx b/packages/app/src/App.tsx index 652582984e..d741dea098 100644 --- a/packages/app/src/App.tsx +++ b/packages/app/src/App.tsx @@ -33,22 +33,14 @@ import { OAuthRequestDialog, SignInPage, } from '@backstage/core-components'; -import { apiDocsPlugin, ApiExplorerPage } from '@backstage/plugin-api-docs'; -import { - CatalogEntityPage, - CatalogIndexPage, - catalogPlugin, -} from '@backstage/plugin-catalog'; +import { ApiExplorerPage } from '@backstage/plugin-api-docs'; +import { CatalogEntityPage, CatalogIndexPage } from '@backstage/plugin-catalog'; import { CatalogGraphPage } from '@backstage/plugin-catalog-graph'; -import { - CatalogImportPage, - catalogImportPlugin, -} from '@backstage/plugin-catalog-import'; -import { orgPlugin } from '@backstage/plugin-org'; +import { CatalogImportPage } from '@backstage/plugin-catalog-import'; import { HomepageCompositionRoot, VisitListener } from '@backstage/plugin-home'; -import { ScaffolderPage, scaffolderPlugin } from '@backstage/plugin-scaffolder'; +import { ScaffolderPage } from '@backstage/plugin-scaffolder'; import { ScaffolderFieldExtensions, ScaffolderLayouts, @@ -56,7 +48,6 @@ import { import { SearchPage } from '@backstage/plugin-search'; import { TechDocsIndexPage, - techdocsPlugin, TechDocsReaderPage, } from '@backstage/plugin-techdocs'; import { TechDocsAddons } from '@backstage/plugin-techdocs-react'; @@ -119,23 +110,6 @@ const app = createApp({ ); }, }, - bindRoutes({ bind }) { - bind(catalogPlugin.externalRoutes, { - createComponent: scaffolderPlugin.routes.root, - viewTechDoc: techdocsPlugin.routes.docRoot, - createFromTemplate: scaffolderPlugin.routes.selectedTemplate, - }); - bind(apiDocsPlugin.externalRoutes, { - registerApi: catalogImportPlugin.routes.importPage, - }); - bind(scaffolderPlugin.externalRoutes, { - registerComponent: catalogImportPlugin.routes.importPage, - viewTechDoc: techdocsPlugin.routes.docRoot, - }); - bind(orgPlugin.externalRoutes, { - catalogIndex: catalogPlugin.routes.catalogIndex, - }); - }, }); const routes = ( From 5d464f82752406d1fd82d5db47ad3192b00b1c81 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Mon, 3 Jun 2024 16:59:33 +0200 Subject: [PATCH 11/13] core-app-api: sticky error for route resolution too Signed-off-by: Patrik Oldsberg --- packages/core-app-api/src/app/AppManager.tsx | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/packages/core-app-api/src/app/AppManager.tsx b/packages/core-app-api/src/app/AppManager.tsx index 48501b52de..b28b843d34 100644 --- a/packages/core-app-api/src/app/AppManager.tsx +++ b/packages/core-app-api/src/app/AppManager.tsx @@ -297,22 +297,22 @@ 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, - ); - + if (routeValidationError) { + throw routeValidationError; + } else if (!routeBindings) { try { + routeBindings = resolveRouteBindings( + this.bindRoutes, + loadedConfig.api, + this.plugins, + ); + 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 From edc430d7b1e33f15e04d7fd96a8f53bc45c3b6fc Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Mon, 3 Jun 2024 17:03:29 +0200 Subject: [PATCH 12/13] core-compat-api: safer getDefaultTarget conversion Signed-off-by: Patrik Oldsberg --- packages/core-compat-api/src/convertLegacyRouteRef.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/core-compat-api/src/convertLegacyRouteRef.ts b/packages/core-compat-api/src/convertLegacyRouteRef.ts index 37277fc422..c967f9e9b5 100644 --- a/packages/core-compat-api/src/convertLegacyRouteRef.ts +++ b/packages/core-compat-api/src/convertLegacyRouteRef.ts @@ -204,7 +204,9 @@ export function convertLegacyRouteRef( return legacyRefStr; }, // This might already be implemented in the legacy ref, but we override it just to be sure - getDefaultTarget: newRef.getDefaultTarget, + getDefaultTarget() { + return newRef.getDefaultTarget(); + }, setId(id: string) { newRef.setId(id); }, From d55b40a67ddbb81c12df050fc28b57e33b187d09 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Mon, 3 Jun 2024 17:04:43 +0200 Subject: [PATCH 13/13] docs: language fix Signed-off-by: Patrik Oldsberg --- docs/frontend-system/architecture/07-routes.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/frontend-system/architecture/07-routes.md b/docs/frontend-system/architecture/07-routes.md index 42750fad39..99cfbae937 100644 --- a/docs/frontend-system/architecture/07-routes.md +++ b/docs/frontend-system/architecture/07-routes.md @@ -278,7 +278,7 @@ Another thing to note is that this indirection in the routing is particularly us It is possible to define a default target for an external route reference, potentially removing the need to bind the route in the app. This reduces the need for configuration when installing new plugins through providing a sensible default. It is of course still possible to override the route binding in the app. -The default target uses the same syntax as the route binding configuration, and will only be used if the target plugin an route exists. For example, this is how the catalog can define a default target for the create component external route in a way that removes the need for the binding in the previous example: +The default target uses the same syntax as the route binding configuration, and will only be used if the target plugin and route exist. For example, this is how the catalog can define a default target for the create component external route in a way that removes the need for the binding in the previous example: ```tsx title="plugins/catalog/src/routes.ts" import { createExternalRouteRef } from '@backstage/frontend-plugin-api';