From e74b07578a5b1d88510aec987db0a105399abfea Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Sun, 7 Mar 2021 12:08:15 +0100 Subject: [PATCH] core-api: added tests for FlatRoutes and fix fragment handling Signed-off-by: Patrik Oldsberg --- .changeset/blue-insects-sin.md | 5 + .../core-api/src/routing/FlatRoutes.test.tsx | 111 ++++++++++++++++++ packages/core-api/src/routing/FlatRoutes.tsx | 69 +++++------ 3 files changed, 151 insertions(+), 34 deletions(-) create mode 100644 .changeset/blue-insects-sin.md create mode 100644 packages/core-api/src/routing/FlatRoutes.test.tsx diff --git a/.changeset/blue-insects-sin.md b/.changeset/blue-insects-sin.md new file mode 100644 index 0000000000..6b601d2a8f --- /dev/null +++ b/.changeset/blue-insects-sin.md @@ -0,0 +1,5 @@ +--- +'@backstage/core-api': patch +--- + +Fixed a bug where FlatRoutes didn't handle React Fragments properly. diff --git a/packages/core-api/src/routing/FlatRoutes.test.tsx b/packages/core-api/src/routing/FlatRoutes.test.tsx new file mode 100644 index 0000000000..a9b83d1016 --- /dev/null +++ b/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 = ( + ({ + NotFoundErrorPage: () => <>Not Found, + }), + } as unknown) as AppContext + } + > + + + ); + if (rendered) { + rendered.unmount(); + rendered.rerender(content); + } else { + rendered = render(content); + } + return rendered; + }; +} + +describe('FlatRoutes', () => { + it('renders some routes', () => { + const renderRoute = makeRouteRenderer( + + a} /> + b} /> + , + ); + 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 = ( + <> + a-1} /> + a} /> + a-2} /> + + ); + const renderRoute = makeRouteRenderer({routes}); + 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}); + 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 = ( + <> + }> + a + + }> + a-b + + }> + b + + + ); + const renderRoute = makeRouteRenderer({routes}); + expect(renderRoute('/a').getByText('Outlet: a')).toBeInTheDocument(); + expect(renderRoute('/a/b').getByText('Outlet: a-b')).toBeInTheDocument(); + expect(renderRoute('/b').getByText('Outlet: b')).toBeInTheDocument(); + }); +}); diff --git a/packages/core-api/src/routing/FlatRoutes.tsx b/packages/core-api/src/routing/FlatRoutes.tsx index 3702442811..5f2cfc150d 100644 --- a/packages/core-api/src/routing/FlatRoutes.tsx +++ b/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({