From e2aa76f19b4dcdc673cfe343011587faa3e51fc8 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Wed, 2 Dec 2020 11:43:30 +0100 Subject: [PATCH 01/14] core-api: initial implementation of external route refs Co-authored-by: blam --- packages/core-api/src/routing/RouteRef.ts | 10 ++++++ packages/core-api/src/routing/hooks.test.tsx | 37 +++++++++++++++++--- packages/core-api/src/routing/hooks.tsx | 19 ++++++++-- 3 files changed, 59 insertions(+), 7 deletions(-) diff --git a/packages/core-api/src/routing/RouteRef.ts b/packages/core-api/src/routing/RouteRef.ts index 0fbc57c6f1..c7db48045a 100644 --- a/packages/core-api/src/routing/RouteRef.ts +++ b/packages/core-api/src/routing/RouteRef.ts @@ -53,3 +53,13 @@ export function createRouteRef< >(config: RouteRefConfig): RouteRef { return new AbsoluteRouteRef(config); } + +export class ExternalRouteRef { + toString() { + return `externalRouteRef{}`; + } +} + +export function createExternalRouteRef(): ExternalRouteRef { + return new ExternalRouteRef(); +} diff --git a/packages/core-api/src/routing/hooks.test.tsx b/packages/core-api/src/routing/hooks.test.tsx index ae1053b58d..6bc80b7c08 100644 --- a/packages/core-api/src/routing/hooks.test.tsx +++ b/packages/core-api/src/routing/hooks.test.tsx @@ -35,7 +35,11 @@ import { validateRoutes, RouteFunc, } from './hooks'; -import { createRouteRef } from './RouteRef'; +import { + createRouteRef, + createExternalRouteRef, + ExternalRouteRef, +} from './RouteRef'; import { RouteRef, RouteRefConfig } from './types'; const mockConfig = (extra?: Partial>) => ({ @@ -52,10 +56,13 @@ const ref2 = createRouteRef(mockConfig({ path: '/wat2' })); const ref3 = createRouteRef(mockConfig({ path: '/wat3' })); const ref4 = createRouteRef(mockConfig({ path: '/wat4' })); const ref5 = createRouteRef(mockConfig({ path: '/wat5' })); +const eRefA = createExternalRouteRef(); +const eRefB = createExternalRouteRef(); +const eRefC = createExternalRouteRef(); const MockRouteSource = (props: { name: string; - routeRef: RouteRef; + routeRef: RouteRef | ExternalRouteRef; params?: T; }) => { try { @@ -90,7 +97,10 @@ const Extension5 = plugin.provide( createRoutableExtension({ component: MockComponent, mountPoint: ref5 }), ); -function withRoutingProvider(root: ReactElement) { +function withRoutingProvider( + root: ReactElement, + routeBindings: [ExternalRouteRef, RouteRef][] = [], +) { const { routePaths, routeParents, routeObjects } = traverseElementTree({ root, discoverers: [childDiscoverer, routeElementDiscoverer], @@ -106,6 +116,7 @@ function withRoutingProvider(root: ReactElement) { routePaths={routePaths} routeParents={routeParents} routeObjects={routeObjects} + routeBindings={new Map(routeBindings)} > {root} @@ -119,17 +130,35 @@ describe('discovery', () => { + + + ); - const rendered = render(withRoutingProvider(root)); + const rendered = render( + withRoutingProvider(root, [ + [eRefA, ref3], + [eRefB, ref1], + [eRefC, ref2], + ]), + ); expect(rendered.getByText('Path at inside: /foo/bar')).toBeInTheDocument(); + expect( + rendered.getByText('Path at insideExternal: /baz'), + ).toBeInTheDocument(); expect(rendered.getByText('Path at outside: /foo/bar')).toBeInTheDocument(); + expect( + rendered.getByText('Path at outsideExternal1: /foo'), + ).toBeInTheDocument(); + expect( + rendered.getByText('Path at outsideExternal2: /foo/bar'), + ).toBeInTheDocument(); }); it('should handle routeRefs with parameters', () => { diff --git a/packages/core-api/src/routing/hooks.tsx b/packages/core-api/src/routing/hooks.tsx index 563bc84a76..55fde89de2 100644 --- a/packages/core-api/src/routing/hooks.tsx +++ b/packages/core-api/src/routing/hooks.tsx @@ -17,6 +17,7 @@ import React, { createContext, ReactNode, useContext, useMemo } from 'react'; import { AnyRouteRef, BackstageRouteObject, RouteRef } from './types'; import { generatePath, matchRoutes, useLocation } from 'react-router-dom'; +import { ExternalRouteRef } 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. @@ -34,12 +35,17 @@ class RouteResolver { private readonly routePaths: Map, private readonly routeParents: Map, private readonly routeObjects: BackstageRouteObject[], + private readonly routeBindings: Map, ) {} resolve( - routeRef: RouteRef, + routeRefOrExternalRouteRef: RouteRef | ExternalRouteRef, sourceLocation: ReturnType, ): RouteFunc { + const routeRef = + this.routeBindings.get(routeRefOrExternalRouteRef) ?? + (routeRefOrExternalRouteRef as RouteRef); + const match = matchRoutes(this.routeObjects, sourceLocation) ?? []; const lastPath = this.routePaths.get(routeRef); @@ -106,7 +112,7 @@ class RouteResolver { const RoutingContext = createContext(undefined); export function useRouteRef( - routeRef: RouteRef, + routeRef: RouteRef | ExternalRouteRef, ): RouteFunc { const sourceLocation = useLocation(); const resolver = useContext(RoutingContext); @@ -126,6 +132,7 @@ type ProviderProps = { routePaths: Map; routeParents: Map; routeObjects: BackstageRouteObject[]; + routeBindings: Map; children: ReactNode; }; @@ -133,9 +140,15 @@ export const RoutingProvider = ({ routePaths, routeParents, routeObjects, + routeBindings, children, }: ProviderProps) => { - const resolver = new RouteResolver(routePaths, routeParents, routeObjects); + const resolver = new RouteResolver( + routePaths, + routeParents, + routeObjects, + routeBindings, + ); return ( {children} From c78bc66cfba1954a27a46fcf5e680116b12eb7b5 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Wed, 2 Dec 2020 18:31:59 +0100 Subject: [PATCH 02/14] core-api: add routes and externalRoutes to plugin interface Co-authored-by: blam --- packages/core-api/src/app/App.tsx | 6 ++--- packages/core-api/src/app/types.ts | 4 ++-- .../core-api/src/extensions/extensions.tsx | 2 +- packages/core-api/src/plugin/Plugin.tsx | 24 ++++++++++++++++--- packages/core-api/src/plugin/collectors.ts | 7 ++++-- packages/core-api/src/plugin/types.ts | 21 +++++++++++++--- 6 files changed, 50 insertions(+), 14 deletions(-) diff --git a/packages/core-api/src/app/App.tsx b/packages/core-api/src/app/App.tsx index 3ca91966bf..b42278e4a6 100644 --- a/packages/core-api/src/app/App.tsx +++ b/packages/core-api/src/app/App.tsx @@ -59,7 +59,7 @@ import { ApiResolver, ApiFactoryRegistry } from '../apis/system'; type FullAppOptions = { apis: Iterable; icons: SystemIcons; - plugins: BackstagePlugin[]; + plugins: BackstagePlugin[]; components: AppComponents; themes: AppTheme[]; configLoader?: AppConfigLoader; @@ -107,7 +107,7 @@ export class PrivateAppImpl implements BackstageApp { private readonly apis: Iterable; private readonly icons: SystemIcons; - private readonly plugins: BackstagePlugin[]; + private readonly plugins: BackstagePlugin[]; private readonly components: AppComponents; private readonly themes: AppTheme[]; private readonly configLoader?: AppConfigLoader; @@ -125,7 +125,7 @@ export class PrivateAppImpl implements BackstageApp { this.defaultApis = options.defaultApis; } - getPlugins(): BackstagePlugin[] { + getPlugins(): BackstagePlugin[] { return this.plugins; } diff --git a/packages/core-api/src/app/types.ts b/packages/core-api/src/app/types.ts index 8a1eb92e8e..aa432bd005 100644 --- a/packages/core-api/src/app/types.ts +++ b/packages/core-api/src/app/types.ts @@ -93,7 +93,7 @@ export type AppOptions = { /** * A list of all plugins to include in the app. */ - plugins?: BackstagePlugin[]; + plugins?: BackstagePlugin[]; /** * Supply components to the app to override the default ones. @@ -140,7 +140,7 @@ export type BackstageApp = { /** * Returns all plugins registered for the app. */ - getPlugins(): BackstagePlugin[]; + getPlugins(): BackstagePlugin[]; /** * Get a common icon for this app. diff --git a/packages/core-api/src/extensions/extensions.tsx b/packages/core-api/src/extensions/extensions.tsx index 4087d5f963..079fec4597 100644 --- a/packages/core-api/src/extensions/extensions.tsx +++ b/packages/core-api/src/extensions/extensions.tsx @@ -54,7 +54,7 @@ export function createReactExtension(options: { }): Extension> { const { component: Component, data = {} } = options; return { - expose(plugin: BackstagePlugin): NamedExoticComponent { + expose(plugin: BackstagePlugin): NamedExoticComponent { const Result = (props: Props) => ; attachComponentData(Result, 'core.plugin', plugin); diff --git a/packages/core-api/src/plugin/Plugin.tsx b/packages/core-api/src/plugin/Plugin.tsx index f477674f4d..797dcd852e 100644 --- a/packages/core-api/src/plugin/Plugin.tsx +++ b/packages/core-api/src/plugin/Plugin.tsx @@ -19,13 +19,18 @@ import { PluginOutput, BackstagePlugin, Extension, + AnyRoutes, + AnyExternalRoutes, } from './types'; import { AnyApiFactory } from '../apis'; -export class PluginImpl implements BackstagePlugin { +export class PluginImpl< + Routes extends AnyRoutes, + ExternalRoutes extends AnyExternalRoutes +> implements BackstagePlugin { private storedOutput?: PluginOutput[]; - constructor(private readonly config: PluginConfig) {} + constructor(private readonly config: PluginConfig) {} getId(): string { return this.config.id; @@ -35,6 +40,14 @@ export class PluginImpl implements BackstagePlugin { return this.config.apis ?? []; } + get routes(): Routes { + return this.config.routes ?? ({} as Routes); + } + + get externalRoutes(): ExternalRoutes { + return this.config.externalRoutes ?? ({} as ExternalRoutes); + } + output(): PluginOutput[] { if (this.storedOutput) { return this.storedOutput; @@ -79,6 +92,11 @@ export class PluginImpl implements BackstagePlugin { } } -export function createPlugin(config: PluginConfig): BackstagePlugin { +export function createPlugin< + Routes extends AnyRoutes, + ExternalRoutes extends AnyExternalRoutes +>( + config: PluginConfig, +): BackstagePlugin { return new PluginImpl(config); } diff --git a/packages/core-api/src/plugin/collectors.ts b/packages/core-api/src/plugin/collectors.ts index 3eadfc0185..b04c7b41c4 100644 --- a/packages/core-api/src/plugin/collectors.ts +++ b/packages/core-api/src/plugin/collectors.ts @@ -34,9 +34,12 @@ import { getComponentData } from '../extensions'; import { createCollector } from '../extensions/traversal'; export const pluginCollector = createCollector( - () => new Set(), + () => new Set>(), (acc, node) => { - const plugin = getComponentData(node, 'core.plugin'); + const plugin = getComponentData>( + node, + 'core.plugin', + ); if (plugin) { acc.add(plugin); } diff --git a/packages/core-api/src/plugin/types.ts b/packages/core-api/src/plugin/types.ts index dd19948a0a..e3f2e38972 100644 --- a/packages/core-api/src/plugin/types.ts +++ b/packages/core-api/src/plugin/types.ts @@ -17,6 +17,7 @@ import { ComponentType } from 'react'; import { RouteRef } from '../routing'; import { AnyApiFactory } from '../apis/system'; +import { ExternalRouteRef } from '../routing/RouteRef'; export type RouteOptions = { // Whether the route path must match exactly, defaults to true. @@ -67,20 +68,34 @@ export type PluginOutput = | FeatureFlagOutput; export type Extension = { - expose(plugin: BackstagePlugin): T; + expose(plugin: BackstagePlugin): T; }; -export type BackstagePlugin = { +export type AnyRoutes = { [name: string]: RouteRef }; + +export type AnyExternalRoutes = { [name: string]: ExternalRouteRef }; + +export type BackstagePlugin< + Routes extends AnyRoutes, + ExternalRoutes extends AnyExternalRoutes +> = { getId(): string; output(): PluginOutput[]; getApis(): Iterable; provide(extension: Extension): T; + routes: Routes; + externalRoutes: ExternalRoutes; }; -export type PluginConfig = { +export type PluginConfig< + Routes extends AnyRoutes, + ExternalRoutes extends AnyExternalRoutes +> = { id: string; apis?: Iterable; register?(hooks: PluginHooks): void; + routes?: Routes; + externalRoutes?: ExternalRoutes; }; export type PluginHooks = { From ad5eab923cb1cb2aa7b00e121e5664aa38064f4e Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Thu, 3 Dec 2020 10:43:20 +0100 Subject: [PATCH 03/14] core-api: add unused optional bindRoutes method to AppOptions --- packages/core-api/src/app/types.ts | 28 +++++++++++++++++++++++++++- 1 file changed, 27 insertions(+), 1 deletion(-) diff --git a/packages/core-api/src/app/types.ts b/packages/core-api/src/app/types.ts index aa432bd005..d121a2e2a0 100644 --- a/packages/core-api/src/app/types.ts +++ b/packages/core-api/src/app/types.ts @@ -16,7 +16,8 @@ import { ComponentType } from 'react'; import { IconComponent, SystemIconKey, SystemIcons } from '../icons'; -import { BackstagePlugin } from '../plugin'; +import { BackstagePlugin, AnyExternalRoutes } from '../plugin'; +import { RouteRef } from '../routing'; import { AnyApiFactory } from '../apis'; import { AppTheme, ProfileInfo } from '../apis/definitions'; import { AppConfig } from '@backstage/config'; @@ -78,6 +79,11 @@ export type AppComponents = { */ export type AppConfigLoader = () => Promise; +type BindRouteFunc = ( + externalRoutes: T, + targetRoutes: { [key in keyof T]: RouteRef }, +) => void; + export type AppOptions = { /** * A collection of ApiFactories to register in the application to either @@ -134,6 +140,26 @@ export type AppOptions = { * that was packaged by the backstage-cli and default docker container boot script. */ configLoader?: AppConfigLoader; + + /** + * A function that is used to register associations between cross-plugin route + * references, enabling plugins to navigate between each other. + * + * The `bind` function that is passed in should be used to bind all external + * routes of all used plugins. + * + * ```ts + * bindRoutes({ bind }) { + * bind(docsPlugin.externalRoutes, { + * homePage: managePlugin.routes.managePage, + * }) + * bind(homePagePlugin.externalRoutes, { + * settingsPage: settingsPlugin.routes.settingsPage, + * }) + * } + * ``` + */ + bindRoutes?(context: { bind: BindRouteFunc }): void; }; export type BackstageApp = { From 5d52e20a44f5b9d79e2af9fccd5dd6a34f948222 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fredrik=20Adel=C3=B6w?= Date: Mon, 7 Dec 2020 15:59:37 +0100 Subject: [PATCH 04/14] core-api: run bindRoutes in the app impl and inject provider --- packages/core-api/src/app/App.test.ts | 40 ++++++ packages/core-api/src/app/App.tsx | 115 +++++++++++++----- packages/core-api/src/app/types.ts | 2 +- packages/core/src/api-wrappers/createApp.tsx | 1 + .../test-utils/src/testUtils/appWrappers.tsx | 1 + 5 files changed, 130 insertions(+), 29 deletions(-) create mode 100644 packages/core-api/src/app/App.test.ts diff --git a/packages/core-api/src/app/App.test.ts b/packages/core-api/src/app/App.test.ts new file mode 100644 index 0000000000..a25256491b --- /dev/null +++ b/packages/core-api/src/app/App.test.ts @@ -0,0 +1,40 @@ +/* + * 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 { createExternalRouteRef, createRouteRef } from '../routing/RouteRef'; +import { generateBoundRoutes } from './App'; + +describe('generateBoundRoutes', () => { + it('runs happy path', () => { + const external = { myRoute: createExternalRouteRef() }; + const ref = createRouteRef({ path: '', title: '' }); + const result = generateBoundRoutes(({ bind }) => { + bind(external, { myRoute: ref }); + }); + + expect(result.get(external.myRoute)).toBe(ref); + }); + + it('throws on unknown keys', () => { + const external = { myRoute: createExternalRouteRef() }; + const ref = createRouteRef({ path: '', title: '' }); + expect(() => + generateBoundRoutes(({ bind }) => { + bind(external, { someOtherRoute: ref } as any); + }), + ).toThrow('Key someOtherRoute is not an existing external route'); + }); +}); diff --git a/packages/core-api/src/app/App.tsx b/packages/core-api/src/app/App.tsx index b42278e4a6..b7265fb5ac 100644 --- a/packages/core-api/src/app/App.tsx +++ b/packages/core-api/src/app/App.tsx @@ -15,46 +15,82 @@ */ import React, { ComponentType, + PropsWithChildren, + ReactElement, useMemo, useState, - ReactElement, - PropsWithChildren, } from 'react'; -import { Route, Routes, Navigate } from 'react-router-dom'; -import { AppContextProvider } from './AppContext'; -import { - BackstageApp, - AppComponents, - AppConfigLoader, - SignInResult, - SignInPageProps, -} from './types'; -import { BackstagePlugin } from '../plugin'; -import { - featureFlagsApiRef, - AppThemeApi, - ConfigApi, - identityApiRef, -} from '../apis/definitions'; -import { AppThemeProvider } from './AppThemeProvider'; - -import { IconComponent, SystemIcons, SystemIconKey } from '../icons'; +import { Navigate, Route, Routes } from 'react-router-dom'; +import { useAsync } from 'react-use'; import { + AnyApiFactory, + ApiHolder, ApiProvider, ApiRegistry, AppTheme, - AppThemeSelector, appThemeApiRef, + AppThemeSelector, configApiRef, ConfigReader, - useApi, - AnyApiFactory, - ApiHolder, LocalStorageFeatureFlags, + useApi, } from '../apis'; -import { useAsync } from 'react-use'; +import { + AppThemeApi, + ConfigApi, + featureFlagsApiRef, + identityApiRef, +} from '../apis/definitions'; +import { ApiFactoryRegistry, ApiResolver } from '../apis/system'; +import { + childDiscoverer, + routeElementDiscoverer, + traverseElementTree, +} from '../extensions/traversal'; +import { IconComponent, SystemIconKey, SystemIcons } from '../icons'; +import { BackstagePlugin } from '../plugin'; +import { RouteRef } from '../routing'; +import { + routeObjectCollector, + routeParentCollector, + routePathCollector, +} from '../routing/collectors'; +import { RoutingProvider } from '../routing/hooks'; +import { ExternalRouteRef } from '../routing/RouteRef'; +import { AppContextProvider } from './AppContext'; import { AppIdentity } from './AppIdentity'; -import { ApiResolver, ApiFactoryRegistry } from '../apis/system'; +import { AppThemeProvider } from './AppThemeProvider'; +import { + AppComponents, + AppConfigLoader, + AppOptions, + BackstageApp, + BindRouteFunc, + SignInPageProps, + SignInResult, +} from './types'; + +export function generateBoundRoutes( + bindRoutes: AppOptions['bindRoutes'], +): Map { + const result = new Map(); + + if (bindRoutes) { + const bind: BindRouteFunc = (externalRoutes, targetRoutes) => { + for (const [key, value] of Object.entries(targetRoutes)) { + const externalRoute = externalRoutes[key]; + if (!externalRoute) { + throw new Error(`Key ${key} is not an existing external route`); + } + + result.set(externalRoute, value); + } + }; + bindRoutes({ bind }); + } + + return result; +} type FullAppOptions = { apis: Iterable; @@ -64,6 +100,7 @@ type FullAppOptions = { themes: AppTheme[]; configLoader?: AppConfigLoader; defaultApis: Iterable; + bindRoutes?: AppOptions['bindRoutes']; }; function useConfigLoader( @@ -112,6 +149,7 @@ export class PrivateAppImpl implements BackstageApp { private readonly themes: AppTheme[]; private readonly configLoader?: AppConfigLoader; private readonly defaultApis: Iterable; + private readonly bindRoutes: AppOptions['bindRoutes']; private readonly identityApi = new AppIdentity(); @@ -123,6 +161,7 @@ export class PrivateAppImpl implements BackstageApp { this.themes = options.themes; this.configLoader = options.configLoader; this.defaultApis = options.defaultApis; + this.bindRoutes = options.bindRoutes; } getPlugins(): BackstagePlugin[] { @@ -202,6 +241,17 @@ export class PrivateAppImpl implements BackstageApp { [], ); + // Probably want to memo this? + const { routePaths, routeParents, routeObjects } = traverseElementTree({ + root: children, + discoverers: [childDiscoverer, routeElementDiscoverer], + collectors: { + routePaths: routePathCollector, + routeParents: routeParentCollector, + routeObjects: routeObjectCollector, + }, + }); + const loadedConfig = useConfigLoader( this.configLoader, this.components, @@ -218,7 +268,16 @@ export class PrivateAppImpl implements BackstageApp { return ( - {children} + + + {children} + + ); diff --git a/packages/core-api/src/app/types.ts b/packages/core-api/src/app/types.ts index d121a2e2a0..3eeac72a30 100644 --- a/packages/core-api/src/app/types.ts +++ b/packages/core-api/src/app/types.ts @@ -79,7 +79,7 @@ export type AppComponents = { */ export type AppConfigLoader = () => Promise; -type BindRouteFunc = ( +export type BindRouteFunc = ( externalRoutes: T, targetRoutes: { [key in keyof T]: RouteRef }, ) => void; diff --git a/packages/core/src/api-wrappers/createApp.tsx b/packages/core/src/api-wrappers/createApp.tsx index c99ba0ed03..9a8f7a36df 100644 --- a/packages/core/src/api-wrappers/createApp.tsx +++ b/packages/core/src/api-wrappers/createApp.tsx @@ -142,6 +142,7 @@ export function createApp(options?: AppOptions) { themes, configLoader, defaultApis, + bindRoutes: options?.bindRoutes, }); app.verify(); diff --git a/packages/test-utils/src/testUtils/appWrappers.tsx b/packages/test-utils/src/testUtils/appWrappers.tsx index 22bc8ceee4..1f6c7622b8 100644 --- a/packages/test-utils/src/testUtils/appWrappers.tsx +++ b/packages/test-utils/src/testUtils/appWrappers.tsx @@ -80,6 +80,7 @@ export function wrapInTestApp( }, ], defaultApis: mockApis, + bindRoutes: () => {}, }); let Wrapper: ComponentType; From 797d170873db7bd3df67471dcfd7a0cdf78104c1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fredrik=20Adel=C3=B6w?= Date: Mon, 7 Dec 2020 16:45:30 +0100 Subject: [PATCH 05/14] core-api: add full PrivateAppImpl test for bindRoutes --- packages/core-api/src/app/App.test.ts | 40 -------- packages/core-api/src/app/App.test.tsx | 128 +++++++++++++++++++++++++ packages/core-api/src/app/App.tsx | 1 + 3 files changed, 129 insertions(+), 40 deletions(-) delete mode 100644 packages/core-api/src/app/App.test.ts create mode 100644 packages/core-api/src/app/App.test.tsx diff --git a/packages/core-api/src/app/App.test.ts b/packages/core-api/src/app/App.test.ts deleted file mode 100644 index a25256491b..0000000000 --- a/packages/core-api/src/app/App.test.ts +++ /dev/null @@ -1,40 +0,0 @@ -/* - * 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 { createExternalRouteRef, createRouteRef } from '../routing/RouteRef'; -import { generateBoundRoutes } from './App'; - -describe('generateBoundRoutes', () => { - it('runs happy path', () => { - const external = { myRoute: createExternalRouteRef() }; - const ref = createRouteRef({ path: '', title: '' }); - const result = generateBoundRoutes(({ bind }) => { - bind(external, { myRoute: ref }); - }); - - expect(result.get(external.myRoute)).toBe(ref); - }); - - it('throws on unknown keys', () => { - const external = { myRoute: createExternalRouteRef() }; - const ref = createRouteRef({ path: '', title: '' }); - expect(() => - generateBoundRoutes(({ bind }) => { - bind(external, { someOtherRoute: ref } as any); - }), - ).toThrow('Key someOtherRoute is not an existing external route'); - }); -}); diff --git a/packages/core-api/src/app/App.test.tsx b/packages/core-api/src/app/App.test.tsx new file mode 100644 index 0000000000..431ffa5959 --- /dev/null +++ b/packages/core-api/src/app/App.test.tsx @@ -0,0 +1,128 @@ +/* + * 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 { renderWithEffects } from '@backstage/test-utils'; +import { lightTheme } from '@backstage/theme'; +import { screen } from '@testing-library/react'; +import React from 'react'; +import { BrowserRouter, Routes } from 'react-router-dom'; +import { createRoutableExtension } from '../extensions'; +import { defaultSystemIcons } from '../icons'; +import { createPlugin } from '../plugin'; +import { useRouteRef } from '../routing/hooks'; +import { createExternalRouteRef, createRouteRef } from '../routing/RouteRef'; +import { generateBoundRoutes, PrivateAppImpl } from './App'; + +describe('generateBoundRoutes', () => { + it('runs happy path', () => { + const external = { myRoute: createExternalRouteRef() }; + const ref = createRouteRef({ path: '', title: '' }); + const result = generateBoundRoutes(({ bind }) => { + bind(external, { myRoute: ref }); + }); + + expect(result.get(external.myRoute)).toBe(ref); + }); + + it('throws on unknown keys', () => { + const external = { myRoute: createExternalRouteRef() }; + const ref = createRouteRef({ path: '', title: '' }); + expect(() => + generateBoundRoutes(({ bind }) => { + bind(external, { someOtherRoute: ref } as any); + }), + ).toThrow('Key someOtherRoute is not an existing external route'); + }); +}); + +describe('Integration Test', () => { + const plugin1RouteRef = createRouteRef({ path: '/blah1', title: '' }); + const plugin2RouteRef = createRouteRef({ path: '/blah2', title: '' }); + const externalRouteRef = createExternalRouteRef(); + + const plugin1 = createPlugin({ + id: 'blob', + externalRoutes: { + foo: externalRouteRef, + }, + }); + + const plugin2 = createPlugin({ + id: 'plugin2', + }); + + const HiddenComponent = plugin2.provide( + createRoutableExtension({ + component: () => null, + mountPoint: plugin2RouteRef, + }), + ); + + const ExposedComponent = plugin1.provide( + createRoutableExtension({ + component: () => { + // eslint-disable-next-line react-hooks/rules-of-hooks + const routeRefFunction = useRouteRef(externalRouteRef); + return
Our Route Is: {routeRefFunction({})}
; + }, + mountPoint: plugin1RouteRef, + }), + ); + + it('runs happy path', () => { + const components = { + NotFoundErrorPage: () => null, + BootErrorPage: () => null, + Progress: () => null, + Router: BrowserRouter, + }; + + const app = new PrivateAppImpl({ + apis: [], + defaultApis: [], + themes: [ + { + id: 'light', + title: 'Light Theme', + variant: 'light', + theme: lightTheme, + }, + ], + icons: defaultSystemIcons, + plugins: [], + components, + bindRoutes: ({ bind }) => { + bind(plugin1.externalRoutes, { foo: plugin2RouteRef }); + }, + }); + + const Provider = app.getProvider(); + const Router = app.getRouter(); + + renderWithEffects( + + + + + + + + , + ); + + expect(screen.getByText('Our Route Is: /foo/bar')).toBeInTheDocument(); + }); +}); diff --git a/packages/core-api/src/app/App.tsx b/packages/core-api/src/app/App.tsx index b7265fb5ac..82b224fe59 100644 --- a/packages/core-api/src/app/App.tsx +++ b/packages/core-api/src/app/App.tsx @@ -13,6 +13,7 @@ * See the License for the specific language governing permissions and * limitations under the License. */ + import React, { ComponentType, PropsWithChildren, From ad66d1c8c43642985b14b07d89a35cd0292dd999 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Tue, 8 Dec 2020 15:56:34 +0100 Subject: [PATCH 06/14] core-api,plugins/graphiql: export extension APIs from core-api and port the graphiql plugin Co-authored-by: blam --- packages/app/src/App.tsx | 4 ++-- packages/core-api/src/public.ts | 1 + packages/core-api/src/routing/index.ts | 1 + plugins/graphiql/src/index.ts | 2 +- plugins/graphiql/src/plugin.ts | 15 ++++++++++++++- 5 files changed, 19 insertions(+), 4 deletions(-) diff --git a/packages/app/src/App.tsx b/packages/app/src/App.tsx index 627b5d19aa..a58bf45f9c 100644 --- a/packages/app/src/App.tsx +++ b/packages/app/src/App.tsx @@ -29,7 +29,7 @@ import { hot } from 'react-hot-loader/root'; import { providers } from './identityProviders'; import { Router as CatalogRouter } from '@backstage/plugin-catalog'; import { Router as DocsRouter } from '@backstage/plugin-techdocs'; -import { Router as GraphiQLRouter } from '@backstage/plugin-graphiql'; +import { GraphiQLPage } from '@backstage/plugin-graphiql'; import { Router as TechRadarRouter } from '@backstage/plugin-tech-radar'; import { Router as LighthouseRouter } from '@backstage/plugin-lighthouse'; import { Router as RegisterComponentRouter } from '@backstage/plugin-register-component'; @@ -81,7 +81,7 @@ const AppRoutes = () => ( path="/tech-radar" element={} /> - } /> + } /> } /> Date: Wed, 16 Dec 2020 12:03:17 +0100 Subject: [PATCH 07/14] core-api: rename BindRouteFunc to AppRouteBinder --- packages/core-api/src/app/App.tsx | 4 ++-- packages/core-api/src/app/types.ts | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/core-api/src/app/App.tsx b/packages/core-api/src/app/App.tsx index 82b224fe59..4a23c26bba 100644 --- a/packages/core-api/src/app/App.tsx +++ b/packages/core-api/src/app/App.tsx @@ -65,8 +65,8 @@ import { AppComponents, AppConfigLoader, AppOptions, + AppRouteBinder, BackstageApp, - BindRouteFunc, SignInPageProps, SignInResult, } from './types'; @@ -77,7 +77,7 @@ export function generateBoundRoutes( const result = new Map(); if (bindRoutes) { - const bind: BindRouteFunc = (externalRoutes, targetRoutes) => { + const bind: AppRouteBinder = (externalRoutes, targetRoutes) => { for (const [key, value] of Object.entries(targetRoutes)) { const externalRoute = externalRoutes[key]; if (!externalRoute) { diff --git a/packages/core-api/src/app/types.ts b/packages/core-api/src/app/types.ts index 3eeac72a30..898f4d978b 100644 --- a/packages/core-api/src/app/types.ts +++ b/packages/core-api/src/app/types.ts @@ -79,7 +79,7 @@ export type AppComponents = { */ export type AppConfigLoader = () => Promise; -export type BindRouteFunc = ( +export type AppRouteBinder = ( externalRoutes: T, targetRoutes: { [key in keyof T]: RouteRef }, ) => void; @@ -159,7 +159,7 @@ export type AppOptions = { * } * ``` */ - bindRoutes?(context: { bind: BindRouteFunc }): void; + bindRoutes?(context: { bind: AppRouteBinder }): void; }; export type BackstageApp = { From eb6a928eb0f0f1f7057437a3b28dc14552511a0d Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Wed, 16 Dec 2020 16:12:29 +0100 Subject: [PATCH 08/14] core-api: avoid exporting AnyRoutes, AnyExternalRoutes, and ExternalRouteRef constructor --- packages/core-api/src/app/types.ts | 2 +- packages/core-api/src/plugin/index.ts | 17 ++++++++++++++++- packages/core-api/src/routing/RouteRef.ts | 10 +++++++++- 3 files changed, 26 insertions(+), 3 deletions(-) diff --git a/packages/core-api/src/app/types.ts b/packages/core-api/src/app/types.ts index 898f4d978b..b6b1002d12 100644 --- a/packages/core-api/src/app/types.ts +++ b/packages/core-api/src/app/types.ts @@ -16,7 +16,7 @@ import { ComponentType } from 'react'; import { IconComponent, SystemIconKey, SystemIcons } from '../icons'; -import { BackstagePlugin, AnyExternalRoutes } from '../plugin'; +import { BackstagePlugin, AnyExternalRoutes } from '../plugin/types'; import { RouteRef } from '../routing'; import { AnyApiFactory } from '../apis'; import { AppTheme, ProfileInfo } from '../apis/definitions'; diff --git a/packages/core-api/src/plugin/index.ts b/packages/core-api/src/plugin/index.ts index 79b0575755..bbeeca4824 100644 --- a/packages/core-api/src/plugin/index.ts +++ b/packages/core-api/src/plugin/index.ts @@ -15,4 +15,19 @@ */ export { createPlugin } from './Plugin'; -export * from './types'; +export type { + BackstagePlugin, + Extension, + FeatureFlagOutput, + FeatureFlagsHooks, + LegacyRedirectRouteOutput, + LegacyRouteOutput, + PluginConfig, + PluginHooks, + PluginOutput, + RedirectRouteOutput, + RouteOptions, + RouteOutput, + RoutePath, + RouterHooks, +} from './types'; diff --git a/packages/core-api/src/routing/RouteRef.ts b/packages/core-api/src/routing/RouteRef.ts index c7db48045a..f89634e0ec 100644 --- a/packages/core-api/src/routing/RouteRef.ts +++ b/packages/core-api/src/routing/RouteRef.ts @@ -54,12 +54,20 @@ export function createRouteRef< return new AbsoluteRouteRef(config); } +const create = Symbol('create-external-route-ref'); + export class ExternalRouteRef { + static [create]() { + return new ExternalRouteRef(); + } + + private constructor() {} + toString() { return `externalRouteRef{}`; } } export function createExternalRouteRef(): ExternalRouteRef { - return new ExternalRouteRef(); + return ExternalRouteRef[create](); } From 1dc445e8937c5e48b07d21114ed80cbacc2e6c95 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Wed, 16 Dec 2020 18:06:08 +0100 Subject: [PATCH 09/14] changeset: add changesets for new plugin extension API --- .changeset/quiet-pants-happen.md | 7 +++++++ .changeset/short-dancers-explode.md | 5 +++++ 2 files changed, 12 insertions(+) create mode 100644 .changeset/quiet-pants-happen.md create mode 100644 .changeset/short-dancers-explode.md diff --git a/.changeset/quiet-pants-happen.md b/.changeset/quiet-pants-happen.md new file mode 100644 index 0000000000..165e3ae12c --- /dev/null +++ b/.changeset/quiet-pants-happen.md @@ -0,0 +1,7 @@ +--- +'@backstage/core': patch +'@backstage/test-utils': patch +'@backstage/plugin-graphiql': patch +--- + +Update to use new plugin extension API diff --git a/.changeset/short-dancers-explode.md b/.changeset/short-dancers-explode.md new file mode 100644 index 0000000000..56fee63fe7 --- /dev/null +++ b/.changeset/short-dancers-explode.md @@ -0,0 +1,5 @@ +--- +'@backstage/core-api': patch +--- + +Introduce new plugin extension API From 6d263b0bdf5663bc57a89dbf8ad3993934935d7d Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Wed, 16 Dec 2020 21:21:28 +0100 Subject: [PATCH 10/14] core-api: default type parameters for BackstagePlugin to avoid breakage Co-authored-by: blam --- packages/core-api/src/plugin/Plugin.tsx | 4 ++-- packages/core-api/src/plugin/types.ts | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/core-api/src/plugin/Plugin.tsx b/packages/core-api/src/plugin/Plugin.tsx index 797dcd852e..dd6d7960ac 100644 --- a/packages/core-api/src/plugin/Plugin.tsx +++ b/packages/core-api/src/plugin/Plugin.tsx @@ -93,8 +93,8 @@ export class PluginImpl< } export function createPlugin< - Routes extends AnyRoutes, - ExternalRoutes extends AnyExternalRoutes + Routes extends AnyRoutes = {}, + ExternalRoutes extends AnyExternalRoutes = {} >( config: PluginConfig, ): BackstagePlugin { diff --git a/packages/core-api/src/plugin/types.ts b/packages/core-api/src/plugin/types.ts index e3f2e38972..94bd4e8172 100644 --- a/packages/core-api/src/plugin/types.ts +++ b/packages/core-api/src/plugin/types.ts @@ -76,8 +76,8 @@ export type AnyRoutes = { [name: string]: RouteRef }; export type AnyExternalRoutes = { [name: string]: ExternalRouteRef }; export type BackstagePlugin< - Routes extends AnyRoutes, - ExternalRoutes extends AnyExternalRoutes + Routes extends AnyRoutes = {}, + ExternalRoutes extends AnyExternalRoutes = {} > = { getId(): string; output(): PluginOutput[]; From aeb9fb254ee7b0c88a9caa0b760fef1ee2f03f69 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Thu, 17 Dec 2020 13:49:21 +0100 Subject: [PATCH 11/14] core-api: add route validation to app + test Co-authored-by: blam --- packages/core-api/src/app/App.test.tsx | 60 +++++++++++++++++++++++++- packages/core-api/src/app/App.tsx | 4 +- 2 files changed, 61 insertions(+), 3 deletions(-) diff --git a/packages/core-api/src/app/App.test.tsx b/packages/core-api/src/app/App.test.tsx index 431ffa5959..7ab4928abf 100644 --- a/packages/core-api/src/app/App.test.tsx +++ b/packages/core-api/src/app/App.test.tsx @@ -14,9 +14,9 @@ * limitations under the License. */ -import { renderWithEffects } from '@backstage/test-utils'; +import { renderWithEffects, withLogCollector } from '@backstage/test-utils'; import { lightTheme } from '@backstage/theme'; -import { screen } from '@testing-library/react'; +import { render, screen } from '@testing-library/react'; import React from 'react'; import { BrowserRouter, Routes } from 'react-router-dom'; import { createRoutableExtension } from '../extensions'; @@ -125,4 +125,60 @@ describe('Integration Test', () => { expect(screen.getByText('Our Route Is: /foo/bar')).toBeInTheDocument(); }); + + it('should throw some error when the route has duplicate params', () => { + const components = { + NotFoundErrorPage: () => null, + BootErrorPage: () => null, + Progress: () => null, + Router: BrowserRouter, + }; + + const app = new PrivateAppImpl({ + apis: [], + defaultApis: [], + themes: [ + { + id: 'light', + title: 'Light Theme', + variant: 'light', + theme: lightTheme, + }, + ], + icons: defaultSystemIcons, + plugins: [], + components, + bindRoutes: ({ bind }) => { + bind(plugin1.externalRoutes, { foo: plugin2RouteRef }); + }, + }); + + const Provider = app.getProvider(); + const Router = app.getRouter(); + const { error: errorLogs } = withLogCollector(() => { + expect(() => + render( + + + + + + + + + , + ), + ).toThrow( + 'Parameter :thing is duplicated in path /test/:thing/some/:thing', + ); + }); + expect(errorLogs).toEqual([ + expect.stringContaining( + 'Parameter :thing is duplicated in path /test/:thing/some/:thing', + ), + expect.stringContaining( + 'The above error occurred in the component', + ), + ]); + }); }); diff --git a/packages/core-api/src/app/App.tsx b/packages/core-api/src/app/App.tsx index 4a23c26bba..08393f3e9e 100644 --- a/packages/core-api/src/app/App.tsx +++ b/packages/core-api/src/app/App.tsx @@ -56,7 +56,7 @@ import { routeParentCollector, routePathCollector, } from '../routing/collectors'; -import { RoutingProvider } from '../routing/hooks'; +import { RoutingProvider, validateRoutes } from '../routing/hooks'; import { ExternalRouteRef } from '../routing/RouteRef'; import { AppContextProvider } from './AppContext'; import { AppIdentity } from './AppIdentity'; @@ -253,6 +253,8 @@ export class PrivateAppImpl implements BackstageApp { }, }); + validateRoutes(routePaths, routeParents); + const loadedConfig = useConfigLoader( this.configLoader, this.components, From adb1afdc2bad5bf2ed4157b5155fec48e9ef34df Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Thu, 17 Dec 2020 16:11:00 +0100 Subject: [PATCH 12/14] core-api: refactor createReactExtension to forward component types directly and avoid @types/react dependency Co-authored-by: blam --- packages/core-api/src/app/App.test.tsx | 6 +-- .../src/extensions/extensions.test.tsx | 4 +- .../core-api/src/extensions/extensions.tsx | 43 ++++++++----------- .../core-api/src/plugin/collectors.test.tsx | 4 +- .../core-api/src/routing/collectors.test.tsx | 4 +- packages/core-api/src/routing/hooks.test.tsx | 5 ++- 6 files changed, 33 insertions(+), 33 deletions(-) diff --git a/packages/core-api/src/app/App.test.tsx b/packages/core-api/src/app/App.test.tsx index 7ab4928abf..4050343bbf 100644 --- a/packages/core-api/src/app/App.test.tsx +++ b/packages/core-api/src/app/App.test.tsx @@ -17,7 +17,7 @@ import { renderWithEffects, withLogCollector } from '@backstage/test-utils'; import { lightTheme } from '@backstage/theme'; import { render, screen } from '@testing-library/react'; -import React from 'react'; +import React, { PropsWithChildren } from 'react'; import { BrowserRouter, Routes } from 'react-router-dom'; import { createRoutableExtension } from '../extensions'; import { defaultSystemIcons } from '../icons'; @@ -66,14 +66,14 @@ describe('Integration Test', () => { const HiddenComponent = plugin2.provide( createRoutableExtension({ - component: () => null, + component: (_: { path?: string }) =>
, mountPoint: plugin2RouteRef, }), ); const ExposedComponent = plugin1.provide( createRoutableExtension({ - component: () => { + component: (_: PropsWithChildren<{ path?: string }>) => { // eslint-disable-next-line react-hooks/rules-of-hooks const routeRefFunction = useRouteRef(externalRouteRef); return
Our Route Is: {routeRefFunction({})}
; diff --git a/packages/core-api/src/extensions/extensions.test.tsx b/packages/core-api/src/extensions/extensions.test.tsx index 92fcecebc4..861045fb3d 100644 --- a/packages/core-api/src/extensions/extensions.test.tsx +++ b/packages/core-api/src/extensions/extensions.test.tsx @@ -30,7 +30,7 @@ const plugin = createPlugin({ describe('extensions', () => { it('should create a react extension with component data', () => { - const Component = () => null; + const Component = () =>
; const extension = createReactExtension({ component: Component, @@ -47,7 +47,7 @@ describe('extensions', () => { }); it('should create react extensions of different types', () => { - const Component = () => null; + const Component = () =>
; const routeRef = createRouteRef({ path: '/foo', title: 'Foo' }); const extension1 = createComponentExtension({ diff --git a/packages/core-api/src/extensions/extensions.tsx b/packages/core-api/src/extensions/extensions.tsx index 079fec4597..aca29e87dc 100644 --- a/packages/core-api/src/extensions/extensions.tsx +++ b/packages/core-api/src/extensions/extensions.tsx @@ -14,24 +14,14 @@ * limitations under the License. */ -import React, { - NamedExoticComponent, - ComponentType, - PropsWithChildren, -} from 'react'; +import React from 'react'; import { RouteRef } from '../routing'; import { attachComponentData } from './componentData'; import { Extension, BackstagePlugin } from '../plugin/types'; -export function createRoutableExtension(options: { - component: ComponentType; - mountPoint: RouteRef; - // TODO(Rugvip): We want to carry forward the exact props type from the inner component, with - // or without children. ComponentType stops us from doing that though, as it always - // adds children to the props internally. We may want to work around this with custom types. -}): Extension< - NamedExoticComponent> -> { +export function createRoutableExtension< + T extends (props: any) => JSX.Element +>(options: { component: T; mountPoint: RouteRef }): Extension { const { component, mountPoint } = options; return createReactExtension({ component, @@ -41,21 +31,24 @@ export function createRoutableExtension(options: { }); } -export function createComponentExtension(options: { - component: ComponentType; -}): Extension> { +export function createComponentExtension< + T extends (props: any) => JSX.Element +>(options: { component: T }): Extension { const { component } = options; return createReactExtension({ component }); } -export function createReactExtension(options: { - component: ComponentType; - data?: Record; -}): Extension> { - const { component: Component, data = {} } = options; +export function createReactExtension< + T extends (props: any) => JSX.Element +>(options: { component: T; data?: Record }): Extension { + const { data = {} } = options; + const Component = options.component as T & { + displayName?: string; + }; + return { - expose(plugin: BackstagePlugin): NamedExoticComponent { - const Result = (props: Props) => ; + expose(plugin: BackstagePlugin) { + const Result: any = (props: any) => ; attachComponentData(Result, 'core.plugin', plugin); for (const [key, value] of Object.entries(data)) { @@ -66,7 +59,7 @@ export function createReactExtension(options: { if (name) { Result.displayName = `Extension(${name})`; } - return Result as NamedExoticComponent; + return Result; }, }; } diff --git a/packages/core-api/src/plugin/collectors.test.tsx b/packages/core-api/src/plugin/collectors.test.tsx index f040cfd433..e5f8a123e3 100644 --- a/packages/core-api/src/plugin/collectors.test.tsx +++ b/packages/core-api/src/plugin/collectors.test.tsx @@ -30,7 +30,9 @@ import { import { pluginCollector } from './collectors'; const mockConfig = () => ({ path: '/foo', title: 'Foo' }); -const MockComponent = ({ children }: PropsWithChildren<{}>) => <>{children}; +const MockComponent = ({ children }: PropsWithChildren<{ path?: string }>) => ( + <>{children} +); const pluginA = createPlugin({ id: 'my-plugin-a' }); const pluginB = createPlugin({ id: 'my-plugin-b' }); diff --git a/packages/core-api/src/routing/collectors.test.tsx b/packages/core-api/src/routing/collectors.test.tsx index 2595b8c0a0..e44474565c 100644 --- a/packages/core-api/src/routing/collectors.test.tsx +++ b/packages/core-api/src/routing/collectors.test.tsx @@ -28,7 +28,9 @@ import { createRoutableExtension } from '../extensions'; import { MemoryRouter, Routes, Route } from 'react-router-dom'; const mockConfig = () => ({ path: '/foo', title: 'Foo' }); -const MockComponent = ({ children }: PropsWithChildren<{}>) => <>{children}; +const MockComponent = ({ children }: PropsWithChildren<{ path?: string }>) => ( + <>{children} +); const plugin = createPlugin({ id: 'my-plugin' }); diff --git a/packages/core-api/src/routing/hooks.test.tsx b/packages/core-api/src/routing/hooks.test.tsx index 6bc80b7c08..a29e263e14 100644 --- a/packages/core-api/src/routing/hooks.test.tsx +++ b/packages/core-api/src/routing/hooks.test.tsx @@ -47,7 +47,9 @@ const mockConfig = (extra?: Partial>) => ({ title: 'Unused', ...extra, }); -const MockComponent = ({ children }: PropsWithChildren<{}>) => <>{children}; +const MockComponent = ({ children }: PropsWithChildren<{ path?: string }>) => ( + <>{children} +); const plugin = createPlugin({ id: 'my-plugin' }); @@ -61,6 +63,7 @@ const eRefB = createExternalRouteRef(); const eRefC = createExternalRouteRef(); const MockRouteSource = (props: { + path?: string; name: string; routeRef: RouteRef | ExternalRouteRef; params?: T; From c35554dc4c42dde7279f3a4fbf09602f72f794ad Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Thu, 17 Dec 2020 17:20:24 +0100 Subject: [PATCH 13/14] core-api: add lazy loading of component extensions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Fredrik Adelöw Co-authored-by: blam --- packages/core-api/src/app/App.test.tsx | 17 +++-- .../src/extensions/extensions.test.tsx | 10 ++- .../core-api/src/extensions/extensions.tsx | 54 ++++++++++---- .../core-api/src/plugin/collectors.test.tsx | 16 ++-- .../core-api/src/routing/collectors.test.tsx | 27 +++++-- packages/core-api/src/routing/hooks.test.tsx | 73 ++++++++++++------- plugins/graphiql/src/plugin.ts | 3 +- 7 files changed, 136 insertions(+), 64 deletions(-) diff --git a/packages/core-api/src/app/App.test.tsx b/packages/core-api/src/app/App.test.tsx index 4050343bbf..8dd40d52b0 100644 --- a/packages/core-api/src/app/App.test.tsx +++ b/packages/core-api/src/app/App.test.tsx @@ -66,23 +66,24 @@ describe('Integration Test', () => { const HiddenComponent = plugin2.provide( createRoutableExtension({ - component: (_: { path?: string }) =>
, + component: () => Promise.resolve((_: { path?: string }) =>
), mountPoint: plugin2RouteRef, }), ); const ExposedComponent = plugin1.provide( createRoutableExtension({ - component: (_: PropsWithChildren<{ path?: string }>) => { - // eslint-disable-next-line react-hooks/rules-of-hooks - const routeRefFunction = useRouteRef(externalRouteRef); - return
Our Route Is: {routeRefFunction({})}
; - }, + component: () => + Promise.resolve((_: PropsWithChildren<{ path?: string }>) => { + // eslint-disable-next-line react-hooks/rules-of-hooks + const routeRefFunction = useRouteRef(externalRouteRef); + return
Our Route Is: {routeRefFunction({})}
; + }), mountPoint: plugin1RouteRef, }), ); - it('runs happy path', () => { + it('runs happy path', async () => { const components = { NotFoundErrorPage: () => null, BootErrorPage: () => null, @@ -112,7 +113,7 @@ describe('Integration Test', () => { const Provider = app.getProvider(); const Router = app.getRouter(); - renderWithEffects( + await renderWithEffects( diff --git a/packages/core-api/src/extensions/extensions.test.tsx b/packages/core-api/src/extensions/extensions.test.tsx index 861045fb3d..26755b3bcf 100644 --- a/packages/core-api/src/extensions/extensions.test.tsx +++ b/packages/core-api/src/extensions/extensions.test.tsx @@ -33,7 +33,9 @@ describe('extensions', () => { const Component = () =>
; const extension = createReactExtension({ - component: Component, + component: { + sync: Component, + }, data: { myData: { foo: 'bar' }, }, @@ -51,11 +53,13 @@ describe('extensions', () => { const routeRef = createRouteRef({ path: '/foo', title: 'Foo' }); const extension1 = createComponentExtension({ - component: Component, + component: { + sync: Component, + }, }); const extension2 = createRoutableExtension({ - component: Component, + component: () => Promise.resolve(Component), mountPoint: routeRef, }); diff --git a/packages/core-api/src/extensions/extensions.tsx b/packages/core-api/src/extensions/extensions.tsx index aca29e87dc..adec690f20 100644 --- a/packages/core-api/src/extensions/extensions.tsx +++ b/packages/core-api/src/extensions/extensions.tsx @@ -14,17 +14,30 @@ * limitations under the License. */ -import React from 'react'; +import React, { lazy, Suspense } from 'react'; import { RouteRef } from '../routing'; import { attachComponentData } from './componentData'; import { Extension, BackstagePlugin } from '../plugin/types'; +type ComponentLoader = + | { + lazy: () => Promise; + } + | { + sync: T; + }; + export function createRoutableExtension< T extends (props: any) => JSX.Element ->(options: { component: T; mountPoint: RouteRef }): Extension { +>(options: { + component: () => Promise; + mountPoint: RouteRef; +}): Extension { const { component, mountPoint } = options; return createReactExtension({ - component, + component: { + lazy: component, + }, data: { 'core.mountPoint': mountPoint, }, @@ -33,32 +46,47 @@ export function createRoutableExtension< export function createComponentExtension< T extends (props: any) => JSX.Element ->(options: { component: T }): Extension { +>(options: { component: ComponentLoader }): Extension { const { component } = options; return createReactExtension({ component }); } export function createReactExtension< T extends (props: any) => JSX.Element ->(options: { component: T; data?: Record }): Extension { +>(options: { + component: ComponentLoader; + data?: Record; +}): Extension { const { data = {} } = options; - const Component = options.component as T & { - displayName?: string; - }; + + let Component: T; + if ('lazy' in options.component) { + const lazyLoader = options.component.lazy; + Component = (lazy(() => + lazyLoader().then(component => ({ default: component })), + ) as unknown) as T; + } else { + Component = options.component.sync; + } + const componentName = + (Component as { displayName?: string }).displayName || + Component.name || + 'Component'; return { expose(plugin: BackstagePlugin) { - const Result: any = (props: any) => ; + const Result: any = (props: any) => ( + + + + ); attachComponentData(Result, 'core.plugin', plugin); for (const [key, value] of Object.entries(data)) { attachComponentData(Result, key, value); } - const name = Component.displayName || Component.name || 'Component'; - if (name) { - Result.displayName = `Extension(${name})`; - } + Result.displayName = `Extension(${componentName})`; return Result; }, }; diff --git a/packages/core-api/src/plugin/collectors.test.tsx b/packages/core-api/src/plugin/collectors.test.tsx index e5f8a123e3..5baf2539ab 100644 --- a/packages/core-api/src/plugin/collectors.test.tsx +++ b/packages/core-api/src/plugin/collectors.test.tsx @@ -42,19 +42,25 @@ const ref1 = createRouteRef(mockConfig()); const ref2 = createRouteRef(mockConfig()); const Extension1 = pluginA.provide( - createRoutableExtension({ component: MockComponent, mountPoint: ref1 }), + createRoutableExtension({ + component: () => Promise.resolve(MockComponent), + mountPoint: ref1, + }), ); const Extension2 = pluginB.provide( - createRoutableExtension({ component: MockComponent, mountPoint: ref2 }), + createRoutableExtension({ + component: () => Promise.resolve(MockComponent), + mountPoint: ref2, + }), ); const Extension3 = pluginA.provide( - createComponentExtension({ component: MockComponent }), + createComponentExtension({ component: { sync: MockComponent } }), ); const Extension4 = pluginB.provide( - createComponentExtension({ component: MockComponent }), + createComponentExtension({ component: { sync: MockComponent } }), ); const Extension5 = pluginC.provide( - createComponentExtension({ component: MockComponent }), + createComponentExtension({ component: { sync: MockComponent } }), ); describe('collection', () => { diff --git a/packages/core-api/src/routing/collectors.test.tsx b/packages/core-api/src/routing/collectors.test.tsx index e44474565c..200846f175 100644 --- a/packages/core-api/src/routing/collectors.test.tsx +++ b/packages/core-api/src/routing/collectors.test.tsx @@ -41,19 +41,34 @@ const ref4 = createRouteRef(mockConfig()); const ref5 = createRouteRef(mockConfig()); const Extension1 = plugin.provide( - createRoutableExtension({ component: MockComponent, mountPoint: ref1 }), + createRoutableExtension({ + component: () => Promise.resolve(MockComponent), + mountPoint: ref1, + }), ); const Extension2 = plugin.provide( - createRoutableExtension({ component: MockComponent, mountPoint: ref2 }), + createRoutableExtension({ + component: () => Promise.resolve(MockComponent), + mountPoint: ref2, + }), ); const Extension3 = plugin.provide( - createRoutableExtension({ component: MockComponent, mountPoint: ref3 }), + createRoutableExtension({ + component: () => Promise.resolve(MockComponent), + mountPoint: ref3, + }), ); const Extension4 = plugin.provide( - createRoutableExtension({ component: MockComponent, mountPoint: ref4 }), + createRoutableExtension({ + component: () => Promise.resolve(MockComponent), + mountPoint: ref4, + }), ); const Extension5 = plugin.provide( - createRoutableExtension({ component: MockComponent, mountPoint: ref5 }), + createRoutableExtension({ + component: () => Promise.resolve(MockComponent), + mountPoint: ref5, + }), ); describe('discovery', () => { @@ -190,6 +205,6 @@ describe('discovery', () => { routeParents: routeParentCollector, }, }), - ).toThrow(`Visited element Extension(MockComponent) twice`); + ).toThrow(`Visited element Extension(Component) twice`); }); }); diff --git a/packages/core-api/src/routing/hooks.test.tsx b/packages/core-api/src/routing/hooks.test.tsx index a29e263e14..3dbeaf8e3e 100644 --- a/packages/core-api/src/routing/hooks.test.tsx +++ b/packages/core-api/src/routing/hooks.test.tsx @@ -85,19 +85,34 @@ const MockRouteSource = (props: { }; const Extension1 = plugin.provide( - createRoutableExtension({ component: MockComponent, mountPoint: ref1 }), + createRoutableExtension({ + component: () => Promise.resolve(MockComponent), + mountPoint: ref1, + }), ); const Extension2 = plugin.provide( - createRoutableExtension({ component: MockRouteSource, mountPoint: ref2 }), + createRoutableExtension({ + component: () => Promise.resolve(MockRouteSource), + mountPoint: ref2, + }), ); const Extension3 = plugin.provide( - createRoutableExtension({ component: MockComponent, mountPoint: ref3 }), + createRoutableExtension({ + component: () => Promise.resolve(MockComponent), + mountPoint: ref3, + }), ); const Extension4 = plugin.provide( - createRoutableExtension({ component: MockRouteSource, mountPoint: ref4 }), + createRoutableExtension({ + component: () => Promise.resolve(MockRouteSource), + mountPoint: ref4, + }), ); const Extension5 = plugin.provide( - createRoutableExtension({ component: MockComponent, mountPoint: ref5 }), + createRoutableExtension({ + component: () => Promise.resolve(MockComponent), + mountPoint: ref5, + }), ); function withRoutingProvider( @@ -127,7 +142,7 @@ function withRoutingProvider( } describe('discovery', () => { - it('should handle simple routeRef path creation for routeRefs used in other parts of the app', () => { + it('should handle simple routeRef path creation for routeRefs used in other parts of the app', async () => { const root = ( @@ -151,7 +166,9 @@ describe('discovery', () => { ]), ); - expect(rendered.getByText('Path at inside: /foo/bar')).toBeInTheDocument(); + await expect( + rendered.findByText('Path at inside: /foo/bar'), + ).resolves.toBeInTheDocument(); expect( rendered.getByText('Path at insideExternal: /baz'), ).toBeInTheDocument(); @@ -164,7 +181,7 @@ describe('discovery', () => { ).toBeInTheDocument(); }); - it('should handle routeRefs with parameters', () => { + it('should handle routeRefs with parameters', async () => { const root = ( @@ -187,37 +204,39 @@ describe('discovery', () => { const rendered = render(withRoutingProvider(root)); - expect( - rendered.getByText('Path at inside: /foo/bar/bleb'), - ).toBeInTheDocument(); + await expect( + rendered.findByText('Path at inside: /foo/bar/bleb'), + ).resolves.toBeInTheDocument(); expect( rendered.getByText('Path at outside: /foo/bar/blob'), ).toBeInTheDocument(); }); - it('should handle relative routing within parameterized routePaths', () => { + it('should handle relative routing within parameterized routePaths', async () => { const root = ( - - - - - - - - + + + + + + + + + + ); const rendered = render(withRoutingProvider(root)); - expect( - rendered.getByText('Path at inside: /foo/blob/baz'), - ).toBeInTheDocument(); + await expect( + rendered.findByText('Path at inside: /foo/blob/baz'), + ).resolves.toBeInTheDocument(); }); it('should throw errors for routing to other routeRefs with unsupported parameters', () => { diff --git a/plugins/graphiql/src/plugin.ts b/plugins/graphiql/src/plugin.ts index fb4c33760d..a0bb79ed29 100644 --- a/plugins/graphiql/src/plugin.ts +++ b/plugins/graphiql/src/plugin.ts @@ -19,7 +19,6 @@ import { createApiFactory, createRoutableExtension, } from '@backstage/core'; -import { GraphiQLPage as Component } from './components'; import { graphQlBrowseApiRef, GraphQLEndpoints } from './lib/api'; import { graphiQLRouteRef } from './route-refs'; @@ -42,7 +41,7 @@ export const plugin = createPlugin({ export const GraphiQLPage = plugin.provide( createRoutableExtension({ - component: Component, + component: () => import('./components').then(m => m.GraphiQLPage), mountPoint: graphiQLRouteRef, }), ); From c47ec51a943f73b69cd63535d3f17bae8e05851c Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Thu, 17 Dec 2020 17:48:57 +0100 Subject: [PATCH 14/14] core-api: memoize app element traversal and validation --- packages/core-api/src/app/App.tsx | 25 ++++++++++++++----------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/packages/core-api/src/app/App.tsx b/packages/core-api/src/app/App.tsx index 08393f3e9e..2aaeba9796 100644 --- a/packages/core-api/src/app/App.tsx +++ b/packages/core-api/src/app/App.tsx @@ -242,18 +242,21 @@ export class PrivateAppImpl implements BackstageApp { [], ); - // Probably want to memo this? - const { routePaths, routeParents, routeObjects } = traverseElementTree({ - root: children, - discoverers: [childDiscoverer, routeElementDiscoverer], - collectors: { - routePaths: routePathCollector, - routeParents: routeParentCollector, - routeObjects: routeObjectCollector, - }, - }); + const { routePaths, routeParents, routeObjects } = useMemo(() => { + const result = traverseElementTree({ + root: children, + discoverers: [childDiscoverer, routeElementDiscoverer], + collectors: { + routePaths: routePathCollector, + routeParents: routeParentCollector, + routeObjects: routeObjectCollector, + }, + }); - validateRoutes(routePaths, routeParents); + validateRoutes(result.routePaths, result.routeParents); + + return result; + }, [children]); const loadedConfig = useConfigLoader( this.configLoader,