core-api: added tests for FlatRoutes and fix fragment handling

Signed-off-by: Patrik Oldsberg <poldsberg@gmail.com>

.changeset/blue-insects-sin.md

@@ -0,0 +1,5 @@
+---
+'@backstage/core-api': patch
+---
+
+Fixed a bug where FlatRoutes didn't handle React Fragments properly.

packages/core-api/src/routing/FlatRoutes.test.tsx

@@ -0,0 +1,111 @@
+/*
+ * Copyright 2020 Spotify AB
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import { render, RenderResult } from '@testing-library/react';
+import React, { ReactNode } from 'react';
+import { MemoryRouter, Route, Routes, useOutlet } from 'react-router-dom';
+import { AppContext } from '../app';
+import { AppContextProvider } from '../app/AppContext';
+import { FlatRoutes } from './FlatRoutes';
+
+function makeRouteRenderer(node: ReactNode) {
+  let rendered: RenderResult | undefined = undefined;
+  return (path: string) => {
+    const content = (
+      <AppContextProvider
+        appContext={
+          ({
+            getComponents: () => ({
+              NotFoundErrorPage: () => <>Not Found</>,
+            }),
+          } as unknown) as AppContext
+        }
+      >
+        <MemoryRouter initialEntries={[path]} children={node} />
+      </AppContextProvider>
+    );
+    if (rendered) {
+      rendered.unmount();
+      rendered.rerender(content);
+    } else {
+      rendered = render(content);
+    }
+    return rendered;
+  };
+}
+
+describe('FlatRoutes', () => {
+  it('renders some routes', () => {
+    const renderRoute = makeRouteRenderer(
+      <FlatRoutes>
+        <Route path="/a" element={<>a</>} />
+        <Route path="/b" element={<>b</>} />
+      </FlatRoutes>,
+    );
+    expect(renderRoute('/a').getByText('a')).toBeInTheDocument();
+    expect(renderRoute('/b').getByText('b')).toBeInTheDocument();
+    expect(renderRoute('/c').getByText('Not Found')).toBeInTheDocument();
+    expect(renderRoute('/b').queryByText('Not Found')).not.toBeInTheDocument();
+    expect(renderRoute('/a').getByText('a')).toBeInTheDocument();
+  });
+
+  it('is not sensitive to ordering and overlapping routes', () => {
+    // The '/*' suffixes here are intentional and will be ignored by FlatRoutes
+    const routes = (
+      <>
+        <Route path="/a-1/*" element={<>a-1</>} />
+        <Route path="/a/*" element={<>a</>} />
+        <Route path="/a-2/*" element={<>a-2</>} />
+      </>
+    );
+    const renderRoute = makeRouteRenderer(<FlatRoutes>{routes}</FlatRoutes>);
+    expect(renderRoute('/a').getByText('a')).toBeInTheDocument();
+    expect(renderRoute('/a-1').getByText('a-1')).toBeInTheDocument();
+    expect(renderRoute('/a-2').getByText('a-2')).toBeInTheDocument();
+    renderRoute('').unmount();
+
+    // This uses regular Routes from react-router, not that a-2 renders a, which is the behavior we're working around
+    const renderBadRoute = makeRouteRenderer(<Routes>{routes}</Routes>);
+    expect(renderBadRoute('/a').getByText('a')).toBeInTheDocument();
+    expect(renderBadRoute('/a-1').getByText('a-1')).toBeInTheDocument();
+    expect(renderBadRoute('/a-2').getByText('a')).toBeInTheDocument();
+  });
+
+  it('renders children straight as outlets', () => {
+    const MyPage = () => {
+      return <>Outlet: {useOutlet()}</>;
+    };
+
+    // The '/*' suffixes here are intentional and will be ignored by FlatRoutes
+    const routes = (
+      <>
+        <Route path="/a" element={<MyPage />}>
+          a
+        </Route>
+        <Route path="/a/b" element={<MyPage />}>
+          a-b
+        </Route>
+        <Route path="/b" element={<MyPage />}>
+          b
+        </Route>
+      </>
+    );
+    const renderRoute = makeRouteRenderer(<FlatRoutes>{routes}</FlatRoutes>);
+    expect(renderRoute('/a').getByText('Outlet: a')).toBeInTheDocument();
+    expect(renderRoute('/a/b').getByText('Outlet: a-b')).toBeInTheDocument();
+    expect(renderRoute('/b').getByText('Outlet: b')).toBeInTheDocument();
+  });
+});

packages/core-api/src/routing/FlatRoutes.tsx

@@ -27,44 +27,38 @@ type RouteObject = {
 // Similar to the same function from react-router, this collects routes from the
 // children, but only the first level of routes
 function createRoutesFromChildren(childrenNode: ReactNode): RouteObject[] {
-  return Children.toArray(childrenNode)
-    .flatMap(child => {
-      if (!isValidElement(child)) {
-        return [];
-      }
+  return Children.toArray(childrenNode).flatMap(child => {
+    if (!isValidElement(child)) {
+      return [];
+    }
 
-      const { children } = child.props;
+    const { children } = child.props;
 
-      if (child.type === Fragment) {
-        return createRoutesFromChildren(children);
-      }
+    if (child.type === Fragment) {
+      return createRoutesFromChildren(children);
+    }
 
-      let path = child.props.path as string | undefined;
+    let path = child.props.path as string | undefined;
 
-      // TODO(Rugvip): Work around plugins registering empty paths, remove once deprecated routes are gone
-      if (path === '') {
-        return [];
-      }
-      path = path?.replace(/\/\*$/, '') ?? '/';
+    // TODO(Rugvip): Work around plugins registering empty paths, remove once deprecated routes are gone
+    if (path === '') {
+      return [];
+    }
+    path = path?.replace(/\/\*$/, '') ?? '/';
 
-      return [
-        {
-          path,
-          element: child,
-          children: children && [
-            {
-              path: '/*',
-              element: children,
-            },
-          ],
-        },
-      ];
-    })
-    .sort((a, b) => b.path.localeCompare(a.path))
-    .map(obj => {
-      obj.path = obj.path === '/' ? '/' : `${obj.path}/*`;
-      return obj;
-    });
+    return [
+      {
+        path,
+        element: child,
+        children: children && [
+          {
+            path: '/*',
+            element: children,
+          },
+        ],
+      },
+    ];
+  });
 }
 
 type FlatRoutesProps = {
@@ -74,7 +68,14 @@ type FlatRoutesProps = {
 export const FlatRoutes = (props: FlatRoutesProps): JSX.Element | null => {
   const app = useApp();
   const { NotFoundErrorPage } = app.getComponents();
-  const routes = createRoutesFromChildren(props.children);
+  const routes = createRoutesFromChildren(props.children)
+    // Routes are sorted to work around a bug where prefixes are unexpectedly matched
+    .sort((a, b) => b.path.localeCompare(a.path))
+    // We make sure all routes have '/*' appended, except '/'
+    .map(obj => {
+      obj.path = obj.path === '/' ? '/' : `${obj.path}/*`;
+      return obj;
+    });
 
   // TODO(Rugvip): Possibly add a way to skip this, like a noNotFoundPage prop
   routes.push({