refactor: address PR comments

docs/plugins/index.md

@@ -27,9 +27,9 @@ This helps the community know what plugins are in development.
 You can also use this process if you have an idea for a good plugin but you hope
 that someone else will pick up the work.
 
-## Integrate into the catalog service
+## Integrate into the Service Catalog
 
 If your plugin isn't supposed to live as a standalone page, but rather needs to
-be presented as a part of a catalog service (e.g. a separate tab or a card on an
+be presented as a part of a Service Catalog (e.g. a separate tab or a card on an
 "Overview" tab), then check out
 [the instruction](integrating-plugin-into-service-catalog.md). on how to do it.

docs/plugins/integrating-plugin-into-service-catalog.md

@@ -1,6 +1,6 @@
 ---
 id: integrating-plugin-into-service-catalog
-title: Integrate into the catalog service
+title: Integrate into the Service Catalog
 ---
 
 > This is an advanced use case and currently is an experimental feature. Expect
@@ -119,7 +119,6 @@ type EntityPageLayoutContentProps = {
 };
 ```
 
-> The recommended pattern is to get the `entity` in the App and then pass it to
-> the plugin's Router component as a prop. However if it inconvenient for you,
+> You can either pass the entity from App to the plugin's router as a prop or
 > use `useEntity` hook from `@backstage/plugin-catalog` directly inside your
 > plugin.

packages/app/src/App.tsx

@@ -26,8 +26,7 @@ import * as plugins from './plugins';
 import { apis } from './apis';
 import { hot } from 'react-hot-loader/root';
 import { providers } from './identityProviders';
-import { CatalogRouter } from '@backstage/plugin-catalog';
-// import { ExplorePlugin } from '@backstage/plugin-explore';
+import { Router as CatalogRouter } from '@backstage/plugin-catalog';
 import { Route, Routes, Navigate } from 'react-router';
 
 import { EntityPage } from './components/catalog/EntityPage';
@@ -51,6 +50,7 @@ const app = createApp({
 
 const AppProvider = app.getProvider();
 const AppRouter = app.getRouter();
+const deprecatedAppRoutes = app.getRoutes();
 
 const AppRoutes = () => (
   <Routes>
@@ -58,8 +58,8 @@ const AppRoutes = () => (
       path="/catalog/*"
       element={<CatalogRouter EntityPage={EntityPage} />}
     />
-    <Route path="/" element={<Navigate to="/catalog" />} />
-    {/* <Route path="/explore" element={<ExplorePlugin />} /> */}
+    <Navigate key="/" to="/catalog" />
+    {...deprecatedAppRoutes}
   </Routes>
 );
 

packages/app/src/components/catalog/EntityPage.tsx

@@ -21,9 +21,14 @@ import {
   AboutCard,
 } from '@backstage/plugin-catalog';
 import { Entity } from '@backstage/catalog-model';
+import { Grid } from '@material-ui/core';
 
 const OverviewPage = ({ entity }: { entity: Entity }) => (
-  <AboutCard entity={entity} />
+  <Grid container spacing={3}>
+    <Grid item>
+      <AboutCard entity={entity} />
+    </Grid>
+  </Grid>
 );
 
 const ServiceEntityPage = ({ entity }: { entity: Entity }) => (

packages/core-api/src/app/App.tsx

@@ -136,7 +136,7 @@ export class PrivateAppImpl implements BackstageApp {
     return this.icons[key];
   }
 
-  getRoutes(): ComponentType<{}> {
+  getRoutes(): JSX.Element[] {
     const routes = new Array<JSX.Element>();
     const registeredFeatureFlags = new Array<FeatureFlagsRegistryItem>();
 
@@ -191,14 +191,9 @@ export class PrivateAppImpl implements BackstageApp {
       FeatureFlags.registeredFeatureFlags = registeredFeatureFlags;
     }
 
-    const rendered = (
-      <Routes>
-        {routes}
-        <Route element={<NotFoundErrorPage />} />
-      </Routes>
-    );
+    routes.push(<Route element={<NotFoundErrorPage />} />);
 
-    return () => rendered;
+    return routes;
   }
 
   getProvider(): ComponentType<{}> {

packages/core-api/src/app/types.ts

@@ -168,5 +168,5 @@ export type BackstageApp = {
   /**
    * Routes component that contains all routes for plugin pages in the app.
    */
-  getRoutes(): ComponentType<{}>;
+  getRoutes(): JSX.Element[];
 };

packages/dev-utils/src/devApp/render.tsx

@@ -85,7 +85,7 @@ class DevAppBuilder {
 
     const AppProvider = app.getProvider();
     const AppRouter = app.getRouter();
-    const AppRoutes = app.getRoutes();
+    const deprecatedAppRoutes = app.getRoutes();
 
     const sidebar = this.setupSidebar(this.plugins);
 
@@ -99,7 +99,7 @@ class DevAppBuilder {
           <AppRouter>
             <SidebarPage>
               {sidebar}
-              <AppRoutes />
+              {deprecatedAppRoutes}
             </SidebarPage>
           </AppRouter>
         </AppProvider>

packages/e2e-test/src/e2e-test.ts

@@ -269,7 +269,7 @@ async function createPlugin(pluginName: string, appDir: string) {
 /**
  * Start serving the newly created app and make sure that the create plugin is rendering correctly
  */
-async function testAppServe(_pluginName: string, appDir: string) {
+async function testAppServe(pluginName: string, appDir: string) {
   const startApp = spawnPiped(['yarn', 'start'], {
     cwd: appDir,
   });
@@ -280,12 +280,11 @@ async function testAppServe(_pluginName: string, appDir: string) {
     const browser = new Browser();
 
     await waitForPageWithText(browser, '/', 'Backstage Service Catalog');
-    // TODO(shmidt-i): adjust the plugin creation flow with new routing patterns
-    // await waitForPageWithText(
-    //   browser,
-    //   `/${pluginName}`,
-    //   `Welcome to ${pluginName}!`,
-    // );
+    await waitForPageWithText(
+      browser,
+      `/${pluginName}`,
+      `Welcome to ${pluginName}!`,
+    );
 
     print('Both App and Plugin loaded correctly');
     successful = true;

plugins/catalog/src/components/EntityPageLayout/EntityPageLayout.tsx

@@ -13,10 +13,10 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
-import React, { useState } from 'react';
+import React, { useState, useContext } from 'react';
 import { useParams, useNavigate } from 'react-router';
 
-import { useEntity } from '../../hooks/useEntity';
+import { EntityContext } from '../../hooks/useEntity';
 import {
   pageTheme,
   PageTheme,
@@ -74,7 +74,7 @@ function headerProps(
 export const EntityPageLayout = ({
   children,
 }: {
-  children: React.ReactNode;
+  children?: React.ReactNode;
 }) => {
   const { optionalNamespaceAndName, kind } = useParams() as {
     optionalNamespaceAndName: string;
@@ -82,7 +82,7 @@ export const EntityPageLayout = ({
   };
   const [name, namespace] = optionalNamespaceAndName.split(':').reverse();
 
-  const { entity, loading, error } = useEntity();
+  const { entity, loading, error } = useContext(EntityContext);
   const { headerTitle, headerType } = headerProps(
     kind,
     namespace,
@@ -139,5 +139,4 @@ export const EntityPageLayout = ({
     </Page>
   );
 };
-
 EntityPageLayout.Content = Tabbed.Content;

plugins/catalog/src/components/EntityPageLayout/Tabbed/Tabbed.test.tsx

@@ -36,6 +36,21 @@ describe('Tabbed layout', () => {
     expect(rendered.getByText('tabbed-test-content')).toBeInTheDocument();
   });
 
+  it('throws if any other component is a child of Tabbed.Layout', async () => {
+    await expect(
+      renderInTestApp(
+        <Tabbed.Layout>
+          <Tabbed.Content
+            title="tabbed-test-title"
+            path="*"
+            element={<div>tabbed-test-content</div>}
+          />
+          <div>This will cause app to throw</div>
+        </Tabbed.Layout>,
+      ),
+    ).rejects.toThrow(/EntityPageLayout component only accepts/);
+  });
+
   it('navigates when user clicks different tab', async () => {
     const rendered = await renderInTestApp(
       <Routes>

plugins/catalog/src/components/EntityPageLayout/Tabbed/Tabbed.tsx

@@ -25,7 +25,6 @@ import {
   RouteMatch,
 } from 'react-router';
 import { Tab, HeaderTabs, Content } from '@backstage/core';
-import { Grid } from '@material-ui/core';
 import { Helmet } from 'react-helmet';
 
 const getSelectedIndexOrDefault = (
@@ -67,6 +66,11 @@ export const Tabbed = {
         // Skip conditionals resolved to falses/nulls/undefineds etc
         return;
       }
+      if (child.type !== Tabbed.Content) {
+        throw new Error(
+          'This component only accepts Content elements as direct children. Check the code of the EntityPage.',
+        );
+      }
       const pathAndId = (child as JSX.Element).props.path;
 
       // Child here must be then always a functional component without any wrappers
@@ -92,7 +96,7 @@ export const Tabbed = {
       matchRoutes(routes as RouteObject[], `/${params['*']}`) ?? [];
     const selectedIndex = getSelectedIndexOrDefault(matchedRoute, tabs);
     const currentTab = tabs[selectedIndex];
-    const title = currentTab.label;
+    const title = currentTab?.label;
 
     const onTabChange = (index: number) =>
       // Remove trailing /*
@@ -103,6 +107,7 @@ export const Tabbed = {
 
     const currentRouteElement = useRoutes(routes);
 
+    if (!currentTab) return null;
     return (
       <>
         <HeaderTabs
@@ -111,10 +116,8 @@ export const Tabbed = {
           onChange={onTabChange}
         />
         <Content>
-          <Grid container spacing={3}>
-            <Helmet title={title} />
-            {currentRouteElement}
-          </Grid>
+          <Helmet title={title} />
+          {currentRouteElement}
         </Content>
       </>
     );

plugins/catalog/src/components/Router.tsx

@@ -14,13 +14,14 @@
  * limitations under the License.
  */
 import React, { ComponentType } from 'react';
-import { CatalogPage } from './components/CatalogPage';
-import { EntityPageLayout } from './components/EntityPageLayout';
+import { CatalogPage } from './CatalogPage';
+import { EntityPageLayout } from './EntityPageLayout';
 import { Route, Routes } from 'react-router';
-import { entityRoute, rootRoute, entityRouteDefault } from './routes';
+import { entityRoute, rootRoute } from '../routes';
 import { Content } from '@backstage/core';
 import { Typography, Link } from '@material-ui/core';
-import { EntityProvider } from './components/EntityProvider';
+import { EntityProvider } from './EntityProvider';
+import { useEntity } from '../hooks/useEntity';
 
 const DefaultEntityPage = () => (
   <EntityPageLayout>
@@ -43,7 +44,17 @@ const DefaultEntityPage = () => (
   </EntityPageLayout>
 );
 
-export const CatalogRouter = ({
+const EntityPageSwitch = ({ EntityPage }: { EntityPage: ComponentType }) => {
+  const { entity } = useEntity();
+  // Loading and error states
+  if (!entity) return <EntityPageLayout />;
+
+  // Otherwise EntityPage provided from the App
+  // Note that EntityPage will include EntityPageLayout already
+  return <EntityPage />;
+};
+
+export const Router = ({
   EntityPage = DefaultEntityPage,
 }: {
   EntityPage?: ComponentType;
@@ -54,15 +65,7 @@ export const CatalogRouter = ({
       path={`/${entityRoute.path}`}
       element={
         <EntityProvider>
-          <EntityPage />
-        </EntityProvider>
-      }
-    />
-    <Route
-      path={`/${entityRouteDefault.path}`}
-      element={
-        <EntityProvider>
-          <EntityPage />
+          <EntityPageSwitch EntityPage={EntityPage} />
         </EntityProvider>
       }
     />

plugins/catalog/src/hooks/useEntity.ts

@@ -24,13 +24,13 @@ const REDIRECT_DELAY = 2000;
 
 type EntityLoadingStatus = {
   entity?: Entity;
-  loading: boolean | null;
+  loading: boolean;
   error?: Error;
 };
 
 export const EntityContext = createContext<EntityLoadingStatus>({
-  entity: undefined as any,
-  loading: null,
+  entity: undefined,
+  loading: true,
   error: undefined,
 });
 
@@ -53,25 +53,20 @@ export const useEntityFromUrl = (): EntityLoadingStatus => {
         navigate('/');
       }, REDIRECT_DELAY);
     }
-  }, [errorApi, navigate, error, loading, entity]);
 
-  if (!name) {
-    navigate('/catalog');
-    return {
-      entity: undefined,
-      loading: null,
-      error: new Error('No name in url'),
-    } as never;
-  }
+    if (!name) {
+      errorApi.post(new Error('No name provided!'));
+      navigate('/');
+    }
+  }, [errorApi, navigate, error, loading, entity, name]);
 
   return { entity, loading, error };
 };
 
 /**
  * Always going to return an entity, or throw an error if not a descendant of a EntityProvider.
- * Otherwise the useEntityFromUrl will take care of the `undefined` entity
  */
-export const useEntity = () =>
-  useContext<Omit<EntityLoadingStatus, 'entity'> & { entity: Entity }>(
-    EntityContext as any,
-  );
+export const useEntity = () => {
+  const { entity } = useContext<{ entity: Entity }>(EntityContext as any);
+  return { entity };
+};

plugins/catalog/src/index.ts

@@ -19,7 +19,7 @@ export * from './api/CatalogClient';
 export * from './api/types';
 export * from './routes';
 export { useEntityCompoundName } from './components/useEntityCompoundName';
-export * from './Router';
+export { Router } from './components/Router';
 export { useEntity } from './hooks/useEntity';
 export { AboutCard } from './components/AboutCard';
 export { EntityPageLayout } from './components/EntityPageLayout';

plugins/catalog/src/routes.ts

@@ -28,8 +28,3 @@ export const entityRoute = createRouteRef({
   path: ':kind/:optionalNamespaceAndName/*',
   title: 'Entity',
 });
-export const entityRouteDefault = createRouteRef({
-  icon: NoIcon,
-  path: ':kind/:optionalNamespaceAndName/*',
-  title: 'Entity',
-});

plugins/scaffolder/src/components/JobStatusModal/JobStatusModal.tsx

@@ -26,7 +26,7 @@ import { useJobPolling } from './useJobPolling';
 import { Job } from '../../types';
 import { TemplateEntityV1alpha1 } from '@backstage/catalog-model';
 import { Button } from '@backstage/core';
-import { entityRouteDefault } from '@backstage/plugin-catalog';
+import { entityRoute } from '@backstage/plugin-catalog';
 import { generatePath } from 'react-router-dom';
 
 type Props = {
@@ -72,7 +72,7 @@ export const JobStatusModal = ({
       {entity && (
         <DialogActions>
           <Button
-            to={generatePath(entityRouteDefault.path, {
+            to={generatePath(entityRoute.path, {
               kind: entity.kind,
               optionalNamespaceAndName: [
                 entity.metadata.namespace,