From 2e2c9dfb97c3e5f7d913640555edae52697c7e94 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Wed, 6 Apr 2022 11:10:03 +0200 Subject: [PATCH 1/3] core-app-api: initial refactor into a merged route collector Signed-off-by: Patrik Oldsberg --- packages/core-app-api/src/app/AppManager.tsx | 24 +- .../core-app-api/src/routing/RouteTracker.tsx | 10 +- .../src/routing/RoutingProvider.test.tsx | 29 +-- .../src/routing/collectors.test.tsx | 54 ++--- .../core-app-api/src/routing/collectors.tsx | 229 +++++++++--------- 5 files changed, 162 insertions(+), 184 deletions(-) diff --git a/packages/core-app-api/src/app/AppManager.tsx b/packages/core-app-api/src/app/AppManager.tsx index 0262ece8f4..77a1ecc7cf 100644 --- a/packages/core-app-api/src/app/AppManager.tsx +++ b/packages/core-app-api/src/app/AppManager.tsx @@ -55,9 +55,7 @@ import { import { pluginCollector } from '../plugins/collectors'; import { featureFlagCollector, - routeObjectCollector, - routeParentCollector, - routePathCollector, + routingV1Collector, } from '../routing/collectors'; import { RoutingProvider } from '../routing/RoutingProvider'; import { RouteTracker } from '../routing/RouteTracker'; @@ -205,20 +203,12 @@ export class AppManager implements BackstageApp { [], ); - const { - routePaths, - routeParents, - routeObjects, - featureFlags, - routeBindings, - } = useMemo(() => { + const { routing, featureFlags, routeBindings } = useMemo(() => { const result = traverseElementTree({ root: children, discoverers: [childDiscoverer, routeElementDiscoverer], collectors: { - routePaths: routePathCollector, - routeParents: routeParentCollector, - routeObjects: routeObjectCollector, + routing: routingV1Collector, collectedPlugins: pluginCollector, featureFlags: featureFlagCollector, }, @@ -241,7 +231,7 @@ export class AppManager implements BackstageApp { if (!routesHaveBeenValidated) { routesHaveBeenValidated = true; - validateRouteParameters(routePaths, routeParents); + validateRouteParameters(routing.paths, routing.parents); validateRouteBindings( routeBindings, this.plugins as Iterable>, @@ -304,9 +294,9 @@ export class AppManager implements BackstageApp { diff --git a/packages/core-app-api/src/routing/RouteTracker.tsx b/packages/core-app-api/src/routing/RouteTracker.tsx index b745dd9235..40a9d7ec60 100644 --- a/packages/core-app-api/src/routing/RouteTracker.tsx +++ b/packages/core-app-api/src/routing/RouteTracker.tsx @@ -22,7 +22,7 @@ import { CommonAnalyticsContext, RouteRef, } from '@backstage/core-plugin-api'; -import { routeObjectCollector } from './collectors'; +import { routingV1Collector } from './collectors'; import { childDiscoverer, routeElementDiscoverer, @@ -101,18 +101,20 @@ export const RouteTracker = ({ tree }: { tree: React.ReactNode }) => { const { pathname, search, hash } = useLocation(); // todo(iamEAP): Work this into the existing traversal and make the data // available on the provider. Then grab from app instance on the router. - const { routeObjects } = useMemo(() => { + const { routing } = useMemo(() => { return traverseElementTree({ root: tree, discoverers: [childDiscoverer, routeElementDiscoverer], collectors: { - routeObjects: routeObjectCollector, + routing: routingV1Collector, }, }); }, [tree]); return ( - + ); diff --git a/packages/core-app-api/src/routing/RoutingProvider.test.tsx b/packages/core-app-api/src/routing/RoutingProvider.test.tsx index 3391bab8b0..dec92072fc 100644 --- a/packages/core-app-api/src/routing/RoutingProvider.test.tsx +++ b/packages/core-app-api/src/routing/RoutingProvider.test.tsx @@ -34,11 +34,7 @@ import { ExternalRouteRef, } from '@backstage/core-plugin-api'; import { RoutingProvider } from './RoutingProvider'; -import { - routePathCollector, - routeParentCollector, - routeObjectCollector, -} from './collectors'; +import { routingV1Collector } from './collectors'; import { validateRouteParameters } from './validation'; import { RouteResolver } from './RouteResolver'; import { AnyRouteRef, RouteFunc } from './types'; @@ -135,21 +131,19 @@ function withRoutingProvider( root: ReactElement, routeBindings: [ExternalRouteRef, RouteRef][] = [], ) { - const { routePaths, routeParents, routeObjects } = traverseElementTree({ + const { routing } = traverseElementTree({ root, discoverers: [childDiscoverer, routeElementDiscoverer], collectors: { - routePaths: routePathCollector, - routeParents: routeParentCollector, - routeObjects: routeObjectCollector, + routing: routingV1Collector, }, }); return ( @@ -314,18 +308,17 @@ describe('discovery', () => { ); - const { routePaths, routeParents } = traverseElementTree({ + const { routing } = traverseElementTree({ root, discoverers: [childDiscoverer, routeElementDiscoverer], collectors: { - routePaths: routePathCollector, - routeParents: routeParentCollector, + routing: routingV1Collector, }, }); - expect(() => validateRouteParameters(routePaths, routeParents)).toThrow( - 'Parameter :id is duplicated in path /foo/:id/bar/:id', - ); + expect(() => + validateRouteParameters(routing.paths, routing.parents), + ).toThrow('Parameter :id is duplicated in path /foo/:id/bar/:id'); }); }); diff --git a/packages/core-app-api/src/routing/collectors.test.tsx b/packages/core-app-api/src/routing/collectors.test.tsx index 5fb1795f4c..1c5bc8c639 100644 --- a/packages/core-app-api/src/routing/collectors.test.tsx +++ b/packages/core-app-api/src/routing/collectors.test.tsx @@ -15,11 +15,7 @@ */ import React, { PropsWithChildren } from 'react'; -import { - routePathCollector, - routeParentCollector, - routeObjectCollector, -} from './collectors'; +import { routingV1Collector } from './collectors'; import { traverseElementTree, @@ -162,30 +158,28 @@ describe('discovery', () => { ); - const { routes, routeParents, routeObjects } = traverseElementTree({ + const { routing } = traverseElementTree({ root, discoverers: [childDiscoverer, routeElementDiscoverer], collectors: { - routes: routePathCollector, - routeParents: routeParentCollector, - routeObjects: routeObjectCollector, + routing: routingV1Collector, }, }); - expect(sortedEntries(routes)).toEqual([ + expect(sortedEntries(routing.paths)).toEqual([ [ref1, '/foo'], [ref2, '/bar/:id'], [ref3, '/baz'], [ref4, '/divsoup'], [ref5, '/blop'], ]); - expect(sortedEntries(routeParents)).toEqual([ + expect(sortedEntries(routing.parents)).toEqual([ [ref1, undefined], [ref2, ref1], [ref3, ref2], [ref4, undefined], [ref5, ref1], ]); - expect(routeObjects).toEqual([ + expect(routing.objects).toEqual([ routeObj( '/foo', [ref1], @@ -220,22 +214,21 @@ describe('discovery', () => { ); - const { routes, routeParents } = traverseElementTree({ + const { routing } = traverseElementTree({ root, discoverers: [childDiscoverer, routeElementDiscoverer], collectors: { - routes: routePathCollector, - routeParents: routeParentCollector, + routing: routingV1Collector, }, }); - expect(sortedEntries(routes)).toEqual([ + expect(sortedEntries(routing.paths)).toEqual([ [ref1, '/foo'], [ref2, '/bar/:id'], [ref3, '/baz'], [ref4, '/divsoup'], [ref5, '/blop'], ]); - expect(sortedEntries(routeParents)).toEqual([ + expect(sortedEntries(routing.parents)).toEqual([ [ref1, undefined], [ref2, ref1], [ref3, undefined], @@ -266,30 +259,28 @@ describe('discovery', () => { ); - const { routes, routeParents, routeObjects } = traverseElementTree({ + const { routing } = traverseElementTree({ root, discoverers: [childDiscoverer, routeElementDiscoverer], collectors: { - routes: routePathCollector, - routeParents: routeParentCollector, - routeObjects: routeObjectCollector, + routing: routingV1Collector, }, }); - expect(sortedEntries(routes)).toEqual([ + expect(sortedEntries(routing.paths)).toEqual([ [ref1, '/foo'], [ref2, '/foo'], [ref3, '/bar'], [ref4, '/baz'], [ref5, '/baz'], ]); - expect(sortedEntries(routeParents)).toEqual([ + expect(sortedEntries(routing.parents)).toEqual([ [ref1, undefined], [ref2, undefined], [ref3, undefined], [ref4, ref3], [ref5, ref3], ]); - expect(routeObjects).toEqual([ + expect(routing.objects).toEqual([ routeObj('/foo', [ref1, ref2], [], 'gathered'), routeObj( '/bar', @@ -317,30 +308,28 @@ describe('discovery', () => { ); - const { routes, routeParents, routeObjects } = traverseElementTree({ + const { routing } = traverseElementTree({ root, discoverers: [childDiscoverer, routeElementDiscoverer], collectors: { - routes: routePathCollector, - routeParents: routeParentCollector, - routeObjects: routeObjectCollector, + routing: routingV1Collector, }, }); - expect(sortedEntries(routes)).toEqual([ + expect(sortedEntries(routing.paths)).toEqual([ [ref1, '/foo'], [ref2, '/bar'], [ref3, '/baz'], [ref4, '/blop'], [ref5, '/bar'], ]); - expect(sortedEntries(routeParents)).toEqual([ + expect(sortedEntries(routing.parents)).toEqual([ [ref1, undefined], [ref2, ref1], [ref3, ref1], [ref4, ref3], [ref5, ref1], ]); - expect(routeObjects).toEqual([ + expect(routing.objects).toEqual([ routeObj( '/foo', [ref1], @@ -376,8 +365,7 @@ describe('discovery', () => { root, discoverers: [childDiscoverer, routeElementDiscoverer], collectors: { - routes: routePathCollector, - routeParents: routeParentCollector, + routing: routingV1Collector, }, }); }).toThrow('Mounted routable extension must have a path'); diff --git a/packages/core-app-api/src/routing/collectors.tsx b/packages/core-app-api/src/routing/collectors.tsx index 46734707a6..fcd954201a 100644 --- a/packages/core-app-api/src/routing/collectors.tsx +++ b/packages/core-app-api/src/routing/collectors.tsx @@ -14,106 +14,16 @@ * limitations under the License. */ -import { isValidElement, ReactElement, ReactNode } from 'react'; import { RouteRef, getComponentData, BackstagePlugin, } from '@backstage/core-plugin-api'; +import { isValidElement } from 'react'; import { BackstageRouteObject } from './types'; import { createCollector } from '../extensions/traversal'; import { FeatureFlagged, FeatureFlaggedProps } from './FeatureFlagged'; -function getMountPoint(node: ReactElement): RouteRef | undefined { - const element: ReactNode = node.props?.element; - - let routeRef = getComponentData(node, 'core.mountPoint'); - if (!routeRef && isValidElement(element)) { - routeRef = getComponentData(element, 'core.mountPoint'); - } - - return routeRef; -} - -export const routePathCollector = createCollector( - () => new Map(), - (acc, node, parent, ctxPath: string | undefined) => { - // The context path is used during mount point gathering to assign the same path - // to all discovered mount points - let currentCtxPath = ctxPath; - - if (parent?.props.element === node) { - return currentCtxPath; - } - - // Start gathering mount points when we encounter a mount point gathering flag - if (getComponentData(node, 'core.gatherMountPoints')) { - const path: string | undefined = node.props?.path; - if (!path) { - throw new Error('Mount point gatherer must have a path'); - } - currentCtxPath = path; - } - - const routeRef = getMountPoint(node); - if (routeRef) { - let path: string | undefined = node.props?.path; - // If we're gathering mount points we use the context path as out path, unless - // the element has its own path, in which case we use that instead and stop gathering - if (currentCtxPath) { - if (path) { - currentCtxPath = undefined; - } else { - path = currentCtxPath; - } - } - if (!path) { - throw new Error('Mounted routable extension must have a path'); - } - acc.set(routeRef, path); - } - return currentCtxPath; - }, -); - -export const routeParentCollector = createCollector( - () => new Map(), - (acc, node, parent, parentRouteRef?: RouteRef | { sticky: RouteRef }) => { - if (parent?.props.element === node) { - return parentRouteRef; - } - - let nextParent = parentRouteRef; - - const routeRef = getMountPoint(node); - if (routeRef) { - // "sticky" route ref is when we've encountered a mount point gatherer, and we want a - // mount points beneath it to have the same parent, regardless of internal structure - if (parentRouteRef && 'sticky' in parentRouteRef) { - acc.set(routeRef, parentRouteRef.sticky); - - // When we encounter a mount point with an explicit path, we stop gathering - // mount points within the children and remove the sticky state - if (node.props?.path) { - nextParent = routeRef; - } else { - nextParent = parentRouteRef; - } - } else { - acc.set(routeRef, parentRouteRef); - nextParent = routeRef; - } - } - - // Mount point gatherers are marked as "sticky" - if (getComponentData(node, 'core.gatherMountPoints')) { - return { sticky: nextParent }; - } - - return nextParent; - }, -); - // We always add a child that matches all subroutes but without any route refs. This makes // sure that we're always able to match each route no matter how deep the navigation goes. // The route resolver then takes care of selecting the most specific match in order to find @@ -125,21 +35,107 @@ export const MATCH_ALL_ROUTE: BackstageRouteObject = { routeRefs: new Set(), }; -export const routeObjectCollector = createCollector( - () => Array(), - (acc, node, parent, parentObj: BackstageRouteObject | undefined) => { - const parentChildren = parentObj?.children ?? acc; +interface RoutingV1CollectorContext { + path?: string; + routeRef?: RouteRef; + obj?: BackstageRouteObject; + sticky?: boolean; +} + +/** + * This is the old V1 logic for collecting the routing model. + * It is being replaced by a new collector because this collection + * logic does not work well beyond react-router v6 beta. + * + * The breaking change is that react-router now requires route + * elements to be `Route` components, and directly renders the + * element prop rather than the `Route` itself. This means it is + * no longer possible to create utility route components. In order + * to fill this gap and in general simplify the route collection + * logic, a new route collection logic is created. + * + * @internal + */ +export const routingV1Collector = createCollector( + () => ({ + paths: new Map(), + parents: new Map(), + objects: new Array(), + }), + (acc, node, parent, ctx?: RoutingV1CollectorContext) => { + // Ignore the top-level element within element props, since it's already been collected. if (parent?.props.element === node) { - return parentObj; + return ctx; } + let currentObj = ctx?.obj; + let currentParentRouteRef = ctx?.routeRef; + let sticky = ctx?.sticky; + const path: string | undefined = node.props?.path; + const parentChildren = currentObj?.children ?? acc.objects; const caseSensitive: boolean = Boolean(node.props?.caseSensitive); - const routeRef = getMountPoint(node); + // The context path is used during mount point gathering to assign the same path + // to all discovered mount points + let currentCtxPath = ctx?.path; + + // Start gathering mount points when we encounter a mount point gathering flag + if (getComponentData(node, 'core.gatherMountPoints')) { + if (!path) { + throw new Error('Mount point gatherer must have a path'); + } + currentCtxPath = path; + } + + // Route refs are discovered on the element itself, and on the top-level + // element within the element prop if it exists. + const element = node.props?.element; + let routeRef = getComponentData(node, 'core.mountPoint'); + if (!routeRef && isValidElement(element)) { + routeRef = getComponentData(element, 'core.mountPoint'); + } + if (routeRef) { + // First the path gathering + + let routePath: string | undefined = path; + // If we're gathering mount points we use the context path as out path, unless + // the element has its own path, in which case we use that instead and stop gathering + if (currentCtxPath) { + if (routePath) { + currentCtxPath = undefined; + } else { + routePath = currentCtxPath; + } + } + if (!routePath) { + throw new Error('Mounted routable extension must have a path'); + } + acc.paths.set(routeRef, routePath); + + // Then the parent gathering + + // "sticky" route ref is when we've encountered a mount point gatherer, and we want a + // mount points beneath it to have the same parent, regardless of internal structure + if (currentParentRouteRef && sticky) { + acc.parents.set(routeRef, currentParentRouteRef); + + // When we encounter a mount point with an explicit path, we stop gathering + // mount points within the children and remove the sticky state + if (path) { + currentParentRouteRef = routeRef; + sticky = false; + } + } else { + acc.parents.set(routeRef, currentParentRouteRef); + currentParentRouteRef = routeRef; + } + + // Then construct the objects + if (path) { - const newObject: BackstageRouteObject = { + currentObj = { caseSensitive, path, element: 'mounted', @@ -150,11 +146,14 @@ export const routeObjectCollector = createCollector( 'core.plugin', ), }; - parentChildren.push(newObject); - return newObject; + parentChildren.push(currentObj); + } else { + currentObj?.routeRefs.add(routeRef); } + } - parentObj?.routeRefs.add(routeRef); + if (getComponentData(node, 'core.gatherMountPoints')) { + sticky = true; } const isGatherer = getComponentData( @@ -165,19 +164,25 @@ export const routeObjectCollector = createCollector( if (!path) { throw new Error('Mount point gatherer must have a path'); } - const newObject: BackstageRouteObject = { - caseSensitive, - path, - element: 'gathered', - routeRefs: new Set(), - children: [MATCH_ALL_ROUTE], - plugin: parentObj?.plugin, - }; - parentChildren.push(newObject); - return newObject; + if (!routeRef) { + currentObj = { + caseSensitive, + path, + element: 'gathered', + routeRefs: new Set(), + children: [MATCH_ALL_ROUTE], + plugin: ctx?.obj?.plugin, + }; + parentChildren.push(currentObj); + } } - return parentObj; + return { + obj: currentObj, + path: currentCtxPath, + routeRef: currentParentRouteRef, + sticky, + }; }, ); From 27e09c01eab062a808b747a096cd3e97f6414c55 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Wed, 6 Apr 2022 13:01:31 +0200 Subject: [PATCH 2/3] core-app-api: refactor RouteTracker integration Signed-off-by: Patrik Oldsberg --- packages/core-app-api/src/app/AppManager.tsx | 18 ++++++++++-- .../core-app-api/src/routing/RouteTracker.tsx | 29 +++++-------------- 2 files changed, 22 insertions(+), 25 deletions(-) diff --git a/packages/core-app-api/src/app/AppManager.tsx b/packages/core-app-api/src/app/AppManager.tsx index 77a1ecc7cf..75bab5c2df 100644 --- a/packages/core-app-api/src/app/AppManager.tsx +++ b/packages/core-app-api/src/app/AppManager.tsx @@ -17,8 +17,10 @@ import { Config } from '@backstage/config'; import React, { ComponentType, + createContext, PropsWithChildren, ReactElement, + useContext, useEffect, useMemo, useState, @@ -77,6 +79,7 @@ import { AppThemeProvider } from './AppThemeProvider'; import { defaultConfigLoader } from './defaultConfigLoader'; import { ApiRegistry } from '../apis/system/ApiRegistry'; import { resolveRouteBindings } from './resolveRouteBindings'; +import { BackstageRouteObject } from '../routing/types'; type CompatiblePlugin = | BackstagePlugin @@ -84,6 +87,10 @@ type CompatiblePlugin = output(): Array<{ type: 'feature-flag'; name: string }>; }); +const InternalAppContext = createContext<{ + routeObjects: BackstageRouteObject[]; +}>({ routeObjects: [] }); + /** * Get the app base path from the configured app baseUrl. * @@ -300,7 +307,11 @@ export class AppManager implements BackstageApp { routeBindings={routeBindings} basePath={getBasePath(loadedConfig.api)} > - {children} + + {children} + @@ -335,6 +346,7 @@ export class AppManager implements BackstageApp { const AppRouter = ({ children }: PropsWithChildren<{}>) => { const configApi = useApi(configApiRef); const mountPath = `${getBasePath(configApi)}/*`; + const { routeObjects } = useContext(InternalAppContext); // If the app hasn't configured a sign-in page, we just continue as guest. if (!SignInPageComponent) { @@ -360,7 +372,7 @@ export class AppManager implements BackstageApp { return ( - + {children}} /> @@ -370,7 +382,7 @@ export class AppManager implements BackstageApp { return ( - + {children}} /> diff --git a/packages/core-app-api/src/routing/RouteTracker.tsx b/packages/core-app-api/src/routing/RouteTracker.tsx index 40a9d7ec60..f9d8c78fc0 100644 --- a/packages/core-app-api/src/routing/RouteTracker.tsx +++ b/packages/core-app-api/src/routing/RouteTracker.tsx @@ -14,7 +14,7 @@ * limitations under the License. */ -import React, { useEffect, useMemo } from 'react'; +import React, { useEffect } from 'react'; import { matchRoutes, useLocation } from 'react-router-dom'; import { useAnalytics, @@ -22,12 +22,6 @@ import { CommonAnalyticsContext, RouteRef, } from '@backstage/core-plugin-api'; -import { routingV1Collector } from './collectors'; -import { - childDiscoverer, - routeElementDiscoverer, - traverseElementTree, -} from '../extensions/traversal'; import { BackstageRouteObject } from './types'; /** @@ -97,24 +91,15 @@ const TrackNavigation = ({ * Logs a "navigate" event with appropriate plugin-level analytics context * attributes each time the user navigates to a page. */ -export const RouteTracker = ({ tree }: { tree: React.ReactNode }) => { +export const RouteTracker = ({ + routeObjects, +}: { + routeObjects: BackstageRouteObject[]; +}) => { const { pathname, search, hash } = useLocation(); - // todo(iamEAP): Work this into the existing traversal and make the data - // available on the provider. Then grab from app instance on the router. - const { routing } = useMemo(() => { - return traverseElementTree({ - root: tree, - discoverers: [childDiscoverer, routeElementDiscoverer], - collectors: { - routing: routingV1Collector, - }, - }); - }, [tree]); return ( - + ); From 3ff2bfb66ee0379929cd553cfa037c45f80ac731 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Wed, 6 Apr 2022 13:03:27 +0200 Subject: [PATCH 3/3] changesets: added changeset for core-app-api route collection refactoring Signed-off-by: Patrik Oldsberg --- .changeset/eight-hounds-dream.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/eight-hounds-dream.md diff --git a/.changeset/eight-hounds-dream.md b/.changeset/eight-hounds-dream.md new file mode 100644 index 0000000000..4368113f03 --- /dev/null +++ b/.changeset/eight-hounds-dream.md @@ -0,0 +1,5 @@ +--- +'@backstage/core-app-api': patch +--- + +Refactored the route collection logic to prepare for future changes and avoid duplicate element tree traversal for the analytics context.