Fix review comments

Signed-off-by: David Festal <dfestal@redhat.com>

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)

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({

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],
     });
   });
 

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);
 }

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'
+  );
+}

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();
   });