From a0fd23ed04341d87f5d536c6b98526c9d78e5c3d Mon Sep 17 00:00:00 2001 From: David Festal Date: Tue, 11 Mar 2025 22:41:23 +0100 Subject: [PATCH] Fix review comments Signed-off-by: David Festal --- packages/frontend-defaults/report.api.md | 3 +- packages/frontend-defaults/src/createApp.tsx | 12 +-- .../frontend-defaults/src/discovery.test.ts | 3 +- packages/frontend-defaults/src/discovery.ts | 55 +++++-------- packages/frontend-defaults/src/resolution.ts | 77 ++++++++++--------- .../createFrontendFeatureLoader.test.ts | 18 ++++- 6 files changed, 81 insertions(+), 87 deletions(-) diff --git a/packages/frontend-defaults/report.api.md b/packages/frontend-defaults/report.api.md index abee0b0ef0..8bfe2ee017 100644 --- a/packages/frontend-defaults/report.api.md +++ b/packages/frontend-defaults/report.api.md @@ -54,8 +54,7 @@ export function createPublicSignInApp(options?: CreateAppOptions): { // @public (undocumented) export function discoverAvailableFeatures(config: Config): { - features: FrontendFeature[]; - featureLoaders?: FrontendFeatureLoader[]; + features: (FrontendFeature | FrontendFeatureLoader)[]; }; // @public (undocumented) diff --git a/packages/frontend-defaults/src/createApp.tsx b/packages/frontend-defaults/src/createApp.tsx index 7dd5a6eb15..c6ed4759fa 100644 --- a/packages/frontend-defaults/src/createApp.tsx +++ b/packages/frontend-defaults/src/createApp.tsx @@ -100,17 +100,11 @@ export function createApp(options?: CreateAppOptions): { overrideBaseUrlConfigs(defaultConfigLoaderSync()), ); - const { - features: discoveredFeatures, - featureLoaders: discoveredFeatureLoaders, - } = discoverAvailableFeatures(config); + const { features: discoveredFeaturesAndLoaders } = + discoverAvailableFeatures(config); const { features: loadedFeatures } = await resolveAsyncFeatures({ config, - features: [ - ...discoveredFeatures, - ...(discoveredFeatureLoaders ?? []), - ...(options?.features ?? []), - ], + features: [...discoveredFeaturesAndLoaders, ...(options?.features ?? [])], }); const app = createSpecializedApp({ diff --git a/packages/frontend-defaults/src/discovery.test.ts b/packages/frontend-defaults/src/discovery.test.ts index f939495da2..52a2fcde11 100644 --- a/packages/frontend-defaults/src/discovery.test.ts +++ b/packages/frontend-defaults/src/discovery.test.ts @@ -64,8 +64,7 @@ describe('discoverAvailableFeatures', () => { modules: [{ default: testLoader }], }); expect(discoverAvailableFeatures(config)).toEqual({ - features: [], - featureLoaders: [testLoader], + features: [testLoader], }); }); diff --git a/packages/frontend-defaults/src/discovery.ts b/packages/frontend-defaults/src/discovery.ts index 473315d3bf..6da6146389 100644 --- a/packages/frontend-defaults/src/discovery.ts +++ b/packages/frontend-defaults/src/discovery.ts @@ -19,6 +19,7 @@ import { FrontendFeature, FrontendFeatureLoader, } from '@backstage/frontend-plugin-api'; +import { isBackstageFeatureLoader } from './resolution'; interface DiscoveryGlobal { modules: Array<{ name: string; export?: string; default: unknown }>; @@ -59,8 +60,7 @@ function readPackageDetectionConfig(config: Config) { * @public */ export function discoverAvailableFeatures(config: Config): { - features: FrontendFeature[]; - featureLoaders?: FrontendFeatureLoader[]; + features: (FrontendFeature | FrontendFeatureLoader)[]; } { const discovered = ( window as { '__@backstage/discovered__'?: DiscoveryGlobal } @@ -71,34 +71,20 @@ export function discoverAvailableFeatures(config: Config): { return { features: [] }; } - const detectedExports = discovered?.modules - .filter(({ name }) => { - if (detection.exclude?.includes(name)) { - return false; - } - if (detection.include && !detection.include.includes(name)) { - return false; - } - return true; - }) - .map(m => m.default); - - if (detectedExports === undefined) { - return { - features: [], - }; - } - - const features = detectedExports.filter(isBackstageFeature); - const detectedFeatureLoaders = detectedExports.filter( - isBackstageFeatureLoader, - ); - const featureLoaders = - detectedFeatureLoaders.length > 0 ? detectedFeatureLoaders : undefined; - return { - features, - featureLoaders, + features: + discovered?.modules + .filter(({ name }) => { + if (detection.exclude?.includes(name)) { + return false; + } + if (detection.include && !detection.include.includes(name)) { + return false; + } + return true; + }) + .map(m => m.default) + .filter(isFeatureOrLoader) ?? [], }; } @@ -112,13 +98,8 @@ function isBackstageFeature(obj: unknown): obj is FrontendFeature { return false; } -export function isBackstageFeatureLoader( +function isFeatureOrLoader( obj: unknown, -): obj is FrontendFeatureLoader { - return ( - obj !== null && - typeof obj === 'object' && - '$$type' in obj && - obj.$$type === '@backstage/FrontendFeatureLoader' - ); +): obj is FrontendFeature | FrontendFeatureLoader { + return isBackstageFeature(obj) || isBackstageFeatureLoader(obj); } diff --git a/packages/frontend-defaults/src/resolution.ts b/packages/frontend-defaults/src/resolution.ts index 147aa06bc2..6ab26c3d1d 100644 --- a/packages/frontend-defaults/src/resolution.ts +++ b/packages/frontend-defaults/src/resolution.ts @@ -23,7 +23,6 @@ import { import { CreateAppFeatureLoader } from './createApp'; // eslint-disable-next-line @backstage/no-relative-monorepo-imports import { isInternalFrontendFeatureLoader } from '../../frontend-plugin-api/src/wiring/createFrontendFeatureLoader'; -import { isBackstageFeatureLoader } from './discovery'; /** @public */ export async function resolveAsyncFeatures(options: { @@ -34,7 +33,7 @@ export async function resolveAsyncFeatures(options: { | CreateAppFeatureLoader )[]; }): Promise<{ features: FrontendFeature[] }> { - const featuresOrLoaders: (FrontendFeature | FrontendFeatureLoader)[] = []; + const features: (FrontendFeature | FrontendFeatureLoader)[] = []; // Separate deprecated CreateAppFeatureLoader elements from the frontend features, // and manage the deprecated elements first. @@ -42,7 +41,7 @@ export async function resolveAsyncFeatures(options: { if ('load' in item) { try { const result = await item.load({ config: options.config }); - featuresOrLoaders.push(...result.features); + features.push(...result.features); } catch (e) { throw new Error( `Failed to read frontend features from loader '${item.getLoaderName()}', ${stringifyError( @@ -51,7 +50,7 @@ export async function resolveAsyncFeatures(options: { ); } } else { - featuresOrLoaders.push(item); + features.push(item); } } @@ -60,49 +59,55 @@ export async function resolveAsyncFeatures(options: { const maxRecursionDepth = 5; async function applyFeatureLoaders( - toLoad: (FrontendFeature | FrontendFeatureLoader)[], + featuresOrLoaders: (FrontendFeature | FrontendFeatureLoader)[], recursionDepth: number, ) { if (featuresOrLoaders.length === 0) { return; } - const featureLoaders: FrontendFeatureLoader[] = []; - for (const item of toLoad) { - if (isBackstageFeatureLoader(item)) { - featureLoaders.push(item); + for (const featureOrLoader of featuresOrLoaders) { + if (isBackstageFeatureLoader(featureOrLoader)) { + if (alreadyMetFeatureLoaders.some(l => l === featureOrLoader)) { + continue; + } + if (isInternalFrontendFeatureLoader(featureOrLoader)) { + if (recursionDepth > maxRecursionDepth) { + throw new Error( + `Maximum feature loading recursion depth (${maxRecursionDepth}) reached for the feature loader ${featureOrLoader.description}`, + ); + } + alreadyMetFeatureLoaders.push(featureOrLoader); + let result: (FrontendFeature | FrontendFeatureLoader)[]; + try { + result = await featureOrLoader.loader({ config: options.config }); + } catch (e) { + throw new Error( + `Failed to read frontend features from loader ${ + featureOrLoader.description + }: ${stringifyError(e)}`, + ); + } + await applyFeatureLoaders(result, recursionDepth + 1); + } } else { - loadedFeatures.push(item); - } - } - - for (const featureLoader of featureLoaders) { - if (alreadyMetFeatureLoaders.some(l => l === featureLoader)) { - continue; - } - if (isInternalFrontendFeatureLoader(featureLoader)) { - if (recursionDepth > maxRecursionDepth) { - throw new Error( - `Maximum feature loading recursion depth (${maxRecursionDepth}) reached for the feature loader ${featureLoader.description}`, - ); - } - alreadyMetFeatureLoaders.push(featureLoader); - let result: (FrontendFeature | FrontendFeatureLoader)[]; - try { - result = await featureLoader.loader({ config: options.config }); - } catch (e) { - throw new Error( - `Failed to read frontend features from loader ${ - featureLoader.description - }: ${stringifyError(e)}`, - ); - } - await applyFeatureLoaders(result, recursionDepth + 1); + loadedFeatures.push(featureOrLoader); } } } - await applyFeatureLoaders(featuresOrLoaders, 1); + await applyFeatureLoaders(features, 1); return { features: loadedFeatures }; } + +export function isBackstageFeatureLoader( + obj: unknown, +): obj is FrontendFeatureLoader { + return ( + obj !== null && + typeof obj === 'object' && + '$$type' in obj && + obj.$$type === '@backstage/FrontendFeatureLoader' + ); +} diff --git a/packages/frontend-plugin-api/src/wiring/createFrontendFeatureLoader.test.ts b/packages/frontend-plugin-api/src/wiring/createFrontendFeatureLoader.test.ts index 310623d541..f240b4f678 100644 --- a/packages/frontend-plugin-api/src/wiring/createFrontendFeatureLoader.test.ts +++ b/packages/frontend-plugin-api/src/wiring/createFrontendFeatureLoader.test.ts @@ -165,6 +165,22 @@ describe('createFrontendFeatureLoader', () => { createFrontendFeatureLoader({ async loader(_) { return [ + createFrontendPlugin({ + id: 'plugin-0', + extensions: [ + createExtension({ + name: '0', + attachTo: { + id: 'plugin-output/output', + input: 'names', + }, + output: [nameExtensionDataRef], + factory() { + return [nameExtensionDataRef('extension-0')]; + }, + }), + ], + }), createFrontendFeatureLoader({ async *loader(__) { yield createFrontendPlugin({ @@ -248,7 +264,7 @@ describe('createFrontendFeatureLoader', () => { ); await expect( - screen.findByText('Names: extension-1, extension-2'), + screen.findByText('Names: extension-0, extension-1, extension-2'), ).resolves.toBeInTheDocument(); });