From 55a5dbd547c30514fd9ef9cf59db4f072def752b Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Sat, 7 Aug 2021 13:51:36 +0200 Subject: [PATCH 1/4] core-components: fix route matching in SidebarItem Signed-off-by: Patrik Oldsberg --- .changeset/violet-bobcats-wave.md | 5 ++ .../src/layout/Sidebar/Items.tsx | 59 ++++++++++++++++++- 2 files changed, 61 insertions(+), 3 deletions(-) create mode 100644 .changeset/violet-bobcats-wave.md diff --git a/.changeset/violet-bobcats-wave.md b/.changeset/violet-bobcats-wave.md new file mode 100644 index 0000000000..339d7e0e74 --- /dev/null +++ b/.changeset/violet-bobcats-wave.md @@ -0,0 +1,5 @@ +--- +'@backstage/core-components': patch +--- + +Fix for `SidebarItem` matching the active route too broadly. diff --git a/packages/core-components/src/layout/Sidebar/Items.tsx b/packages/core-components/src/layout/Sidebar/Items.tsx index b930c455b5..1068a877bd 100644 --- a/packages/core-components/src/layout/Sidebar/Items.tsx +++ b/packages/core-components/src/layout/Sidebar/Items.tsx @@ -34,7 +34,12 @@ import React, { useContext, useState, } from 'react'; -import { NavLink, NavLinkProps } from 'react-router-dom'; +import { + Link, + NavLinkProps, + useLocation, + useResolvedPath, +} from 'react-router-dom'; import { sidebarConfig, SidebarContext } from './config'; const useStyles = makeStyles(theme => { @@ -147,6 +152,54 @@ function isButtonItem( return (props as SidebarItemLinkProps).to === undefined; } +// TODO(Rugvip): Remove this once NavLink is updated in react-router-dom. +// This is needed because react-router doesn't handle the path comparison +// properly yet, matching for example /foobar with /foo. +export const WorkaroundNavLink = React.forwardRef< + HTMLAnchorElement, + NavLinkProps +>(function WorkaroundNavLinkWithRef( + { + to, + end, + style, + className, + activeStyle, + caseSensitive, + activeClassName = 'active', + 'aria-current': ariaCurrentProp = 'page', + ...rest + }, + ref, +) { + let { pathname: locationPathname } = useLocation(); + let { pathname: toPathname } = useResolvedPath(to); + + if (!caseSensitive) { + locationPathname = locationPathname.toLowerCase(); + toPathname = toPathname.toLowerCase(); + } + + let isActive = locationPathname === toPathname; + if (!isActive && !end) { + // This is the behavior that is different from the original NavLink + isActive = locationPathname.startsWith(`${toPathname}/`); + } + + const ariaCurrent = isActive ? ariaCurrentProp : undefined; + + return ( + + ); +}); + export const SidebarItem = forwardRef((props, ref) => { const { icon: Icon, @@ -211,7 +264,7 @@ export const SidebarItem = forwardRef((props, ref) => { } return ( - ((props, ref) => { {...navLinkProps} > {content} - + ); }); From 9f8f8dd6b43e259ee0201001ee8f0c7174127907 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Sat, 7 Aug 2021 13:54:35 +0200 Subject: [PATCH 2/4] app,create-app: remove / prefixes in sidebar items Signed-off-by: Patrik Oldsberg --- .changeset/sharp-donkeys-push.md | 12 ++++++++++++ packages/app/src/components/Root/Root.tsx | 2 +- .../packages/app/src/components/Root/Root.tsx | 2 +- plugins/catalog/README.md | 2 +- 4 files changed, 15 insertions(+), 3 deletions(-) create mode 100644 .changeset/sharp-donkeys-push.md diff --git a/.changeset/sharp-donkeys-push.md b/.changeset/sharp-donkeys-push.md new file mode 100644 index 0000000000..fd6304118e --- /dev/null +++ b/.changeset/sharp-donkeys-push.md @@ -0,0 +1,12 @@ +--- +'@backstage/create-app': patch +--- + +Removed the `/` prefix in the catalog `SidebarItem` element, as it is no longer needed. + +To apply this change to an existing app, remove the `/` prefix from the catalog and any other sidebar items in `packages/app/src/components/Root/Root.ts`: + +```diff +- ++ +``` diff --git a/packages/app/src/components/Root/Root.tsx b/packages/app/src/components/Root/Root.tsx index 48141e4f53..7a666a020a 100644 --- a/packages/app/src/components/Root/Root.tsx +++ b/packages/app/src/components/Root/Root.tsx @@ -82,7 +82,7 @@ export const Root = ({ children }: PropsWithChildren<{}>) => ( {/* Global nav, not org-specific */} - + diff --git a/packages/create-app/templates/default-app/packages/app/src/components/Root/Root.tsx b/packages/create-app/templates/default-app/packages/app/src/components/Root/Root.tsx index d8665a2f9b..198e7ec2b1 100644 --- a/packages/create-app/templates/default-app/packages/app/src/components/Root/Root.tsx +++ b/packages/create-app/templates/default-app/packages/app/src/components/Root/Root.tsx @@ -77,7 +77,7 @@ export const Root = ({ children }: PropsWithChildren<{}>) => ( {/* Global nav, not org-specific */} - + diff --git a/plugins/catalog/README.md b/plugins/catalog/README.md index 8527c8da7c..ef7b0a2863 100644 --- a/plugins/catalog/README.md +++ b/plugins/catalog/README.md @@ -80,7 +80,7 @@ sidebar: export const Root = ({ children }: PropsWithChildren<{}>) => ( -+ ++ ... ``` From 3626576230b280b97141b5b6498c1723f67cab16 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Sat, 7 Aug 2021 14:07:41 +0200 Subject: [PATCH 3/4] core-app-api: add support for serving the app on a base path Signed-off-by: Patrik Oldsberg --- .changeset/late-frogs-yell.md | 5 ++++ packages/core-app-api/src/app/App.tsx | 28 +++++++++++------- .../src/routing/RouteResolver.test.ts | 29 ++++++++++++++++++- .../core-app-api/src/routing/RouteResolver.ts | 17 ++++++----- .../src/routing/RoutingProvider.test.tsx | 6 ++-- .../src/routing/RoutingProvider.tsx | 3 ++ 6 files changed, 68 insertions(+), 20 deletions(-) create mode 100644 .changeset/late-frogs-yell.md diff --git a/.changeset/late-frogs-yell.md b/.changeset/late-frogs-yell.md new file mode 100644 index 0000000000..b3e45785e3 --- /dev/null +++ b/.changeset/late-frogs-yell.md @@ -0,0 +1,5 @@ +--- +'@backstage/core-app-api': patch +--- + +Add support for serving the app with a base path other than `/`, which is enabled by including the path in `app.baseUrl`. diff --git a/packages/core-app-api/src/app/App.tsx b/packages/core-app-api/src/app/App.tsx index a95554709a..f9fe3fd26e 100644 --- a/packages/core-app-api/src/app/App.tsx +++ b/packages/core-app-api/src/app/App.tsx @@ -107,6 +107,20 @@ export function generateBoundRoutes(bindRoutes: AppOptions['bindRoutes']) { return result; } +/** + * Get the app base path from the configured app baseUrl. + * + * The returned path does not have a trailing slash. + */ +function getBasePath(configApi: Config) { + let { pathname } = new URL( + configApi.getOptionalString('app.baseUrl') ?? '/', + 'http://dummy.dev', // baseUrl can be specified as just a path + ); + pathname = pathname.replace(/\/*$/, ''); + return pathname; +} + type FullAppOptions = { apis: Iterable; icons: NonNullable; @@ -302,6 +316,7 @@ export class PrivateAppImpl implements BackstageApp { routeParents={routeParents} routeObjects={routeObjects} routeBindings={generateBoundRoutes(this.bindRoutes)} + basePath={getBasePath(loadedConfig.api)} > {children} @@ -339,14 +354,7 @@ export class PrivateAppImpl implements BackstageApp { const AppRouter = ({ children }: PropsWithChildren<{}>) => { const configApi = useApi(configApiRef); - - let { pathname } = new URL( - configApi.getOptionalString('app.baseUrl') ?? '/', - 'http://dummy.dev', // baseUrl can be specified as just a path - ); - if (pathname.endsWith('/')) { - pathname = pathname.replace(/\/$/, ''); - } + const mountPath = `${getBasePath(configApi)}/*`; // If the app hasn't configured a sign-in page, we just continue as guest. if (!SignInPageComponent) { @@ -361,7 +369,7 @@ export class PrivateAppImpl implements BackstageApp { return ( - {children}} /> + {children}} /> ); @@ -371,7 +379,7 @@ export class PrivateAppImpl implements BackstageApp { - {children}} /> + {children}} /> diff --git a/packages/core-app-api/src/routing/RouteResolver.test.ts b/packages/core-app-api/src/routing/RouteResolver.test.ts index b47a18ecce..1b7eb10003 100644 --- a/packages/core-app-api/src/routing/RouteResolver.test.ts +++ b/packages/core-app-api/src/routing/RouteResolver.test.ts @@ -65,7 +65,7 @@ const externalRef4 = createExternalRouteRef({ describe('RouteResolver', () => { it('should not resolve anything with an empty resolver', () => { - const r = new RouteResolver(new Map(), new Map(), [], new Map()); + const r = new RouteResolver(new Map(), new Map(), [], new Map(), ''); expect(r.resolve(ref1, '/')?.()).toBe(undefined); expect(r.resolve(ref2, '/')?.({ x: '1x' })).toBe(undefined); @@ -85,6 +85,7 @@ describe('RouteResolver', () => { new Map(), [{ routeRefs: new Set([ref1]), path: '/my-route', ...rest }], new Map(), + '', ); expect(r.resolve(ref1, '/')?.()).toBe('/my-route'); @@ -99,6 +100,29 @@ describe('RouteResolver', () => { expect(r.resolve(externalRef4, '/')?.({ x: '6x' })).toBe(undefined); }); + it('should resolve an absolute route and an app base path', () => { + const r = new RouteResolver( + new Map([[ref1, '/my-route']]), + new Map(), + [{ routeRefs: new Set([ref1]), path: '/my-route', ...rest }], + new Map(), + '/base', + ); + + expect(r.resolve(ref1, '/')?.()).toBe('/base/my-route'); + expect(r.resolve(ref2, '/')?.({ x: '1x' })).toBe(undefined); + expect(r.resolve(subRef1, '/')?.()).toBe('/base/my-route/foo'); + expect(r.resolve(subRef2, '/')?.({ a: '2a' })).toBe( + '/base/my-route/foo/2a', + ); + expect(r.resolve(subRef3, '/')?.({ x: '3x' })).toBe(undefined); + expect(r.resolve(subRef4, '/')?.({ x: '4x', a: '4a' })).toBe(undefined); + expect(r.resolve(externalRef1, '/')?.()).toBe(undefined); + expect(r.resolve(externalRef2, '/')?.()).toBe(undefined); + expect(r.resolve(externalRef3, '/')?.({ x: '5x' })).toBe(undefined); + expect(r.resolve(externalRef4, '/')?.({ x: '6x' })).toBe(undefined); + }); + it('should resolve an absolute route with a param and with a parent', () => { const r = new RouteResolver( new Map([ @@ -122,6 +146,7 @@ describe('RouteResolver', () => { [externalRef3, ref2], [externalRef4, subRef3], ]), + '', ); expect(r.resolve(ref1, '/')?.()).toBe('/my-route'); @@ -179,6 +204,7 @@ describe('RouteResolver', () => { }, ], new Map(), + '', ); expect(r.resolve(ref2, '/')?.({ x: 'x' })).toBe('/root/x'); @@ -232,6 +258,7 @@ describe('RouteResolver', () => { [externalRef3, ref2], [externalRef4, subRef3], ]), + '', ); const l = '/my-grandparent/my-y/my-parent/my-x'; diff --git a/packages/core-app-api/src/routing/RouteResolver.ts b/packages/core-app-api/src/routing/RouteResolver.ts index e09da28df8..b5be5f893e 100644 --- a/packages/core-app-api/src/routing/RouteResolver.ts +++ b/packages/core-app-api/src/routing/RouteResolver.ts @@ -187,6 +187,7 @@ export class RouteResolver { ExternalRouteRef, RouteRef | SubRouteRef >, + private readonly appBasePath: string, // base path without a trailing slash ) {} resolve( @@ -209,13 +210,15 @@ export class RouteResolver { // Next we figure out the base path, which is the combination of the common parent path // between our current location and our target location, as well as the additional path // that is the difference between the parent path and the base of our target location. - const basePath = resolveBasePath( - targetRef, - sourceLocation, - this.routePaths, - this.routeParents, - this.routeObjects, - ); + const basePath = + this.appBasePath + + resolveBasePath( + targetRef, + sourceLocation, + this.routePaths, + this.routeParents, + this.routeObjects, + ); const routeFunc: RouteFunc = (...[params]) => { return basePath + generatePath(targetPath, params); diff --git a/packages/core-app-api/src/routing/RoutingProvider.test.tsx b/packages/core-app-api/src/routing/RoutingProvider.test.tsx index 499b81a743..c5a070ac68 100644 --- a/packages/core-app-api/src/routing/RoutingProvider.test.tsx +++ b/packages/core-app-api/src/routing/RoutingProvider.test.tsx @@ -152,6 +152,7 @@ function withRoutingProvider( routeParents={routeParents} routeObjects={routeObjects} routeBindings={new Map(routeBindings)} + basePath="" > {root} @@ -367,6 +368,7 @@ describe('v1 consumer', () => { routeParents={new Map()} routeObjects={[]} routeBindings={new Map()} + basePath="/base" children={children} /> ), @@ -375,8 +377,8 @@ describe('v1 consumer', () => { expect(renderedHook.result.current).toBe(undefined); renderedHook.rerender({ routeRef: routeRef2 }); - expect(renderedHook.result.current?.()).toBe('/foo'); + expect(renderedHook.result.current?.()).toBe('/base/foo'); renderedHook.rerender({ routeRef: routeRef3 }); - expect(renderedHook.result.current?.({ x: 'my-x' })).toBe('/bar/my-x'); + expect(renderedHook.result.current?.({ x: 'my-x' })).toBe('/base/bar/my-x'); }); }); diff --git a/packages/core-app-api/src/routing/RoutingProvider.tsx b/packages/core-app-api/src/routing/RoutingProvider.tsx index ea8e8f76f8..2192f5df26 100644 --- a/packages/core-app-api/src/routing/RoutingProvider.tsx +++ b/packages/core-app-api/src/routing/RoutingProvider.tsx @@ -38,6 +38,7 @@ type ProviderProps = { routeParents: Map; routeObjects: BackstageRouteObject[]; routeBindings: Map; + basePath?: string; children: ReactNode; }; @@ -46,6 +47,7 @@ export const RoutingProvider = ({ routeParents, routeObjects, routeBindings, + basePath = '', children, }: ProviderProps) => { const resolver = new RouteResolver( @@ -53,6 +55,7 @@ export const RoutingProvider = ({ routeParents, routeObjects, routeBindings, + basePath, ); const versionedValue = createVersionedValueMap({ 1: resolver }); From 362ea5a7286d7db7bf5d57d21cc1c16b3ccf4a26 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Sat, 7 Aug 2021 14:12:07 +0200 Subject: [PATCH 4/4] app,create-app: fix index path redirect Signed-off-by: Patrik Oldsberg --- .changeset/neat-meals-arrive.md | 12 ++++++++++++ .../software-catalog/catalog-customization.md | 2 +- packages/app/src/App.tsx | 2 +- .../templates/default-app/packages/app/src/App.tsx | 2 +- 4 files changed, 15 insertions(+), 3 deletions(-) create mode 100644 .changeset/neat-meals-arrive.md diff --git a/.changeset/neat-meals-arrive.md b/.changeset/neat-meals-arrive.md new file mode 100644 index 0000000000..292d98de26 --- /dev/null +++ b/.changeset/neat-meals-arrive.md @@ -0,0 +1,12 @@ +--- +'@backstage/create-app': patch +--- + +Updated the index page redirect to work with apps served on a different base path than `/`. + +To apply this change to an existing app, remove the `/` prefix from the target route in the `Navigate` element in `packages/app/src/App.tsx`: + +```diff +- ++ +``` diff --git a/docs/features/software-catalog/catalog-customization.md b/docs/features/software-catalog/catalog-customization.md index 8d209cc8f0..07258a55cb 100644 --- a/docs/features/software-catalog/catalog-customization.md +++ b/docs/features/software-catalog/catalog-customization.md @@ -187,7 +187,7 @@ new `CustomCatalogIndexPage`. # packages/app/src/App.tsx const routes = ( - + - } /> + } /> ``` diff --git a/packages/app/src/App.tsx b/packages/app/src/App.tsx index de62971f96..13bdcbd2e6 100644 --- a/packages/app/src/App.tsx +++ b/packages/app/src/App.tsx @@ -98,7 +98,7 @@ const AppRouter = app.getRouter(); const routes = ( - + } /> - + } />