From 87973f5913ba282c9de97b6f2b1b168a10acbbbb Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Sat, 6 Mar 2021 23:52:02 +0100 Subject: [PATCH 01/15] core-api: add SubRouteRef type Signed-off-by: Patrik Oldsberg --- packages/core-api/src/routing/RouteRef.ts | 84 ++++++++++++++++++++++- packages/core-api/src/routing/types.ts | 15 +++- 2 files changed, 96 insertions(+), 3 deletions(-) diff --git a/packages/core-api/src/routing/RouteRef.ts b/packages/core-api/src/routing/RouteRef.ts index 24f93eeeeb..4a28301173 100644 --- a/packages/core-api/src/routing/RouteRef.ts +++ b/packages/core-api/src/routing/RouteRef.ts @@ -16,6 +16,7 @@ import { RouteRef, + SubRouteRef, ExternalRouteRef, routeRefType, AnyParams, @@ -85,12 +86,91 @@ export function createRouteRef< /** @deprecated Route refs no longer decide their own title */ title: string; }): RouteRef> { - return new RouteRefImpl>({ + return new RouteRefImpl({ ...config, params: (config.params ?? []) as ParamKeys>, }); } +export class SubRouteRefImpl + extends RouteRefBase + implements SubRouteRef { + readonly [routeRefType] = 'sub'; + + constructor( + id: string, + readonly path: string, + readonly parent: RouteRef, + readonly params: ParamKeys, + ) { + super('sub', id); + } +} + +// These utility types help us infer a Param object type from a string path +// For example, `/foo/:bar/:baz` inferred to `{ bar: string, baz: string }` +type ParamPart = S extends `:${infer Param}` ? Param : never; +type ParamNames = S extends `${infer Part}/${infer Rest}` + ? ParamPart | ParamNames + : ParamPart; +type PathParams = { [name in ParamNames]: string }; + +/** + * Merges a param object type with with an optional params type into a params object + */ +type MergeParams< + P1 extends { [param in string]: string }, + P2 extends AnyParams +> = (P1[keyof P1] extends never ? {} : P1) & (P2 extends undefined ? {} : P2); + +/** + * Creates a SubRouteRef type given the desired parameters and parent route parameters. + * The parameters types are merged together while ensuring that there is no overlap between the two. + */ +type MakeSubRouteRef< + Params extends { [param in string]: string }, + ParentParams extends AnyParams +> = keyof Params & keyof ParentParams extends never + ? SubRouteRef>> + : never; + +export function createSubRouteRef< + Path extends string, + ParentParams extends AnyParams = never +>(config: { + id: string; + path: Path; + parent: RouteRef; +}): MakeSubRouteRef, ParentParams> { + const { id, path, parent } = config; + type Params = PathParams; + + // Collect runtime parameters from the path, e.g. ['bar', 'baz'] from '/foo/:bar/:baz' + const pathParams = path.split(/:([^/]+)/).filter((_, i) => i % 2 === 1); + const params = [...parent.params, ...pathParams]; + + if (parent.params.some(p => pathParams.includes(p as string))) { + throw new Error( + 'SubRouteRef may not have params that overlap with its parent params', + ); + } + if (!path.startsWith('/')) { + throw new Error(`SubRouteRef path sub starts with '/', got '${path}'`); + } + + // We ensure that the type of the return type is sane here + const subRouteRef = new SubRouteRefImpl( + id, + path, + parent, + params as ParamKeys>, + ) as SubRouteRef>>; + + // But skip type checking of the return value itself, because the conditional + // type checking of the parent parameter overlap is tricky to express. + return subRouteRef as any; +} + export class ExternalRouteRefImpl< Params extends AnyParams, Optional extends boolean @@ -131,7 +211,7 @@ export function createExternalRouteRef< */ optional?: Optional; }): ExternalRouteRef, Optional> { - return new ExternalRouteRefImpl, Optional>( + return new ExternalRouteRefImpl( options.id, (options.params ?? []) as ParamKeys>, Boolean(options.optional) as Optional, diff --git a/packages/core-api/src/routing/types.ts b/packages/core-api/src/routing/types.ts index 69de5345a5..d91cd77bc5 100644 --- a/packages/core-api/src/routing/types.ts +++ b/packages/core-api/src/routing/types.ts @@ -41,6 +41,16 @@ export type RouteRef = { title?: string; }; +export type SubRouteRef = { + readonly [routeRefType]: 'sub'; + + parent: RouteRef; + + path: string; + + params: ParamKeys; +}; + export type ExternalRouteRef< Params extends AnyParams = any, Optional extends boolean = any @@ -52,7 +62,10 @@ export type ExternalRouteRef< optional?: Optional; }; -export type AnyRouteRef = RouteRef | ExternalRouteRef; +export type AnyRouteRef = + | RouteRef + | SubRouteRef + | ExternalRouteRef; // TODO(Rugvip): None of these should be found in the wild anymore, remove in next minor release /** @deprecated */ From a96241e788e56acedece9125d48d731f4628f3ba Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Sat, 6 Mar 2021 23:52:46 +0100 Subject: [PATCH 02/15] core-api: add is*RouteRef helpers Signed-off-by: Patrik Oldsberg --- packages/core-api/src/routing/RouteRef.ts | 30 +++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/packages/core-api/src/routing/RouteRef.ts b/packages/core-api/src/routing/RouteRef.ts index 4a28301173..6eae4f48c0 100644 --- a/packages/core-api/src/routing/RouteRef.ts +++ b/packages/core-api/src/routing/RouteRef.ts @@ -217,3 +217,33 @@ export function createExternalRouteRef< Boolean(options.optional) as Optional, ); } + +export function isRouteRef( + routeRef: + | RouteRef + | SubRouteRef + | ExternalRouteRef, +): routeRef is RouteRef { + return routeRef[routeRefType] === 'absolute'; +} + +export function isSubRouteRef( + routeRef: + | RouteRef + | SubRouteRef + | ExternalRouteRef, +): routeRef is SubRouteRef { + return routeRef[routeRefType] === 'sub'; +} + +export function isExternalRouteRef< + Params extends AnyParams, + Optional extends boolean +>( + routeRef: + | RouteRef + | SubRouteRef + | ExternalRouteRef, +): routeRef is ExternalRouteRef { + return routeRef[routeRefType] === 'external'; +} From 9bc954b9f82b73b7be4f0c956b6660a2101307f5 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Sat, 6 Mar 2021 23:53:58 +0100 Subject: [PATCH 03/15] core-api: implement SubRouteRef routing Signed-off-by: Patrik Oldsberg --- packages/core-api/src/app/App.test.tsx | 120 ++++++++++++++++-------- packages/core-api/src/app/types.ts | 3 +- packages/core-api/src/routing/hooks.tsx | 67 ++++++++++--- 3 files changed, 137 insertions(+), 53 deletions(-) diff --git a/packages/core-api/src/app/App.test.tsx b/packages/core-api/src/app/App.test.tsx index b9fdf05444..a263a452b9 100644 --- a/packages/core-api/src/app/App.test.tsx +++ b/packages/core-api/src/app/App.test.tsx @@ -23,7 +23,11 @@ import { createRoutableExtension } from '../extensions'; import { defaultSystemIcons } from '../icons'; import { createPlugin } from '../plugin'; import { useRouteRef } from '../routing/hooks'; -import { createExternalRouteRef, createRouteRef } from '../routing/RouteRef'; +import { + createExternalRouteRef, + createRouteRef, + createSubRouteRef, +} from '../routing/RouteRef'; import { generateBoundRoutes, PrivateAppImpl } from './App'; describe('generateBoundRoutes', () => { @@ -55,14 +59,37 @@ describe('Integration Test', () => { title: '', params: ['x'], }); - const err = createExternalRouteRef({ id: 'err' }); - const errParams = createExternalRouteRef({ id: 'errParams', params: ['x'] }); - const errOptional = createExternalRouteRef({ - id: 'errOptional', + const subRouteRef1 = createSubRouteRef({ + id: 'sub1', + path: '/sub1', + parent: plugin1RouteRef, + }); + const subRouteRef2 = createSubRouteRef({ + id: 'sub2', + path: '/sub2/:x', + parent: plugin1RouteRef, + }); + const subRouteRef3 = createSubRouteRef({ + id: 'sub3', + path: '/sub3', + parent: plugin2RouteRef, + }); + const subRouteRef4 = createSubRouteRef({ + id: 'sub4', + path: '/sub4/:y', + parent: plugin2RouteRef, + }); + const extRouteRef1 = createExternalRouteRef({ id: 'extRouteRef1' }); + const extRouteRef2 = createExternalRouteRef({ + id: 'extRouteRef2', + params: ['x'], + }); + const extRouteRef3 = createExternalRouteRef({ + id: 'extRouteRef3', optional: true, }); - const errParamsOptional = createExternalRouteRef({ - id: 'errParamsOptional', + const extRouteRef4 = createExternalRouteRef({ + id: 'extRouteRef4', optional: true, params: ['x'], }); @@ -70,10 +97,10 @@ describe('Integration Test', () => { const plugin1 = createPlugin({ id: 'blob', externalRoutes: { - err, - errParams, - errOptional, - errParamsOptional, + extRouteRef1, + extRouteRef2, + extRouteRef3, + extRouteRef4, }, }); @@ -92,19 +119,28 @@ describe('Integration Test', () => { createRoutableExtension({ component: () => Promise.resolve((_: PropsWithChildren<{ path?: string }>) => { - const errLink = useRouteRef(err); - const errParamsLink = useRouteRef(errParams); - const errOptionalLink = useRouteRef(errOptional); - const errParamsOptionalLink = useRouteRef(errParamsOptional); + const link1 = useRouteRef(plugin1RouteRef); + const link2 = useRouteRef(plugin2RouteRef); + const subLink1 = useRouteRef(subRouteRef1); + const subLink2 = useRouteRef(subRouteRef2); + const subLink3 = useRouteRef(subRouteRef3); + const subLink4 = useRouteRef(subRouteRef4); + const extLink1 = useRouteRef(extRouteRef1); + const extLink2 = useRouteRef(extRouteRef2); + const extLink3 = useRouteRef(extRouteRef3); + const extLink4 = useRouteRef(extRouteRef4); return (
- err: {errLink()} - errParams: {errParamsLink({ x: 'a' })} - errOptional: {errOptionalLink?.() ?? ''} - - errParamsOptional:{' '} - {errParamsOptionalLink?.({ x: 'b' }) ?? ''} - + link1: {link1()} + link2: {link2({ x: 'a' })} + subLink1: {subLink1()} + subLink2: {subLink2({ x: 'a' })} + subLink3: {subLink3({ x: 'b' })} + subLink4: {subLink4({ x: 'c', y: 'd' })} + extLink1: {extLink1()} + extLink2: {extLink2({ x: 'a' })} + extLink3: {extLink3?.() ?? ''} + extLink4: {extLink4?.({ x: 'b' }) ?? ''}
); }), @@ -136,10 +172,10 @@ describe('Integration Test', () => { components, bindRoutes: ({ bind }) => { bind(plugin1.externalRoutes, { - err: plugin1RouteRef, - errParams: plugin2RouteRef, - errOptional: plugin1RouteRef, - errParamsOptional: plugin2RouteRef, + extRouteRef1: plugin1RouteRef, + extRouteRef2: plugin2RouteRef, + extRouteRef3: subRouteRef1, + extRouteRef4: plugin2RouteRef, }); }, }); @@ -152,16 +188,22 @@ describe('Integration Test', () => { - + , ); - expect(screen.getByText('err: /')).toBeInTheDocument(); - expect(screen.getByText('errParams: /foo')).toBeInTheDocument(); - expect(screen.getByText('errOptional: /')).toBeInTheDocument(); - expect(screen.getByText('errParamsOptional: /foo')).toBeInTheDocument(); + expect(screen.getByText('link1: /')).toBeInTheDocument(); + expect(screen.getByText('link2: /foo/a')).toBeInTheDocument(); + expect(screen.getByText('subLink1: /sub1')).toBeInTheDocument(); + expect(screen.getByText('subLink2: /sub2/a')).toBeInTheDocument(); + expect(screen.getByText('subLink3: /foo/b/sub3')).toBeInTheDocument(); + expect(screen.getByText('subLink4: /foo/c/sub4/d')).toBeInTheDocument(); + expect(screen.getByText('extLink1: /')).toBeInTheDocument(); + expect(screen.getByText('extLink2: /foo/a')).toBeInTheDocument(); + expect(screen.getByText('extLink3: /sub1')).toBeInTheDocument(); + expect(screen.getByText('extLink4: /foo/b')).toBeInTheDocument(); }); it('runs happy paths without optional routes', async () => { @@ -181,8 +223,8 @@ describe('Integration Test', () => { components, bindRoutes: ({ bind }) => { bind(plugin1.externalRoutes, { - err: plugin1RouteRef, - errParams: plugin2RouteRef, + extRouteRef1: plugin1RouteRef, + extRouteRef2: plugin2RouteRef, }); }, }); @@ -201,10 +243,10 @@ describe('Integration Test', () => { , ); - expect(screen.getByText('err: /')).toBeInTheDocument(); - expect(screen.getByText('errParams: /foo')).toBeInTheDocument(); - expect(screen.getByText('errOptional: ')).toBeInTheDocument(); - expect(screen.getByText('errParamsOptional: ')).toBeInTheDocument(); + expect(screen.getByText('extLink1: /')).toBeInTheDocument(); + expect(screen.getByText('extLink2: /foo')).toBeInTheDocument(); + expect(screen.getByText('extLink3: ')).toBeInTheDocument(); + expect(screen.getByText('extLink4: ')).toBeInTheDocument(); }); it('should throw some error when the route has duplicate params', () => { @@ -224,8 +266,8 @@ describe('Integration Test', () => { components, bindRoutes: ({ bind }) => { bind(plugin1.externalRoutes, { - err: plugin1RouteRef, - errParams: plugin2RouteRef, + extRouteRef1: plugin1RouteRef, + extRouteRef2: plugin2RouteRef, }); }, }); diff --git a/packages/core-api/src/app/types.ts b/packages/core-api/src/app/types.ts index df4180c6e4..5bdf51837d 100644 --- a/packages/core-api/src/app/types.ts +++ b/packages/core-api/src/app/types.ts @@ -21,6 +21,7 @@ import { ExternalRouteRef, RouteRef } from '../routing'; import { AnyApiFactory } from '../apis'; import { AppTheme, ProfileInfo } from '../apis/definitions'; import { AppConfig } from '@backstage/config'; +import { SubRouteRef } from '../routing/types'; export type BootErrorPageProps = { step: 'load-config'; @@ -102,7 +103,7 @@ type TargetRouteMap = { infer Params, any > - ? RouteRef + ? RouteRef | SubRouteRef : never; }; diff --git a/packages/core-api/src/routing/hooks.tsx b/packages/core-api/src/routing/hooks.tsx index 4d419485f6..e37bb32f8b 100644 --- a/packages/core-api/src/routing/hooks.tsx +++ b/packages/core-api/src/routing/hooks.tsx @@ -15,14 +15,17 @@ */ import React, { createContext, ReactNode, useContext, useMemo } from 'react'; +import { generatePath, matchRoutes, useLocation } from 'react-router-dom'; import { AnyRouteRef, BackstageRouteObject, RouteRef, ExternalRouteRef, AnyParams, + SubRouteRef, + routeRefType, } from './types'; -import { generatePath, matchRoutes, useLocation } from 'react-router-dom'; +import { isRouteRef, isSubRouteRef, isExternalRouteRef } from './RouteRef'; // The extra TS magic here is to require a single params argument if the RouteRef // had at least one param defined, but require 0 arguments if there are no params defined. @@ -38,29 +41,64 @@ class RouteResolver { private readonly routePaths: Map, private readonly routeParents: Map, private readonly routeObjects: BackstageRouteObject[], - private readonly routeBindings: Map, + private readonly routeBindings: Map< + ExternalRouteRef, + RouteRef | SubRouteRef + >, ) {} resolve( - routeRefOrExternalRouteRef: RouteRef | ExternalRouteRef, + anyRouteRef: + | RouteRef + | SubRouteRef + | ExternalRouteRef, sourceLocation: ReturnType, ): RouteFunc | undefined { - const routeRef = - this.routeBindings.get(routeRefOrExternalRouteRef) ?? - (routeRefOrExternalRouteRef as RouteRef); + let resolvedRef: AnyRouteRef; + let subRoutePath = ''; + if (isRouteRef(anyRouteRef)) { + resolvedRef = anyRouteRef; + } else if (isSubRouteRef(anyRouteRef)) { + resolvedRef = anyRouteRef.parent; + subRoutePath = anyRouteRef.path; + } else if (isExternalRouteRef(anyRouteRef)) { + const resolvedRoute = this.routeBindings.get(anyRouteRef); + if (!resolvedRoute) { + return undefined; + } + if (isSubRouteRef(resolvedRoute)) { + subRoutePath = resolvedRoute.path; + resolvedRef = resolvedRoute.parent; + } else { + resolvedRef = resolvedRoute; + } + } else if (anyRouteRef[routeRefType]) { + throw new Error( + `Unknown or invalid route ref type, ${anyRouteRef[routeRefType]}`, + ); + } else { + throw new Error( + `Unknown object passed to useRouteRef, got ${anyRouteRef}`, + ); + } const match = matchRoutes(this.routeObjects, sourceLocation) ?? []; // If our route isn't bound to a path we fail the resolution and let the caller decide the failure mode - const lastPath = this.routePaths.get(routeRef); - if (!lastPath) { + const resolvedPath = this.routePaths.get(resolvedRef); + if (!resolvedPath) { return undefined; } + // SubRouteRefs join the path from the parent route with its own path + const lastPath = + resolvedPath + + (resolvedPath.endsWith('/') ? subRoutePath.slice(1) : subRoutePath); + const targetRefStack = Array(); let matchIndex = -1; for ( - let currentRouteRef: AnyRouteRef | undefined = routeRef; + let currentRouteRef: AnyRouteRef | undefined = resolvedRef; currentRouteRef; currentRouteRef = this.routeParents.get(currentRouteRef) ) { @@ -98,7 +136,7 @@ class RouteResolver { } if (path.includes(':')) { throw new Error( - `Cannot route to ${routeRef} with parent ${ref} as it has parameters`, + `Cannot route to ${resolvedRef} with parent ${ref} as it has parameters`, ); } return path; @@ -119,10 +157,13 @@ export function useRouteRef( routeRef: ExternalRouteRef, ): Optional extends true ? RouteFunc | undefined : RouteFunc; export function useRouteRef( - routeRef: RouteRef, + routeRef: RouteRef | SubRouteRef, ): RouteFunc; export function useRouteRef( - routeRef: RouteRef | ExternalRouteRef, + routeRef: + | RouteRef + | SubRouteRef + | ExternalRouteRef, ): RouteFunc | undefined { const sourceLocation = useLocation(); const resolver = useContext(RoutingContext); @@ -147,7 +188,7 @@ type ProviderProps = { routePaths: Map; routeParents: Map; routeObjects: BackstageRouteObject[]; - routeBindings: Map; + routeBindings: Map; children: ReactNode; }; From d1bb014a787296e458b74f2ff62d4b0e65d0da43 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Sun, 7 Mar 2021 00:02:06 +0100 Subject: [PATCH 04/15] core-api: split route ref types into a separate modules Signed-off-by: Patrik Oldsberg --- packages/core-api/src/app/App.test.tsx | 2 +- .../core-api/src/routing/ExternalRouteRef.ts | 84 +++++++++ packages/core-api/src/routing/RouteRef.ts | 167 +----------------- packages/core-api/src/routing/SubRouteRef.ts | 114 ++++++++++++ packages/core-api/src/routing/hooks.test.tsx | 7 +- packages/core-api/src/routing/hooks.tsx | 4 +- packages/core-api/src/routing/index.ts | 4 +- packages/core-api/src/routing/types.ts | 3 + 8 files changed, 216 insertions(+), 169 deletions(-) create mode 100644 packages/core-api/src/routing/ExternalRouteRef.ts create mode 100644 packages/core-api/src/routing/SubRouteRef.ts diff --git a/packages/core-api/src/app/App.test.tsx b/packages/core-api/src/app/App.test.tsx index a263a452b9..e474a8b851 100644 --- a/packages/core-api/src/app/App.test.tsx +++ b/packages/core-api/src/app/App.test.tsx @@ -27,7 +27,7 @@ import { createExternalRouteRef, createRouteRef, createSubRouteRef, -} from '../routing/RouteRef'; +} from '../routing'; import { generateBoundRoutes, PrivateAppImpl } from './App'; describe('generateBoundRoutes', () => { diff --git a/packages/core-api/src/routing/ExternalRouteRef.ts b/packages/core-api/src/routing/ExternalRouteRef.ts new file mode 100644 index 0000000000..e6af9cf8a7 --- /dev/null +++ b/packages/core-api/src/routing/ExternalRouteRef.ts @@ -0,0 +1,84 @@ +/* + * 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 { + RouteRef, + SubRouteRef, + ExternalRouteRef, + routeRefType, + AnyParams, + ParamKeys, + OptionalParams, +} from './types'; + +export class ExternalRouteRefImpl< + Params extends AnyParams, + Optional extends boolean +> implements ExternalRouteRef { + readonly [routeRefType] = 'external'; + + constructor( + private readonly id: string, + readonly params: ParamKeys, + readonly optional: Optional, + ) {} + + toString() { + return `routeRef{type=external,id=${this.id}}`; + } +} + +export function createExternalRouteRef< + Params extends { [param in ParamKey]: string }, + Optional extends boolean = false, + ParamKey extends string = never +>(options: { + /** + * An identifier for this route, used to identify it in error messages + */ + id: string; + + /** + * The parameters that will be provided to the external route reference. + */ + params?: ParamKey[]; + + /** + * Whether or not this route is optional, defaults to false. + * + * Optional external routes are not required to be bound in the app, and + * if they aren't, `useRouteRef` will return `undefined`. + */ + optional?: Optional; +}): ExternalRouteRef, Optional> { + return new ExternalRouteRefImpl( + options.id, + (options.params ?? []) as ParamKeys>, + Boolean(options.optional) as Optional, + ); +} + +export function isExternalRouteRef< + Params extends AnyParams, + Optional extends boolean +>( + routeRef: + | RouteRef + | SubRouteRef + | ExternalRouteRef, +): routeRef is ExternalRouteRef { + return routeRef[routeRefType] === 'external'; +} diff --git a/packages/core-api/src/routing/RouteRef.ts b/packages/core-api/src/routing/RouteRef.ts index 6eae4f48c0..3f14d2f49b 100644 --- a/packages/core-api/src/routing/RouteRef.ts +++ b/packages/core-api/src/routing/RouteRef.ts @@ -21,6 +21,7 @@ import { routeRefType, AnyParams, ParamKeys, + OptionalParams, } from './types'; import { IconComponent } from '../icons'; @@ -32,20 +33,11 @@ export type RouteRefConfig = { title: string; }; -class RouteRefBase { - constructor(type: string, id: string) { - this.toString = () => `routeRef{type=${type},id=${id}}`; - } -} - export class RouteRefImpl - extends RouteRefBase implements RouteRef { readonly [routeRefType] = 'absolute'; - constructor(private readonly config: RouteRefConfig) { - super('absolute', config.title); - } + constructor(private readonly config: RouteRefConfig) {} get params(): ParamKeys { return this.config.params as any; @@ -63,11 +55,11 @@ export class RouteRefImpl get title() { return this.config.title; } -} -type OptionalParams< - Params extends { [param in string]: string } -> = Params[keyof Params] extends never ? undefined : Params; + toString() { + return `routeRef{type=absolute,id=${this.config.title}}`; + } +} export function createRouteRef< // Params is the type that we care about and the one to be embedded in the route ref. @@ -92,132 +84,6 @@ export function createRouteRef< }); } -export class SubRouteRefImpl - extends RouteRefBase - implements SubRouteRef { - readonly [routeRefType] = 'sub'; - - constructor( - id: string, - readonly path: string, - readonly parent: RouteRef, - readonly params: ParamKeys, - ) { - super('sub', id); - } -} - -// These utility types help us infer a Param object type from a string path -// For example, `/foo/:bar/:baz` inferred to `{ bar: string, baz: string }` -type ParamPart = S extends `:${infer Param}` ? Param : never; -type ParamNames = S extends `${infer Part}/${infer Rest}` - ? ParamPart | ParamNames - : ParamPart; -type PathParams = { [name in ParamNames]: string }; - -/** - * Merges a param object type with with an optional params type into a params object - */ -type MergeParams< - P1 extends { [param in string]: string }, - P2 extends AnyParams -> = (P1[keyof P1] extends never ? {} : P1) & (P2 extends undefined ? {} : P2); - -/** - * Creates a SubRouteRef type given the desired parameters and parent route parameters. - * The parameters types are merged together while ensuring that there is no overlap between the two. - */ -type MakeSubRouteRef< - Params extends { [param in string]: string }, - ParentParams extends AnyParams -> = keyof Params & keyof ParentParams extends never - ? SubRouteRef>> - : never; - -export function createSubRouteRef< - Path extends string, - ParentParams extends AnyParams = never ->(config: { - id: string; - path: Path; - parent: RouteRef; -}): MakeSubRouteRef, ParentParams> { - const { id, path, parent } = config; - type Params = PathParams; - - // Collect runtime parameters from the path, e.g. ['bar', 'baz'] from '/foo/:bar/:baz' - const pathParams = path.split(/:([^/]+)/).filter((_, i) => i % 2 === 1); - const params = [...parent.params, ...pathParams]; - - if (parent.params.some(p => pathParams.includes(p as string))) { - throw new Error( - 'SubRouteRef may not have params that overlap with its parent params', - ); - } - if (!path.startsWith('/')) { - throw new Error(`SubRouteRef path sub starts with '/', got '${path}'`); - } - - // We ensure that the type of the return type is sane here - const subRouteRef = new SubRouteRefImpl( - id, - path, - parent, - params as ParamKeys>, - ) as SubRouteRef>>; - - // But skip type checking of the return value itself, because the conditional - // type checking of the parent parameter overlap is tricky to express. - return subRouteRef as any; -} - -export class ExternalRouteRefImpl< - Params extends AnyParams, - Optional extends boolean - > - extends RouteRefBase - implements ExternalRouteRef { - readonly [routeRefType] = 'external'; - - constructor( - id: string, - readonly params: ParamKeys, - readonly optional: Optional, - ) { - super('external', id); - } -} - -export function createExternalRouteRef< - Params extends { [param in ParamKey]: string }, - Optional extends boolean = false, - ParamKey extends string = never ->(options: { - /** - * An identifier for this route, used to identify it in error messages - */ - id: string; - - /** - * The parameters that will be provided to the external route reference. - */ - params?: ParamKey[]; - - /** - * Whether or not this route is optional, defaults to false. - * - * Optional external routes are not required to be bound in the app, and - * if they aren't, `useRouteRef` will return `undefined`. - */ - optional?: Optional; -}): ExternalRouteRef, Optional> { - return new ExternalRouteRefImpl( - options.id, - (options.params ?? []) as ParamKeys>, - Boolean(options.optional) as Optional, - ); -} - export function isRouteRef( routeRef: | RouteRef @@ -226,24 +92,3 @@ export function isRouteRef( ): routeRef is RouteRef { return routeRef[routeRefType] === 'absolute'; } - -export function isSubRouteRef( - routeRef: - | RouteRef - | SubRouteRef - | ExternalRouteRef, -): routeRef is SubRouteRef { - return routeRef[routeRefType] === 'sub'; -} - -export function isExternalRouteRef< - Params extends AnyParams, - Optional extends boolean ->( - routeRef: - | RouteRef - | SubRouteRef - | ExternalRouteRef, -): routeRef is ExternalRouteRef { - return routeRef[routeRefType] === 'external'; -} diff --git a/packages/core-api/src/routing/SubRouteRef.ts b/packages/core-api/src/routing/SubRouteRef.ts new file mode 100644 index 0000000000..8ff23c0c5e --- /dev/null +++ b/packages/core-api/src/routing/SubRouteRef.ts @@ -0,0 +1,114 @@ +/* + * 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 { + AnyParams, + ExternalRouteRef, + OptionalParams, + ParamKeys, + RouteRef, + routeRefType, + SubRouteRef, +} from './types'; + +export class SubRouteRefImpl + implements SubRouteRef { + readonly [routeRefType] = 'sub'; + + constructor( + private readonly id: string, + readonly path: string, + readonly parent: RouteRef, + readonly params: ParamKeys, + ) {} + + toString() { + return `routeRef{type=sub,id=${this.id}}`; + } +} + +// These utility types help us infer a Param object type from a string path +// For example, `/foo/:bar/:baz` inferred to `{ bar: string, baz: string }` +type ParamPart = S extends `:${infer Param}` ? Param : never; +type ParamNames = S extends `${infer Part}/${infer Rest}` + ? ParamPart | ParamNames + : ParamPart; +type PathParams = { [name in ParamNames]: string }; + +/** + * Merges a param object type with with an optional params type into a params object + */ +type MergeParams< + P1 extends { [param in string]: string }, + P2 extends AnyParams +> = (P1[keyof P1] extends never ? {} : P1) & (P2 extends undefined ? {} : P2); + +/** + * Creates a SubRouteRef type given the desired parameters and parent route parameters. + * The parameters types are merged together while ensuring that there is no overlap between the two. + */ +type MakeSubRouteRef< + Params extends { [param in string]: string }, + ParentParams extends AnyParams +> = keyof Params & keyof ParentParams extends never + ? SubRouteRef>> + : never; + +export function createSubRouteRef< + Path extends string, + ParentParams extends AnyParams = never +>(config: { + id: string; + path: Path; + parent: RouteRef; +}): MakeSubRouteRef, ParentParams> { + const { id, path, parent } = config; + type Params = PathParams; + + // Collect runtime parameters from the path, e.g. ['bar', 'baz'] from '/foo/:bar/:baz' + const pathParams = path.split(/:([^/]+)/).filter((_, i) => i % 2 === 1); + const params = [...parent.params, ...pathParams]; + + if (parent.params.some(p => pathParams.includes(p as string))) { + throw new Error( + 'SubRouteRef may not have params that overlap with its parent params', + ); + } + if (!path.startsWith('/')) { + throw new Error(`SubRouteRef path sub starts with '/', got '${path}'`); + } + + // We ensure that the type of the return type is sane here + const subRouteRef = new SubRouteRefImpl( + id, + path, + parent, + params as ParamKeys>, + ) as SubRouteRef>>; + + // But skip type checking of the return value itself, because the conditional + // type checking of the parent parameter overlap is tricky to express. + return subRouteRef as any; +} + +export function isSubRouteRef( + routeRef: + | RouteRef + | SubRouteRef + | ExternalRouteRef, +): routeRef is SubRouteRef { + return routeRef[routeRefType] === 'sub'; +} diff --git a/packages/core-api/src/routing/hooks.test.tsx b/packages/core-api/src/routing/hooks.test.tsx index 0ada37b18d..e66b76b130 100644 --- a/packages/core-api/src/routing/hooks.test.tsx +++ b/packages/core-api/src/routing/hooks.test.tsx @@ -35,11 +35,8 @@ import { validateRoutes, RouteFunc, } from './hooks'; -import { - createRouteRef, - createExternalRouteRef, - RouteRefConfig, -} from './RouteRef'; +import { createRouteRef, RouteRefConfig } from './RouteRef'; +import { createExternalRouteRef } from './ExternalRouteRef'; import { AnyRouteRef, RouteRef, ExternalRouteRef } from './types'; const mockConfig = (extra?: Partial>) => ({ diff --git a/packages/core-api/src/routing/hooks.tsx b/packages/core-api/src/routing/hooks.tsx index e37bb32f8b..2e305d8c38 100644 --- a/packages/core-api/src/routing/hooks.tsx +++ b/packages/core-api/src/routing/hooks.tsx @@ -25,7 +25,9 @@ import { SubRouteRef, routeRefType, } from './types'; -import { isRouteRef, isSubRouteRef, isExternalRouteRef } from './RouteRef'; +import { isRouteRef } from './RouteRef'; +import { isSubRouteRef } from './SubRouteRef'; +import { isExternalRouteRef } from './ExternalRouteRef'; // The extra TS magic here is to require a single params argument if the RouteRef // had at least one param defined, but require 0 arguments if there are no params defined. diff --git a/packages/core-api/src/routing/index.ts b/packages/core-api/src/routing/index.ts index 2a0685f583..c6134cfe4f 100644 --- a/packages/core-api/src/routing/index.ts +++ b/packages/core-api/src/routing/index.ts @@ -22,6 +22,8 @@ export type { ExternalRouteRef, } from './types'; export { FlatRoutes } from './FlatRoutes'; -export { createRouteRef, createExternalRouteRef } from './RouteRef'; +export { createRouteRef } from './RouteRef'; +export { createSubRouteRef } from './SubRouteRef'; +export { createExternalRouteRef } from './ExternalRouteRef'; export type { RouteRefConfig } from './RouteRef'; export { useRouteRef } from './hooks'; diff --git a/packages/core-api/src/routing/types.ts b/packages/core-api/src/routing/types.ts index d91cd77bc5..80b1f0c4e7 100644 --- a/packages/core-api/src/routing/types.ts +++ b/packages/core-api/src/routing/types.ts @@ -21,6 +21,9 @@ export type AnyParams = { [param in string]: string } | undefined; export type ParamKeys = keyof Params extends never ? [] : (keyof Params)[]; +export type OptionalParams< + Params extends { [param in string]: string } +> = Params[keyof Params] extends never ? undefined : Params; export const routeRefType: unique symbol = getGlobalSingleton( 'route-ref-type', From 4406fe84954ccd3820677844969f9cbf72f2dda7 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Sun, 7 Mar 2021 11:26:04 +0100 Subject: [PATCH 05/15] core-api: split up routing hooks module Signed-off-by: Patrik Oldsberg --- packages/core-api/src/app/App.tsx | 3 +- .../core-api/src/routing/RouteResolver.ts | 145 +++++++++++++++ packages/core-api/src/routing/hooks.test.tsx | 10 +- packages/core-api/src/routing/hooks.tsx | 171 +----------------- packages/core-api/src/routing/types.ts | 9 + packages/core-api/src/routing/validation.ts | 56 ++++++ 6 files changed, 218 insertions(+), 176 deletions(-) create mode 100644 packages/core-api/src/routing/RouteResolver.ts create mode 100644 packages/core-api/src/routing/validation.ts diff --git a/packages/core-api/src/app/App.tsx b/packages/core-api/src/app/App.tsx index 3c58930709..9e14c9a5fb 100644 --- a/packages/core-api/src/app/App.tsx +++ b/packages/core-api/src/app/App.tsx @@ -57,7 +57,8 @@ import { routeParentCollector, routePathCollector, } from '../routing/collectors'; -import { RoutingProvider, validateRoutes } from '../routing/hooks'; +import { RoutingProvider } from '../routing/hooks'; +import { validateRoutes } from '../routing/validation'; import { AppContextProvider } from './AppContext'; import { AppIdentity } from './AppIdentity'; import { AppThemeProvider } from './AppThemeProvider'; diff --git a/packages/core-api/src/routing/RouteResolver.ts b/packages/core-api/src/routing/RouteResolver.ts new file mode 100644 index 0000000000..511d6bce2e --- /dev/null +++ b/packages/core-api/src/routing/RouteResolver.ts @@ -0,0 +1,145 @@ +/* + * 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 { generatePath, matchRoutes, useLocation } from 'react-router-dom'; +import { + AnyRouteRef, + BackstageRouteObject, + RouteRef, + ExternalRouteRef, + AnyParams, + SubRouteRef, + routeRefType, + RouteFunc, +} from './types'; +import { isRouteRef } from './RouteRef'; +import { isSubRouteRef } from './SubRouteRef'; +import { isExternalRouteRef } from './ExternalRouteRef'; + +export class RouteResolver { + constructor( + private readonly routePaths: Map, + private readonly routeParents: Map, + private readonly routeObjects: BackstageRouteObject[], + private readonly routeBindings: Map< + ExternalRouteRef, + RouteRef | SubRouteRef + >, + ) {} + + resolve( + anyRouteRef: + | RouteRef + | SubRouteRef + | ExternalRouteRef, + sourceLocation: ReturnType, + ): RouteFunc | undefined { + let resolvedRef: AnyRouteRef; + let subRoutePath = ''; + if (isRouteRef(anyRouteRef)) { + resolvedRef = anyRouteRef; + } else if (isSubRouteRef(anyRouteRef)) { + resolvedRef = anyRouteRef.parent; + subRoutePath = anyRouteRef.path; + } else if (isExternalRouteRef(anyRouteRef)) { + const resolvedRoute = this.routeBindings.get(anyRouteRef); + if (!resolvedRoute) { + return undefined; + } + if (isSubRouteRef(resolvedRoute)) { + subRoutePath = resolvedRoute.path; + resolvedRef = resolvedRoute.parent; + } else { + resolvedRef = resolvedRoute; + } + } else if (anyRouteRef[routeRefType]) { + throw new Error( + `Unknown or invalid route ref type, ${anyRouteRef[routeRefType]}`, + ); + } else { + throw new Error( + `Unknown object passed to useRouteRef, got ${anyRouteRef}`, + ); + } + + const match = matchRoutes(this.routeObjects, sourceLocation) ?? []; + + // If our route isn't bound to a path we fail the resolution and let the caller decide the failure mode + const resolvedPath = this.routePaths.get(resolvedRef); + if (!resolvedPath) { + return undefined; + } + // SubRouteRefs join the path from the parent route with its own path + const lastPath = + resolvedPath + + (resolvedPath.endsWith('/') ? subRoutePath.slice(1) : subRoutePath); + + const targetRefStack = Array(); + let matchIndex = -1; + + for ( + let currentRouteRef: AnyRouteRef | undefined = resolvedRef; + currentRouteRef; + currentRouteRef = this.routeParents.get(currentRouteRef) + ) { + matchIndex = match.findIndex(m => + (m.route as BackstageRouteObject).routeRefs.has(currentRouteRef!), + ); + if (matchIndex !== -1) { + break; + } + + targetRefStack.unshift(currentRouteRef); + } + + // If our target route is present in the initial match we need to construct the final path + // from the parent of the matched route segment. That's to allow the caller of the route + // function to supply their own params. + if (targetRefStack.length === 0) { + matchIndex -= 1; + } + + // This is the part of the route tree that the target and source locations have in common. + // We re-use the existing pathname directly along with all params. + const parentPath = matchIndex === -1 ? '' : match[matchIndex].pathname; + + // This constructs the mid section of the path using paths resolved from all route refs + // we need to traverse to reach our target except for the very last one. None of these + // paths are allowed to require any parameters, as the called would have no way of knowing + // what parameters those are. + const prefixPath = targetRefStack + .slice(0, -1) + .map(ref => { + const path = this.routePaths.get(ref); + if (!path) { + throw new Error(`No path for ${ref}`); + } + if (path.includes(':')) { + throw new Error( + `Cannot route to ${resolvedRef} with parent ${ref} as it has parameters`, + ); + } + return path; + }) + .join('/') + .replace(/\/\/+/g, '/'); // Normalize path to not contain repeated /'s + + const routeFunc: RouteFunc = (...[params]) => { + return `${parentPath}${prefixPath}${generatePath(lastPath, params)}`; + }; + return routeFunc; + } +} diff --git a/packages/core-api/src/routing/hooks.test.tsx b/packages/core-api/src/routing/hooks.test.tsx index e66b76b130..9a65286fd6 100644 --- a/packages/core-api/src/routing/hooks.test.tsx +++ b/packages/core-api/src/routing/hooks.test.tsx @@ -29,15 +29,11 @@ import { routeParentCollector, routeObjectCollector, } from './collectors'; -import { - useRouteRef, - RoutingProvider, - validateRoutes, - RouteFunc, -} from './hooks'; +import { validateRoutes } from './validation'; +import { useRouteRef, RoutingProvider } from './hooks'; import { createRouteRef, RouteRefConfig } from './RouteRef'; import { createExternalRouteRef } from './ExternalRouteRef'; -import { AnyRouteRef, RouteRef, ExternalRouteRef } from './types'; +import { AnyRouteRef, RouteFunc, RouteRef, ExternalRouteRef } from './types'; const mockConfig = (extra?: Partial>) => ({ path: '/unused', diff --git a/packages/core-api/src/routing/hooks.tsx b/packages/core-api/src/routing/hooks.tsx index 2e305d8c38..bc43ff8685 100644 --- a/packages/core-api/src/routing/hooks.tsx +++ b/packages/core-api/src/routing/hooks.tsx @@ -15,7 +15,7 @@ */ import React, { createContext, ReactNode, useContext, useMemo } from 'react'; -import { generatePath, matchRoutes, useLocation } from 'react-router-dom'; +import { useLocation } from 'react-router-dom'; import { AnyRouteRef, BackstageRouteObject, @@ -23,135 +23,9 @@ import { ExternalRouteRef, AnyParams, SubRouteRef, - routeRefType, + RouteFunc, } from './types'; -import { isRouteRef } from './RouteRef'; -import { isSubRouteRef } from './SubRouteRef'; -import { isExternalRouteRef } from './ExternalRouteRef'; - -// The extra TS magic here is to require a single params argument if the RouteRef -// had at least one param defined, but require 0 arguments if there are no params defined. -// Without this we'd have to pass in empty object to all parameter-less RouteRefs -// just to make TypeScript happy, or we would have to make the argument optional in -// which case you might forget to pass it in when it is actually required. -export type RouteFunc = ( - ...[params]: Params extends undefined ? readonly [] : readonly [Params] -) => string; - -class RouteResolver { - constructor( - private readonly routePaths: Map, - private readonly routeParents: Map, - private readonly routeObjects: BackstageRouteObject[], - private readonly routeBindings: Map< - ExternalRouteRef, - RouteRef | SubRouteRef - >, - ) {} - - resolve( - anyRouteRef: - | RouteRef - | SubRouteRef - | ExternalRouteRef, - sourceLocation: ReturnType, - ): RouteFunc | undefined { - let resolvedRef: AnyRouteRef; - let subRoutePath = ''; - if (isRouteRef(anyRouteRef)) { - resolvedRef = anyRouteRef; - } else if (isSubRouteRef(anyRouteRef)) { - resolvedRef = anyRouteRef.parent; - subRoutePath = anyRouteRef.path; - } else if (isExternalRouteRef(anyRouteRef)) { - const resolvedRoute = this.routeBindings.get(anyRouteRef); - if (!resolvedRoute) { - return undefined; - } - if (isSubRouteRef(resolvedRoute)) { - subRoutePath = resolvedRoute.path; - resolvedRef = resolvedRoute.parent; - } else { - resolvedRef = resolvedRoute; - } - } else if (anyRouteRef[routeRefType]) { - throw new Error( - `Unknown or invalid route ref type, ${anyRouteRef[routeRefType]}`, - ); - } else { - throw new Error( - `Unknown object passed to useRouteRef, got ${anyRouteRef}`, - ); - } - - const match = matchRoutes(this.routeObjects, sourceLocation) ?? []; - - // If our route isn't bound to a path we fail the resolution and let the caller decide the failure mode - const resolvedPath = this.routePaths.get(resolvedRef); - if (!resolvedPath) { - return undefined; - } - // SubRouteRefs join the path from the parent route with its own path - const lastPath = - resolvedPath + - (resolvedPath.endsWith('/') ? subRoutePath.slice(1) : subRoutePath); - - const targetRefStack = Array(); - let matchIndex = -1; - - for ( - let currentRouteRef: AnyRouteRef | undefined = resolvedRef; - currentRouteRef; - currentRouteRef = this.routeParents.get(currentRouteRef) - ) { - matchIndex = match.findIndex(m => - (m.route as BackstageRouteObject).routeRefs.has(currentRouteRef!), - ); - if (matchIndex !== -1) { - break; - } - - targetRefStack.unshift(currentRouteRef); - } - - // If our target route is present in the initial match we need to construct the final path - // from the parent of the matched route segment. That's to allow the caller of the route - // function to supply their own params. - if (targetRefStack.length === 0) { - matchIndex -= 1; - } - - // This is the part of the route tree that the target and source locations have in common. - // We re-use the existing pathname directly along with all params. - const parentPath = matchIndex === -1 ? '' : match[matchIndex].pathname; - - // This constructs the mid section of the path using paths resolved from all route refs - // we need to traverse to reach our target except for the very last one. None of these - // paths are allowed to require any parameters, as the called would have no way of knowing - // what parameters those are. - const prefixPath = targetRefStack - .slice(0, -1) - .map(ref => { - const path = this.routePaths.get(ref); - if (!path) { - throw new Error(`No path for ${ref}`); - } - if (path.includes(':')) { - throw new Error( - `Cannot route to ${resolvedRef} with parent ${ref} as it has parameters`, - ); - } - return path; - }) - .join('/') - .replace(/\/\/+/g, '/'); // Normalize path to not contain repeated /'s - - const routeFunc: RouteFunc = (...[params]) => { - return `${parentPath}${prefixPath}${generatePath(lastPath, params)}`; - }; - return routeFunc; - } -} +import { RouteResolver } from './RouteResolver'; const RoutingContext = createContext(undefined); @@ -213,42 +87,3 @@ export const RoutingProvider = ({ ); }; - -export function validateRoutes( - routePaths: Map, - routeParents: Map, -) { - const notLeafRoutes = new Set(routeParents.values()); - notLeafRoutes.delete(undefined); - - for (const route of routeParents.keys()) { - if (notLeafRoutes.has(route)) { - continue; - } - - let currentRouteRef: AnyRouteRef | undefined = route; - - let fullPath = ''; - while (currentRouteRef) { - const path = routePaths.get(currentRouteRef); - if (!path) { - throw new Error(`No path for ${currentRouteRef}`); - } - fullPath = `${path}${fullPath}`; - currentRouteRef = routeParents.get(currentRouteRef); - } - - const params = fullPath.match(/:(\w+)/g); - if (params) { - for (let j = 0; j < params.length; j++) { - for (let i = j + 1; i < params.length; i++) { - if (params[i] === params[j]) { - throw new Error( - `Parameter ${params[i]} is duplicated in path ${fullPath}`, - ); - } - } - } - } - } -} diff --git a/packages/core-api/src/routing/types.ts b/packages/core-api/src/routing/types.ts index 80b1f0c4e7..65c12ff954 100644 --- a/packages/core-api/src/routing/types.ts +++ b/packages/core-api/src/routing/types.ts @@ -25,6 +25,15 @@ export type OptionalParams< Params extends { [param in string]: string } > = Params[keyof Params] extends never ? undefined : Params; +// The extra TS magic here is to require a single params argument if the RouteRef +// had at least one param defined, but require 0 arguments if there are no params defined. +// Without this we'd have to pass in empty object to all parameter-less RouteRefs +// just to make TypeScript happy, or we would have to make the argument optional in +// which case you might forget to pass it in when it is actually required. +export type RouteFunc = ( + ...[params]: Params extends undefined ? readonly [] : readonly [Params] +) => string; + export const routeRefType: unique symbol = getGlobalSingleton( 'route-ref-type', () => Symbol('route-ref-type'), diff --git a/packages/core-api/src/routing/validation.ts b/packages/core-api/src/routing/validation.ts new file mode 100644 index 0000000000..2d32471a14 --- /dev/null +++ b/packages/core-api/src/routing/validation.ts @@ -0,0 +1,56 @@ +/* + * 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 { AnyRouteRef } from './types'; + +export function validateRoutes( + routePaths: Map, + routeParents: Map, +) { + const notLeafRoutes = new Set(routeParents.values()); + notLeafRoutes.delete(undefined); + + for (const route of routeParents.keys()) { + if (notLeafRoutes.has(route)) { + continue; + } + + let currentRouteRef: AnyRouteRef | undefined = route; + + let fullPath = ''; + while (currentRouteRef) { + const path = routePaths.get(currentRouteRef); + if (!path) { + throw new Error(`No path for ${currentRouteRef}`); + } + fullPath = `${path}${fullPath}`; + currentRouteRef = routeParents.get(currentRouteRef); + } + + const params = fullPath.match(/:(\w+)/g); + if (params) { + for (let j = 0; j < params.length; j++) { + for (let i = j + 1; i < params.length; i++) { + if (params[i] === params[j]) { + throw new Error( + `Parameter ${params[i]} is duplicated in path ${fullPath}`, + ); + } + } + } + } + } +} From e74b07578a5b1d88510aec987db0a105399abfea Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Sun, 7 Mar 2021 12:08:15 +0100 Subject: [PATCH 06/15] 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({ From 13524b80b2d9ccfcea306a4a9b42fc14d84a284f Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Sun, 7 Mar 2021 13:18:41 +0100 Subject: [PATCH 07/15] core-api: deprecate RouteRef title config and introduce id + add tests Signed-off-by: Patrik Oldsberg --- .changeset/beige-lemons-accept.md | 5 + packages/core-api/src/app/App.test.tsx | 12 +-- .../core-api/src/routing/RouteRef.test.ts | 94 +++++++++++++++++++ packages/core-api/src/routing/RouteRef.ts | 38 +++++--- 4 files changed, 128 insertions(+), 21 deletions(-) create mode 100644 .changeset/beige-lemons-accept.md create mode 100644 packages/core-api/src/routing/RouteRef.test.ts diff --git a/.changeset/beige-lemons-accept.md b/.changeset/beige-lemons-accept.md new file mode 100644 index 0000000000..8204b87b5a --- /dev/null +++ b/.changeset/beige-lemons-accept.md @@ -0,0 +1,5 @@ +--- +'@backstage/core-api': patch +--- + +Fully deprecate `title` option of `RouteRef`s and introduce `id` instead. diff --git a/packages/core-api/src/app/App.test.tsx b/packages/core-api/src/app/App.test.tsx index e474a8b851..a9f1e81a11 100644 --- a/packages/core-api/src/app/App.test.tsx +++ b/packages/core-api/src/app/App.test.tsx @@ -33,7 +33,7 @@ import { generateBoundRoutes, PrivateAppImpl } from './App'; describe('generateBoundRoutes', () => { it('runs happy path', () => { const external = { myRoute: createExternalRouteRef({ id: '1' }) }; - const ref = createRouteRef({ path: '', title: '' }); + const ref = createRouteRef({ id: 'ref-1' }); const result = generateBoundRoutes(({ bind }) => { bind(external, { myRoute: ref }); }); @@ -43,7 +43,7 @@ describe('generateBoundRoutes', () => { it('throws on unknown keys', () => { const external = { myRoute: createExternalRouteRef({ id: '2' }) }; - const ref = createRouteRef({ path: '', title: '' }); + const ref = createRouteRef({ id: 'ref-2' }); expect(() => generateBoundRoutes(({ bind }) => { bind(external, { someOtherRoute: ref } as any); @@ -53,12 +53,8 @@ describe('generateBoundRoutes', () => { }); describe('Integration Test', () => { - const plugin1RouteRef = createRouteRef({ path: '/blah1', title: '' }); - const plugin2RouteRef = createRouteRef({ - path: '/blah2', - title: '', - params: ['x'], - }); + const plugin1RouteRef = createRouteRef({ id: 'ref-1' }); + const plugin2RouteRef = createRouteRef({ id: 'ref-2', params: ['x'] }); const subRouteRef1 = createSubRouteRef({ id: 'sub1', path: '/sub1', diff --git a/packages/core-api/src/routing/RouteRef.test.ts b/packages/core-api/src/routing/RouteRef.test.ts new file mode 100644 index 0000000000..279589a6c7 --- /dev/null +++ b/packages/core-api/src/routing/RouteRef.test.ts @@ -0,0 +1,94 @@ +/* + * 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 { AnyParams, RouteRef } from './types'; +import { createRouteRef, isRouteRef } from './RouteRef'; +import { isSubRouteRef } from './SubRouteRef'; +import { isExternalRouteRef } from './ExternalRouteRef'; +import MyIcon from '@material-ui/icons/AcUnit'; + +describe('RouteRef', () => { + it('should be created', () => { + const routeRef: RouteRef = createRouteRef({ + id: 'my-route-ref', + }); + expect(routeRef.params).toEqual([]); + expect(String(routeRef)).toBe('routeRef{type=absolute,id=my-route-ref}'); + expect(isRouteRef(routeRef)).toBe(true); + expect(isSubRouteRef(routeRef)).toBe(false); + expect(isExternalRouteRef(routeRef)).toBe(false); + + expect(isRouteRef({} as RouteRef)).toBe(false); + }); + + it('should be created with params', () => { + const routeRef: RouteRef<{ + x: string; + y: string; + }> = createRouteRef({ + id: 'my-other-route-ref', + params: ['x', 'y'], + }); + expect(routeRef.params).toEqual(['x', 'y']); + }); + + it('should properly infer and validate parameter types and assignments', () => { + function validateType(_ref: RouteRef) {} + + const _1 = createRouteRef({ id: '1', params: ['x'] }); + // @ts-expect-error + validateType<{ y: string }>(_1); + // @ts-expect-error + validateType(_1); + validateType<{ x: string }>(_1); + + const _2 = createRouteRef({ id: '2', params: ['x', 'y'] }); + // @ts-expect-error + validateType<{ x: string }>(_2); + // @ts-expect-error + validateType(_2); + // @ts-expect-error + validateType<{ x: string; z: string }>(_2); + // TODO(Rugvip): Ideally this would fail as well, but settle for validating it at runtime instead + validateType<{ x: string; y: string; z: string }>(_2); + validateType<{ x: string; y: string }>(_2); + + const _3 = createRouteRef({ id: '3', params: [] }); + // @ts-expect-error + validateType<{ x: string }>(_3); + validateType(_3); + + const _4 = createRouteRef({ id: '4' }); + // @ts-expect-error + validateType<{ x: string }>(_4); + validateType(_4); + + // To avoid complains about missing expectations and unused vars + expect([_1, _2, _3, _4].join('')).toEqual(expect.any(String)); + }); + + it('should support deprecated access', () => { + const routeRef = createRouteRef({ + title: 'My Ref', + path: '/my-path', + icon: MyIcon, + }); + expect(routeRef.title).toBe('My Ref'); + expect(routeRef.path).toBe('/my-path'); + expect(routeRef.icon).toBe(MyIcon); + expect(String(routeRef)).toBe('routeRef{type=absolute,id=My Ref}'); + }); +}); diff --git a/packages/core-api/src/routing/RouteRef.ts b/packages/core-api/src/routing/RouteRef.ts index 3f14d2f49b..7ce2b2afbd 100644 --- a/packages/core-api/src/routing/RouteRef.ts +++ b/packages/core-api/src/routing/RouteRef.ts @@ -25,7 +25,7 @@ import { } from './types'; import { IconComponent } from '../icons'; -// TODO(Rugvip): Remove this once we get rid of the deprecated fields, it's not exported +// TODO(Rugvip): Remove this in the next breaking release, it's exported but unused export type RouteRefConfig = { params?: ParamKeys; path?: string; @@ -37,11 +37,15 @@ export class RouteRefImpl implements RouteRef { readonly [routeRefType] = 'absolute'; - constructor(private readonly config: RouteRefConfig) {} - - get params(): ParamKeys { - return this.config.params as any; - } + constructor( + private readonly id: string, + readonly params: ParamKeys, + private readonly config: { + path?: string; + icon?: IconComponent; + title?: string; + }, + ) {} get icon() { return this.config.icon; @@ -53,11 +57,11 @@ export class RouteRefImpl } get title() { - return this.config.title; + return this.config.title ?? this.id; } toString() { - return `routeRef{type=absolute,id=${this.config.title}}`; + return `routeRef{type=absolute,id=${this.id}}`; } } @@ -70,18 +74,26 @@ export function createRouteRef< // Param = {} if the params array is empty. ParamKey extends string = never >(config: { + /** The id of the route ref, used to identify it when printed */ + id?: string; + /** A list of parameter names that the path that this route ref is bound to must contain */ params?: ParamKey[]; /** @deprecated Route refs no longer decide their own path */ path?: string; /** @deprecated Route refs no longer decide their own icon */ icon?: IconComponent; /** @deprecated Route refs no longer decide their own title */ - title: string; + title?: string; }): RouteRef> { - return new RouteRefImpl({ - ...config, - params: (config.params ?? []) as ParamKeys>, - }); + const id = config.id || config.title; + if (!id) { + throw new Error('RouteRef must be provided a non-empty id'); + } + return new RouteRefImpl( + id, + (config.params ?? []) as ParamKeys>, + config, + ); } export function isRouteRef( From 7f60582cec2a07d9121478f6ec10f2c6e9fef98a Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Sun, 7 Mar 2021 13:30:43 +0100 Subject: [PATCH 08/15] core-api: added tests for ExternalRouteRef Signed-off-by: Patrik Oldsberg --- .../src/routing/ExternalRouteRef.test.ts | 119 ++++++++++++++++++ 1 file changed, 119 insertions(+) create mode 100644 packages/core-api/src/routing/ExternalRouteRef.test.ts diff --git a/packages/core-api/src/routing/ExternalRouteRef.test.ts b/packages/core-api/src/routing/ExternalRouteRef.test.ts new file mode 100644 index 0000000000..785ad2732f --- /dev/null +++ b/packages/core-api/src/routing/ExternalRouteRef.test.ts @@ -0,0 +1,119 @@ +/* + * 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 { AnyParams, ExternalRouteRef } from './types'; +import { createExternalRouteRef, isExternalRouteRef } from './ExternalRouteRef'; +import { isSubRouteRef } from './SubRouteRef'; +import { isRouteRef } from './RouteRef'; + +describe('ExternalRouteRef', () => { + it('should be created', () => { + const routeRef: ExternalRouteRef = createExternalRouteRef({ + id: 'my-route-ref', + }); + expect(routeRef.params).toEqual([]); + expect(routeRef.optional).toBe(false); + expect(String(routeRef)).toBe('routeRef{type=external,id=my-route-ref}'); + expect(isRouteRef(routeRef)).toBe(false); + expect(isSubRouteRef(routeRef)).toBe(false); + expect(isExternalRouteRef(routeRef)).toBe(true); + + expect(isRouteRef({} as ExternalRouteRef)).toBe(false); + }); + + it('should be created as optional', () => { + const routeRef: ExternalRouteRef<{ + x: string; + y: string; + }> = createExternalRouteRef({ + id: 'my-other-route-ref', + params: [], + optional: true, + }); + expect(routeRef.params).toEqual([]); + expect(routeRef.optional).toEqual(true); + }); + + it('should be created with params', () => { + const routeRef: ExternalRouteRef<{ + x: string; + y: string; + }> = createExternalRouteRef({ + id: 'my-other-route-ref', + params: ['x', 'y'], + }); + expect(routeRef.params).toEqual(['x', 'y']); + expect(routeRef.optional).toEqual(false); + }); + + it('should be created as optional with params', () => { + const routeRef: ExternalRouteRef<{ + x: string; + y: string; + }> = createExternalRouteRef({ + id: 'my-other-route-ref', + params: ['x', 'y'], + optional: true, + }); + expect(routeRef.params).toEqual(['x', 'y']); + expect(routeRef.optional).toEqual(true); + }); + + it('should properly infer and validate parameter types and assignments', () => { + function validateType( + _ref: ExternalRouteRef, + ) {} + + const _1 = createExternalRouteRef({ id: '1', params: ['notX'] }); + // @ts-expect-error + validateType<{ x: string }, any>(_1); + validateType<{ notX: string }, any>(_1); + + const _2 = createExternalRouteRef({ + id: '2', + params: ['x'], + optional: true, + }); + // @ts-expect-error + validateType(_2); + validateType<{ x: string }, true>(_2); + + const _3 = createExternalRouteRef({ id: '3', params: ['x', 'y'] }); + // @ts-expect-error + validateType<{ x: string }, any>(_3); + // TODO(Rugvip): Ideally this would fail as well, but settle for validating it at runtime instead + validateType<{ x: string; y: string; z: string }, any>(_3); + validateType<{ x: string; y: string }, false>(_3); + + const _4 = createExternalRouteRef({ id: '4', params: [] }); + // @ts-expect-error + validateType<{ x: string }, any>(_4); + validateType(_4); + + const _5 = createExternalRouteRef({ id: '5' }); + // @ts-expect-error + validateType<{ x: string }, any>(_5); + validateType(_5); + + const _6 = createExternalRouteRef({ id: '6', optional: true }); + // @ts-expect-error + validateType(_6); + validateType(_6); + + // To avoid complains about missing expectations and unused vars + expect([_1, _2, _3, _4, _5, _6].join('')).toEqual(expect.any(String)); + }); +}); From 01cfb1dce43b6f9c4a94a9e5a8eb05736b5c2899 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Sun, 7 Mar 2021 14:04:49 +0100 Subject: [PATCH 09/15] core-api: added tests for SubRouteRef + improve path validation Signed-off-by: Patrik Oldsberg --- .../core-api/src/routing/SubRouteRef.test.ts | 134 ++++++++++++++++++ packages/core-api/src/routing/SubRouteRef.ts | 20 ++- 2 files changed, 151 insertions(+), 3 deletions(-) create mode 100644 packages/core-api/src/routing/SubRouteRef.test.ts diff --git a/packages/core-api/src/routing/SubRouteRef.test.ts b/packages/core-api/src/routing/SubRouteRef.test.ts new file mode 100644 index 0000000000..1c1a3c1b21 --- /dev/null +++ b/packages/core-api/src/routing/SubRouteRef.test.ts @@ -0,0 +1,134 @@ +/* + * 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 { AnyParams, SubRouteRef } from './types'; +import { createSubRouteRef, isSubRouteRef } from './SubRouteRef'; +import { createRouteRef, isRouteRef } from './RouteRef'; +import { isExternalRouteRef } from './ExternalRouteRef'; + +const parent = createRouteRef({ id: 'parent' }); +const parentX = createRouteRef({ id: 'parent-x', params: ['x'] }); + +describe('SubRouteRef', () => { + it('should be created', () => { + const routeRef: SubRouteRef = createSubRouteRef({ + parent, + id: 'my-route-ref', + path: '/foo', + }); + expect(routeRef.path).toBe('/foo'); + expect(routeRef.parent).toBe(parent); + expect(routeRef.params).toEqual([]); + expect(String(routeRef)).toBe('routeRef{type=sub,id=my-route-ref}'); + expect(isRouteRef(routeRef)).toBe(false); + expect(isSubRouteRef(routeRef)).toBe(true); + expect(isExternalRouteRef(routeRef)).toBe(false); + + expect(isRouteRef({} as SubRouteRef)).toBe(false); + }); + + it('should be created with params', () => { + const routeRef: SubRouteRef<{ bar: string }> = createSubRouteRef({ + parent, + id: 'my-other-route-ref', + path: '/foo/:bar', + }); + expect(routeRef.path).toBe('/foo/:bar'); + expect(routeRef.parent).toBe(parent); + expect(routeRef.params).toEqual(['bar']); + }); + + it('should be created with merged params', () => { + const routeRef: SubRouteRef<{ + x: string; + y: string; + z: string; + }> = createSubRouteRef({ + parent: parentX, + id: 'my-other-route-ref', + path: '/foo/:y/:z', + }); + expect(routeRef.path).toBe('/foo/:y/:z'); + expect(routeRef.parent).toBe(parentX); + expect(routeRef.params).toEqual(['x', 'y', 'z']); + }); + + it('should be created with params from parent', () => { + const routeRef: SubRouteRef<{ x: string }> = createSubRouteRef({ + parent: parentX, + id: 'my-other-route-ref', + path: '/foo/bar', + }); + expect(routeRef.path).toBe('/foo/bar'); + expect(routeRef.parent).toBe(parentX); + expect(routeRef.params).toEqual(['x']); + }); + + it.each([ + ['foo', "SubRouteRef path must start with '/', got 'foo'"], + [':foo', "SubRouteRef path must start with '/', got ':foo'"], + ['', "SubRouteRef path must start with '/', got ''"], + ['/', "SubRouteRef path must not end with '/', got '/'"], + ['/foo/', "SubRouteRef path must not end with '/', got '/foo/'"], + ['/foo/:x', 'SubRouteRef may not have params that overlap with its parent'], + ['/:/foo', "SubRouteRef path has invalid param, got ''"], + ['/:inva:lid/foo', "SubRouteRef path has invalid param, got 'inva:lid'"], + ['/:inva=lid/foo', "SubRouteRef path has invalid param, got 'inva=lid'"], + ])('should throw if path is invalid, %s', (path, message) => { + expect(() => + createSubRouteRef({ path, parent: parentX, id: path }), + ).toThrow(message); + }); + + it('should properly infer and parse path parameters', () => { + function validateType(_ref: SubRouteRef) {} + + const _1 = createSubRouteRef({ id: '1', parent, path: '/foo/bar' }); + // @ts-expect-error + validateType<{ x: string }>(_1); + validateType(_1); + + const _2 = createSubRouteRef({ id: '2', parent, path: '/foo/:x/:y' }); + // @ts-expect-error + validateType(_2); + // @ts-expect-error + validateType<{ x: string; z: string }>(_2); + // @ts-expect-error + validateType<{ y: string }>(_2); + // TODO(Rugvip): Ideally this would fail as well, but settle for validating it at runtime instead + validateType<{ x: string; y: string; z: string }>(_2); + validateType<{ x: string; y: string }>(_2); + + const _3 = createSubRouteRef({ id: '3', parent: parentX, path: '/foo' }); + // @ts-expect-error + validateType(_3); + // @ts-expect-error + validateType<{ y: string }>(_3); + validateType<{ x: string }>(_3); + + const _4 = createSubRouteRef({ id: '4', parent: parentX, path: '/foo/:y' }); + // @ts-expect-error + validateType(_4); + // @ts-expect-error + validateType<{ x: string; z: string }>(_4); + // @ts-expect-error + validateType<{ y: string }>(_4); + validateType<{ x: string; y: string }>(_4); + + // To avoid complains about missing expectations and unused vars + expect([_1, _2, _3, _4].join('')).toEqual(expect.any(String)); + }); +}); diff --git a/packages/core-api/src/routing/SubRouteRef.ts b/packages/core-api/src/routing/SubRouteRef.ts index 8ff23c0c5e..7ddfc89c80 100644 --- a/packages/core-api/src/routing/SubRouteRef.ts +++ b/packages/core-api/src/routing/SubRouteRef.ts @@ -24,6 +24,9 @@ import { SubRouteRef, } from './types'; +// Should match the pattern in react-router +const PARAM_PATTERN = /^\w+$/; + export class SubRouteRefImpl implements SubRouteRef { readonly [routeRefType] = 'sub'; @@ -79,16 +82,27 @@ export function createSubRouteRef< type Params = PathParams; // Collect runtime parameters from the path, e.g. ['bar', 'baz'] from '/foo/:bar/:baz' - const pathParams = path.split(/:([^/]+)/).filter((_, i) => i % 2 === 1); + const pathParams = path + .split('/') + .filter(p => p.startsWith(':')) + .map(p => p.substring(1)); const params = [...parent.params, ...pathParams]; if (parent.params.some(p => pathParams.includes(p as string))) { throw new Error( - 'SubRouteRef may not have params that overlap with its parent params', + 'SubRouteRef may not have params that overlap with its parent', ); } if (!path.startsWith('/')) { - throw new Error(`SubRouteRef path sub starts with '/', got '${path}'`); + throw new Error(`SubRouteRef path must start with '/', got '${path}'`); + } + if (path.endsWith('/')) { + throw new Error(`SubRouteRef path must not end with '/', got '${path}'`); + } + for (const param of pathParams) { + if (!PARAM_PATTERN.test(param)) { + throw new Error(`SubRouteRef path has invalid param, got '${param}'`); + } } // We ensure that the type of the return type is sane here From 3f18d8b135daa9229ccd47981a3334c509676845 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Sun, 7 Mar 2021 19:50:19 +0100 Subject: [PATCH 10/15] core-api: added tests for RouteResolver + tweak sourceLocation type Signed-off-by: Patrik Oldsberg --- .../src/routing/RouteResolver.test.ts | 236 ++++++++++++++++++ .../core-api/src/routing/RouteResolver.ts | 4 +- 2 files changed, 238 insertions(+), 2 deletions(-) create mode 100644 packages/core-api/src/routing/RouteResolver.test.ts diff --git a/packages/core-api/src/routing/RouteResolver.test.ts b/packages/core-api/src/routing/RouteResolver.test.ts new file mode 100644 index 0000000000..5b7e655a46 --- /dev/null +++ b/packages/core-api/src/routing/RouteResolver.test.ts @@ -0,0 +1,236 @@ +/* + * 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 { createRouteRef } from './RouteRef'; +import { createSubRouteRef } from './SubRouteRef'; +import { createExternalRouteRef } from './ExternalRouteRef'; +import { RouteResolver } from './RouteResolver'; +import { AnyRouteRef, ExternalRouteRef, RouteRef, SubRouteRef } from './types'; + +const element = () => null; +const rest = { element, caseSensitive: false }; + +const ref1 = createRouteRef({ id: 'rr1' }); +const ref2 = createRouteRef({ id: 'rr2', params: ['x'] }); +const ref3 = createRouteRef({ id: 'rr3', params: ['y'] }); +const subRef1 = createSubRouteRef({ + id: 'srr1', + parent: ref1, + path: '/foo', +}); +const subRef2 = createSubRouteRef({ + id: 'srr2', + parent: ref1, + path: '/foo/:a', +}); +const subRef3 = createSubRouteRef({ + id: 'srr3', + parent: ref2, + path: '/bar', +}); +const subRef4 = createSubRouteRef({ + id: 'srr4', + parent: ref2, + path: '/bar/:a', +}); +const externalRef1 = createExternalRouteRef({ id: 'err1' }); +const externalRef2 = createExternalRouteRef({ + id: 'err2', + optional: true, +}); +const externalRef3 = createExternalRouteRef({ id: 'err3', params: ['x'] }); +const externalRef4 = createExternalRouteRef({ + id: 'err4', + optional: true, + params: ['x'], +}); + +describe('RouteResolver', () => { + it('should not resolve anything with an empty resolver', () => { + const r = new RouteResolver(new Map(), new Map(), [], new Map()); + + expect(r.resolve(ref1, '/')?.()).toBe(undefined); + expect(r.resolve(ref2, '/')?.({ x: '1x' })).toBe(undefined); + expect(r.resolve(subRef1, '/')?.()).toBe(undefined); + expect(r.resolve(subRef2, '/')?.({ a: '2a' })).toBe(undefined); + 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', () => { + const r = new RouteResolver( + new Map([[ref1, '/my-route']]), + new Map(), + [{ routeRefs: new Set([ref1]), path: '/my-route', ...rest }], + new Map(), + ); + + expect(r.resolve(ref1, '/')?.()).toBe('/my-route'); + expect(r.resolve(ref2, '/')?.({ x: '1x' })).toBe(undefined); + expect(r.resolve(subRef1, '/')?.()).toBe('/my-route/foo'); + expect(r.resolve(subRef2, '/')?.({ a: '2a' })).toBe('/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([ + [ref1, '/my-route'], + [ref2, '/my-parent/:x'], + ]), + new Map([[ref2, ref1]]), + [ + { + routeRefs: new Set([ref2]), + path: '/my-parent/:x', + ...rest, + children: [ + { routeRefs: new Set([ref1]), path: '/my-route', ...rest }, + ], + }, + ], + new Map([ + [externalRef1, ref1], + [externalRef3, ref2], + [externalRef4, subRef3], + ]), + ); + + expect(r.resolve(ref1, '/')?.()).toBe('/my-route'); + expect(r.resolve(ref2, '/')?.({ x: '1x' })).toBe('/my-route/my-parent/1x'); + expect(r.resolve(subRef1, '/')?.()).toBe('/my-route/foo'); + expect(r.resolve(subRef2, '/')?.({ a: '2a' })).toBe('/my-route/foo/2a'); + expect(r.resolve(subRef3, '/')?.({ x: '3x' })).toBe( + '/my-route/my-parent/3x/bar', + ); + expect(r.resolve(subRef4, '/')?.({ x: '4x', a: '4a' })).toBe( + '/my-route/my-parent/4x/bar/4a', + ); + expect(r.resolve(externalRef1, '/')?.()).toBe('/my-route'); + expect(r.resolve(externalRef2, '/')?.()).toBe(undefined); + expect(r.resolve(externalRef3, '/')?.({ x: '5x' })).toBe( + '/my-route/my-parent/5x', + ); + expect(r.resolve(externalRef4, '/')?.({ x: '6x' })).toBe( + '/my-route/my-parent/6x/bar', + ); + }); + + it('should resolve an absolute route with multiple parents', () => { + const r = new RouteResolver( + new Map([ + [ref1, '/my-route'], + [ref2, '/my-parent/:x'], + [ref3, '/my-grandparent/:y'], + ]), + new Map([ + [ref1, ref2], + [ref2, ref3], + ]), + [ + { + routeRefs: new Set([ref3]), + path: '/my-grandparent/:y', + ...rest, + children: [ + { + routeRefs: new Set([ref2]), + path: '/my-parent/:x', + ...rest, + children: [ + { routeRefs: new Set([ref1]), path: '/my-route', ...rest }, + ], + }, + ], + }, + ], + new Map([ + [externalRef1, ref1], + [externalRef3, ref2], + [externalRef4, subRef3], + ]), + ); + + const l = '/my-grandparent/my-y/my-parent/my-x'; + expect(r.resolve(ref1, l)?.()).toBe( + '/my-grandparent/my-y/my-parent/my-x/my-route', + ); + expect(() => r.resolve(ref1, '/')?.()).toThrow( + /^Cannot route.*with parent.*as it has parameters$/, + ); + expect(r.resolve(ref2, l)?.({ x: '1x' })).toBe( + '/my-grandparent/my-y/my-parent/1x', + ); + expect(r.resolve(ref2, '/my-grandparent/my-y')?.({ x: '1x' })).toBe( + '/my-grandparent/my-y/my-parent/1x', + ); + expect(() => r.resolve(ref2, '/')?.({ x: '1x' })).toThrow( + /^Cannot route.*with parent.*as it has parameters$/, + ); + expect(r.resolve(subRef1, l)?.()).toBe( + '/my-grandparent/my-y/my-parent/my-x/my-route/foo', + ); + expect(() => r.resolve(subRef1, '/')?.()).toThrow( + /^Cannot route.*with parent.*as it has parameters$/, + ); + expect(r.resolve(subRef2, l)?.({ a: '2a' })).toBe( + '/my-grandparent/my-y/my-parent/my-x/my-route/foo/2a', + ); + expect(() => r.resolve(subRef2, '/')?.({ a: '2a' })).toThrow( + /^Cannot route.*with parent.*as it has parameters$/, + ); + expect(r.resolve(subRef3, l)?.({ x: '3x' })).toBe( + '/my-grandparent/my-y/my-parent/3x/bar', + ); + expect(r.resolve(subRef3, '/my-grandparent/my-y')?.({ x: '3x' })).toBe( + '/my-grandparent/my-y/my-parent/3x/bar', + ); + expect(r.resolve(subRef4, l)?.({ x: '4x', a: '4a' })).toBe( + '/my-grandparent/my-y/my-parent/4x/bar/4a', + ); + expect( + r.resolve(subRef4, '/my-grandparent/my-y')?.({ x: '4x', a: '4a' }), + ).toBe('/my-grandparent/my-y/my-parent/4x/bar/4a'); + expect(r.resolve(externalRef1, l)?.()).toBe( + '/my-grandparent/my-y/my-parent/my-x/my-route', + ); + expect(() => r.resolve(externalRef1, '/')?.()).toThrow( + /^Cannot route.*with parent.*as it has parameters$/, + ); + expect(r.resolve(externalRef2, l)?.()).toBe(undefined); + expect(r.resolve(externalRef3, l)?.({ x: '5x' })).toBe( + '/my-grandparent/my-y/my-parent/5x', + ); + expect(() => r.resolve(externalRef3, '/')?.({ x: '5x' })).toThrow( + /^Cannot route.*with parent.*as it has parameters$/, + ); + expect(r.resolve(externalRef4, l)?.({ x: '6x' })).toBe( + '/my-grandparent/my-y/my-parent/6x/bar', + ); + expect(() => r.resolve(externalRef4, '/')?.({ x: '6x' })).toThrow( + /^Cannot route.*with parent.*as it has parameters$/, + ); + }); +}); diff --git a/packages/core-api/src/routing/RouteResolver.ts b/packages/core-api/src/routing/RouteResolver.ts index 511d6bce2e..f417b9af2a 100644 --- a/packages/core-api/src/routing/RouteResolver.ts +++ b/packages/core-api/src/routing/RouteResolver.ts @@ -14,7 +14,7 @@ * limitations under the License. */ -import { generatePath, matchRoutes, useLocation } from 'react-router-dom'; +import { generatePath, matchRoutes } from 'react-router-dom'; import { AnyRouteRef, BackstageRouteObject, @@ -45,7 +45,7 @@ export class RouteResolver { | RouteRef | SubRouteRef | ExternalRouteRef, - sourceLocation: ReturnType, + sourceLocation: Parameters[1], ): RouteFunc | undefined { let resolvedRef: AnyRouteRef; let subRoutePath = ''; From a3ed283ce25e508edbf9c7d3572fa7123ea0c481 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Sun, 7 Mar 2021 23:13:01 +0100 Subject: [PATCH 11/15] core-api: refactor RouteResolver Signed-off-by: Patrik Oldsberg --- .../src/routing/RouteResolver.test.ts | 8 +- .../core-api/src/routing/RouteResolver.ts | 268 ++++++++++++------ packages/core-api/src/routing/hooks.tsx | 5 +- packages/core-api/src/routing/types.ts | 2 +- 4 files changed, 186 insertions(+), 97 deletions(-) diff --git a/packages/core-api/src/routing/RouteResolver.test.ts b/packages/core-api/src/routing/RouteResolver.test.ts index 5b7e655a46..e5394f6163 100644 --- a/packages/core-api/src/routing/RouteResolver.test.ts +++ b/packages/core-api/src/routing/RouteResolver.test.ts @@ -18,7 +18,7 @@ import { createRouteRef } from './RouteRef'; import { createSubRouteRef } from './SubRouteRef'; import { createExternalRouteRef } from './ExternalRouteRef'; import { RouteResolver } from './RouteResolver'; -import { AnyRouteRef, ExternalRouteRef, RouteRef, SubRouteRef } from './types'; +import { ExternalRouteRef, RouteRef, SubRouteRef } from './types'; const element = () => null; const rest = { element, caseSensitive: false }; @@ -96,7 +96,7 @@ describe('RouteResolver', () => { it('should resolve an absolute route with a param and with a parent', () => { const r = new RouteResolver( - new Map([ + new Map([ [ref1, '/my-route'], [ref2, '/my-parent/:x'], ]), @@ -140,12 +140,12 @@ describe('RouteResolver', () => { it('should resolve an absolute route with multiple parents', () => { const r = new RouteResolver( - new Map([ + new Map([ [ref1, '/my-route'], [ref2, '/my-parent/:x'], [ref3, '/my-grandparent/:y'], ]), - new Map([ + new Map([ [ref1, ref2], [ref2, ref3], ]), diff --git a/packages/core-api/src/routing/RouteResolver.ts b/packages/core-api/src/routing/RouteResolver.ts index f417b9af2a..e22b6259be 100644 --- a/packages/core-api/src/routing/RouteResolver.ts +++ b/packages/core-api/src/routing/RouteResolver.ts @@ -29,10 +29,169 @@ import { isRouteRef } from './RouteRef'; import { isSubRouteRef } from './SubRouteRef'; import { isExternalRouteRef } from './ExternalRouteRef'; +// Joins a list of paths together, avoiding trailing and duplicate slashes +function joinPaths(...paths: string[]): string { + const joined = paths + .map(path => { + let ret = path; + if (path.endsWith('/')) { + ret = path.slice(0, -1); + } + if (!path.startsWith('/')) { + ret = `/${ret}`; + } + return ret; + }) + .join(''); + + if (joined !== '/' && joined.endsWith('/')) { + return joined.slice(0, -1); + } + return joined; +} + +/** + * Resolves the absolute route ref that our target route ref is pointing pointing to, as well + * as the relative target path. + * + * Returns an undefined target ref if one could not be fully resolved. + */ +function resolveTargetRef( + anyRouteRef: AnyRouteRef, + routePaths: Map, + routeBindings: Map, +): readonly [RouteRef | undefined, string] { + // First we figure out which absolute route ref we're dealing with, an if there was an sub route path to append. + // For sub routes it will be the parent path, while for external routes it will be the bound route. + let targetRef: RouteRef; + let subRoutePath = ''; + if (isRouteRef(anyRouteRef)) { + targetRef = anyRouteRef; + } else if (isSubRouteRef(anyRouteRef)) { + targetRef = anyRouteRef.parent; + subRoutePath = anyRouteRef.path; + } else if (isExternalRouteRef(anyRouteRef)) { + const resolvedRoute = routeBindings.get(anyRouteRef); + if (!resolvedRoute) { + return [undefined, '']; + } + if (isRouteRef(resolvedRoute)) { + targetRef = resolvedRoute; + } else if (isSubRouteRef(resolvedRoute)) { + targetRef = resolvedRoute.parent; + subRoutePath = resolvedRoute.path; + } else { + throw new Error( + `ExternalRouteRef was bound to invalid target, ${resolvedRoute}`, + ); + } + } else if (anyRouteRef[routeRefType]) { + throw new Error( + `Unknown or invalid route ref type, ${anyRouteRef[routeRefType]}`, + ); + } else { + throw new Error(`Unknown object passed to useRouteRef, got ${anyRouteRef}`); + } + + // Bail if no absolute path could be resolved + if (!targetRef) { + return [undefined, '']; + } + + // Find the path that our target route is bound to + const resolvedPath = routePaths.get(targetRef); + if (!resolvedPath) { + return [undefined, '']; + } + + // SubRouteRefs join the path from the parent route with its own path + const targetPath = joinPaths(resolvedPath, subRoutePath); + return [targetRef, targetPath]; +} + +/** + * Resolves the complete base path for navigating to the target RouteRef. + */ +function resolveBasePath( + targetRef: RouteRef, + sourceLocation: Parameters[1], + routePaths: Map, + routeParents: Map, + routeObjects: BackstageRouteObject[], +) { + // While traversing the app element tree we build up the routeObjects structure + // used here. It is the same kind of structure that react-router creates, with the + // addition that associated route refs are stored throughout the tree. This lets + // us look up all route refs that can be reached from our source location. + // Because of the similar route object structure, we can use `matchRoutes` from + // react-router to do the lookup of our current location. + const match = matchRoutes(routeObjects, sourceLocation) ?? []; + + // While we search for a common routing root between our current location and + // the target route, we build a list of all route refs we find that we need + // to traverse to reach the target. + const refDiffList = Array(); + + let matchIndex = -1; + for ( + let targetSearchRef: RouteRef | undefined = targetRef; + targetSearchRef; + targetSearchRef = routeParents.get(targetSearchRef) + ) { + // The match contains a list of all ancestral route refs present at our current location + // Starting at the desired target ref and traversing back through its parents, we search + // for a target ref that is present in the match for our current location. When a match + // is found it means we have found a common base to resolve the route from. + matchIndex = match.findIndex(m => + (m.route as BackstageRouteObject).routeRefs.has(targetSearchRef!), + ); + if (matchIndex !== -1) { + break; + } + + // Every time we move a step up in the ancestry of the target ref, we add the current ref + // to the diff list, which ends up being the list of route refs to traverse form the common base + // in order to reach our target. + refDiffList.unshift(targetSearchRef); + } + + // If our target route is present in the initial match we need to construct the final path + // from the parent of the matched route segment. That's to allow the caller of the route + // function to supply their own params. + if (refDiffList.length === 0) { + matchIndex -= 1; + } + + // This is the part of the route tree that the target and source locations have in common. + // We re-use the existing pathname directly along with all params. + const parentPath = matchIndex === -1 ? '' : match[matchIndex].pathname; + + // This constructs the mid section of the path using paths resolved from all route refs + // we need to traverse to reach our target except for the very last one. None of these + // paths are allowed to require any parameters, as the caller would have no way of knowing + // what parameters those are. + const diffPath = joinPaths( + ...refDiffList.slice(0, -1).map(ref => { + const path = routePaths.get(ref); + if (!path) { + throw new Error(`No path for ${ref}`); + } + if (path.includes(':')) { + throw new Error( + `Cannot route to ${targetRef} with parent ${ref} as it has parameters`, + ); + } + return path; + }), + ); + + return parentPath + diffPath; +} + export class RouteResolver { constructor( - private readonly routePaths: Map, - private readonly routeParents: Map, + private readonly routePaths: Map, + private readonly routeParents: Map, private readonly routeObjects: BackstageRouteObject[], private readonly routeBindings: Map< ExternalRouteRef, @@ -47,98 +206,29 @@ export class RouteResolver { | ExternalRouteRef, sourceLocation: Parameters[1], ): RouteFunc | undefined { - let resolvedRef: AnyRouteRef; - let subRoutePath = ''; - if (isRouteRef(anyRouteRef)) { - resolvedRef = anyRouteRef; - } else if (isSubRouteRef(anyRouteRef)) { - resolvedRef = anyRouteRef.parent; - subRoutePath = anyRouteRef.path; - } else if (isExternalRouteRef(anyRouteRef)) { - const resolvedRoute = this.routeBindings.get(anyRouteRef); - if (!resolvedRoute) { - return undefined; - } - if (isSubRouteRef(resolvedRoute)) { - subRoutePath = resolvedRoute.path; - resolvedRef = resolvedRoute.parent; - } else { - resolvedRef = resolvedRoute; - } - } else if (anyRouteRef[routeRefType]) { - throw new Error( - `Unknown or invalid route ref type, ${anyRouteRef[routeRefType]}`, - ); - } else { - throw new Error( - `Unknown object passed to useRouteRef, got ${anyRouteRef}`, - ); - } - - const match = matchRoutes(this.routeObjects, sourceLocation) ?? []; - - // If our route isn't bound to a path we fail the resolution and let the caller decide the failure mode - const resolvedPath = this.routePaths.get(resolvedRef); - if (!resolvedPath) { + // First figure out what our target absolute ref is, as well as our target path. + const [targetRef, targetPath] = resolveTargetRef( + anyRouteRef, + this.routePaths, + this.routeBindings, + ); + if (!targetRef) { return undefined; } - // SubRouteRefs join the path from the parent route with its own path - const lastPath = - resolvedPath + - (resolvedPath.endsWith('/') ? subRoutePath.slice(1) : subRoutePath); - const targetRefStack = Array(); - let matchIndex = -1; - - for ( - let currentRouteRef: AnyRouteRef | undefined = resolvedRef; - currentRouteRef; - currentRouteRef = this.routeParents.get(currentRouteRef) - ) { - matchIndex = match.findIndex(m => - (m.route as BackstageRouteObject).routeRefs.has(currentRouteRef!), - ); - if (matchIndex !== -1) { - break; - } - - targetRefStack.unshift(currentRouteRef); - } - - // If our target route is present in the initial match we need to construct the final path - // from the parent of the matched route segment. That's to allow the caller of the route - // function to supply their own params. - if (targetRefStack.length === 0) { - matchIndex -= 1; - } - - // This is the part of the route tree that the target and source locations have in common. - // We re-use the existing pathname directly along with all params. - const parentPath = matchIndex === -1 ? '' : match[matchIndex].pathname; - - // This constructs the mid section of the path using paths resolved from all route refs - // we need to traverse to reach our target except for the very last one. None of these - // paths are allowed to require any parameters, as the called would have no way of knowing - // what parameters those are. - const prefixPath = targetRefStack - .slice(0, -1) - .map(ref => { - const path = this.routePaths.get(ref); - if (!path) { - throw new Error(`No path for ${ref}`); - } - if (path.includes(':')) { - throw new Error( - `Cannot route to ${resolvedRef} with parent ${ref} as it has parameters`, - ); - } - return path; - }) - .join('/') - .replace(/\/\/+/g, '/'); // Normalize path to not contain repeated /'s + // 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 routeFunc: RouteFunc = (...[params]) => { - return `${parentPath}${prefixPath}${generatePath(lastPath, params)}`; + return basePath + generatePath(targetPath, params); }; return routeFunc; } diff --git a/packages/core-api/src/routing/hooks.tsx b/packages/core-api/src/routing/hooks.tsx index bc43ff8685..2e65efaba6 100644 --- a/packages/core-api/src/routing/hooks.tsx +++ b/packages/core-api/src/routing/hooks.tsx @@ -17,7 +17,6 @@ import React, { createContext, ReactNode, useContext, useMemo } from 'react'; import { useLocation } from 'react-router-dom'; import { - AnyRouteRef, BackstageRouteObject, RouteRef, ExternalRouteRef, @@ -61,8 +60,8 @@ export function useRouteRef( } type ProviderProps = { - routePaths: Map; - routeParents: Map; + routePaths: Map; + routeParents: Map; routeObjects: BackstageRouteObject[]; routeBindings: Map; children: ReactNode; diff --git a/packages/core-api/src/routing/types.ts b/packages/core-api/src/routing/types.ts index 65c12ff954..56b991f3af 100644 --- a/packages/core-api/src/routing/types.ts +++ b/packages/core-api/src/routing/types.ts @@ -93,5 +93,5 @@ export interface BackstageRouteObject { children?: BackstageRouteObject[]; element: React.ReactNode; path: string; - routeRefs: Set; + routeRefs: Set; } From a7967040f4a25ec1b43859c036be9906f63f9c97 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Sun, 7 Mar 2021 23:16:11 +0100 Subject: [PATCH 12/15] core-api: simplify joinPaths Signed-off-by: Patrik Oldsberg --- .../core-api/src/routing/RouteResolver.ts | 20 ++++--------------- 1 file changed, 4 insertions(+), 16 deletions(-) diff --git a/packages/core-api/src/routing/RouteResolver.ts b/packages/core-api/src/routing/RouteResolver.ts index e22b6259be..fb44b8c486 100644 --- a/packages/core-api/src/routing/RouteResolver.ts +++ b/packages/core-api/src/routing/RouteResolver.ts @@ -31,23 +31,11 @@ import { isExternalRouteRef } from './ExternalRouteRef'; // Joins a list of paths together, avoiding trailing and duplicate slashes function joinPaths(...paths: string[]): string { - const joined = paths - .map(path => { - let ret = path; - if (path.endsWith('/')) { - ret = path.slice(0, -1); - } - if (!path.startsWith('/')) { - ret = `/${ret}`; - } - return ret; - }) - .join(''); - - if (joined !== '/' && joined.endsWith('/')) { - return joined.slice(0, -1); + const normalized = paths.join('/').replace(/\/\/+/g, '/'); + if (normalized !== '/' && normalized.endsWith('/')) { + return normalized.slice(0, -1); } - return joined; + return normalized; } /** From 6fb4258a83588276fa5d5d18e83b3686d21a54c1 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Sun, 7 Mar 2021 23:23:55 +0100 Subject: [PATCH 13/15] changesets: added changeset for SubRoutes Signed-off-by: Patrik Oldsberg --- .changeset/heavy-lemons-hope.md | 41 +++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) create mode 100644 .changeset/heavy-lemons-hope.md diff --git a/.changeset/heavy-lemons-hope.md b/.changeset/heavy-lemons-hope.md new file mode 100644 index 0000000000..7a47ec3644 --- /dev/null +++ b/.changeset/heavy-lemons-hope.md @@ -0,0 +1,41 @@ +--- +'@backstage/core-api': patch +--- + +Add `SubRouteRef`s, which can be used to create a route ref with a fixed path relative to an absolute `RouteRef`. They are useful if you for example have a page that is mounted at a sub route of a routable extension component, and you want other plugins to be able to route to that page. + +For example: + +```tsx +// routes.ts +const rootRouteRef = createRouteRef({ id: 'root' }); +const detailsRouteRef = createSubRouteRef({ + id: 'root-sub', + parent: rootRouteRef, + path: '/details', +}); + +// plugin.ts +export const myPlugin = createPlugin({ + routes: { + root: rootRouteRef, + details: detailsRouteRef, + } +}) + +export const MyPage = plugin.provide(createRoutableExtension({ + component: () => import('./components/MyPage').then(m => m.MyPage), + mountPoint: rootRouteRef, +})) + +// components/MyPage.tsx +const MyPage = () => ( + + {/* myPlugin.routes.root will take the user to this page */} + }> + + {/* myPlugin.routes.details will take the user to this page */} + }> + +) +``` From 2c9c165b2e7d270ec368eed1d41e38d0d5aa2b66 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Sun, 7 Mar 2021 23:25:57 +0100 Subject: [PATCH 14/15] docs/plugins/composability: added docs for SubRouteRefs Signed-off-by: Patrik Oldsberg --- .github/styles/vocab.txt | 1 + docs/plugins/composability.md | 44 +++++++++++++++++++++++++++++++++++ 2 files changed, 45 insertions(+) diff --git a/.github/styles/vocab.txt b/.github/styles/vocab.txt index fcc5d63d20..91385d397c 100644 --- a/.github/styles/vocab.txt +++ b/.github/styles/vocab.txt @@ -250,6 +250,7 @@ stefanalund subcomponent subcomponents subkey +subroutes subtree superfences superset diff --git a/docs/plugins/composability.md b/docs/plugins/composability.md index 75993dbc46..e95a67417b 100644 --- a/docs/plugins/composability.md +++ b/docs/plugins/composability.md @@ -369,6 +369,50 @@ It is currently not possible to have parameterized `ExternalRouteRef`s, or to bind an external route to a parameterized route, although this may be added in the future if needed. +### Subroutes + +The last kind of route refs that can be created are `SubRouteRef`s, which can be +used to create a route ref with a fixed path relative to an absolute `RouteRef`. +They are useful if you have a page that internally is mounted at a sub route of +a routable extension component, and you want other plugins to be able to route +to that page. + +For example: + +```tsx +// routes.ts +const rootRouteRef = createRouteRef({ id: 'root' }); +const detailsRouteRef = createSubRouteRef({ + id: 'root-sub', + parent: rootRouteRef, + path: '/details', +}); + +// plugin.ts +export const myPlugin = createPlugin({ + routes: { + root: rootRouteRef, + details: detailsRouteRef, + } +}) + +export const MyPage = plugin.provide(createRoutableExtension({ + component: () => import('./components/MyPage').then(m => m.MyPage), + mountPoint: rootRouteRef, +})) + +// components/MyPage.tsx +const MyPage = () => ( + + {/* myPlugin.routes.root will take the user to this page */} + }> + + {/* myPlugin.routes.details will take the user to this page */} + }> + +) +``` + ### New Catalog Components The established pattern for selecting what plugins should be available on each From 47c11f3748040601524a1a0ed579607703d5d90e Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Mon, 8 Mar 2021 16:13:15 +0100 Subject: [PATCH 15/15] test-utils: add workaround for external routes no longer being resolved when mounted Signed-off-by: Patrik Oldsberg --- .../test-utils/src/testUtils/appWrappers.tsx | 31 +++++++++++++++++-- 1 file changed, 29 insertions(+), 2 deletions(-) diff --git a/packages/test-utils/src/testUtils/appWrappers.tsx b/packages/test-utils/src/testUtils/appWrappers.tsx index 05cd0b8fd4..301366cbbd 100644 --- a/packages/test-utils/src/testUtils/appWrappers.tsx +++ b/packages/test-utils/src/testUtils/appWrappers.tsx @@ -24,6 +24,7 @@ import privateExports, { RouteRef, ExternalRouteRef, attachComponentData, + createRouteRef, } from '@backstage/core-api'; import { RenderResult } from '@testing-library/react'; import { renderWithEffects } from '@backstage/test-utils-core'; @@ -65,6 +66,13 @@ type TestAppOptions = { mountedRoutes?: { [path: string]: RouteRef | ExternalRouteRef }; }; +function isExternalRouteRef( + routeRef: RouteRef | ExternalRouteRef, +): routeRef is ExternalRouteRef { + // TODO(Rugvip): Least ugly workaround for now, but replace :D + return String(routeRef).includes('{type=external,'); +} + /** * Wraps a component inside a Backstage test app, providing a mocked theme * and app context, along with mocked APIs. @@ -77,6 +85,7 @@ export function wrapInTestApp( options: TestAppOptions = {}, ): ReactElement { const { routeEntries = ['/'] } = options; + const boundRoutes = new Map(); const app = new PrivateAppImpl({ apis: [], @@ -99,7 +108,16 @@ export function wrapInTestApp( }, ], defaultApis: mockApis, - bindRoutes: () => {}, + bindRoutes: ({ bind }) => { + for (const [externalRef, absoluteRef] of boundRoutes) { + bind( + { ref: externalRef }, + { + ref: absoluteRef, + }, + ); + } + }, }); let Wrapper: ComponentType; @@ -112,7 +130,16 @@ export function wrapInTestApp( const routeElements = Object.entries(options.mountedRoutes ?? {}).map( ([path, routeRef]) => { const Page = () =>
Mounted at {path}
; - attachComponentData(Page, 'core.mountPoint', routeRef); + + // Allow external route refs to be bound to paths as well, for convenience. + // We work around it by creating and binding an absolute ref to the external one. + if (isExternalRouteRef(routeRef)) { + const absoluteRef = createRouteRef({ id: 'id' }); + boundRoutes.set(routeRef, absoluteRef); + attachComponentData(Page, 'core.mountPoint', absoluteRef); + } else { + attachComponentData(Page, 'core.mountPoint', routeRef); + } return } />; }, );