From 41c8e3d35fb8df53ab84372f4f16d5136492ec83 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Thu, 27 May 2021 00:20:02 +0200 Subject: [PATCH] app-api,plugin-api: sync with core-api at master@e9e70677 Signed-off-by: Patrik Oldsberg --- .../AlertApi/AlertApiForwarder.ts | 2 +- .../AppThemeApi/AppThemeSelector.ts | 2 +- .../ErrorApi/ErrorApiForwarder.ts | 2 +- .../OAuthRequestApi/OAuthPendingRequests.ts | 2 +- .../OAuthRequestApi/OAuthRequestManager.ts | 2 +- .../apis/system/ApiFactoryRegistry.test.ts | 4 +- .../src/apis/system/ApiProvider.test.tsx | 8 +- packages/app-api/src/app/App.test.tsx | 74 ++++++++++++ packages/app-api/src/app/App.tsx | 84 ++++++++------ packages/app-api/src/app/createApp.tsx | 6 +- packages/app-api/src/app/types.ts | 2 +- .../app-api/src/extensions/extensions.tsx | 70 ++++++++---- packages/app-api/src/extensions/traversal.ts | 9 -- .../AuthSessionManager/SessionStateTracker.ts | 2 +- packages/app-api/src/lib/subjects.ts | 8 ++ .../app-api/src/plugins/collectors.test.tsx | 105 ++++++++++++++++++ packages/app-api/src/plugins/collectors.ts | 47 ++++++++ packages/app-api/src/plugins/index.ts | 17 +++ .../app-api/src/routing/RouteResolver.test.ts | 59 +++++++++- .../app-api/src/routing/collectors.test.tsx | 48 ++++---- packages/app-api/src/routing/collectors.tsx | 19 +++- packages/app-api/src/types.ts | 14 ++- .../src/apis/definitions/OAuthRequestApi.ts | 2 +- packages/plugin-api/src/app/types.ts | 2 +- packages/plugin-api/src/index.test.ts | 1 + packages/plugin-api/src/plugin/types.ts | 4 +- packages/plugin-api/src/routing/index.ts | 1 + .../src/routing/useRouteRefParams.test.tsx | 54 +++++++++ .../src/routing/useRouteRefParams.ts | 24 ++++ 29 files changed, 560 insertions(+), 114 deletions(-) create mode 100644 packages/app-api/src/plugins/collectors.test.tsx create mode 100644 packages/app-api/src/plugins/collectors.ts create mode 100644 packages/app-api/src/plugins/index.ts create mode 100644 packages/plugin-api/src/routing/useRouteRefParams.test.tsx create mode 100644 packages/plugin-api/src/routing/useRouteRefParams.ts diff --git a/packages/app-api/src/apis/implementations/AlertApi/AlertApiForwarder.ts b/packages/app-api/src/apis/implementations/AlertApi/AlertApiForwarder.ts index 322afe71c5..43865a4329 100644 --- a/packages/app-api/src/apis/implementations/AlertApi/AlertApiForwarder.ts +++ b/packages/app-api/src/apis/implementations/AlertApi/AlertApiForwarder.ts @@ -14,7 +14,7 @@ * limitations under the License. */ import { AlertApi, AlertMessage } from '@backstage/plugin-api'; -import { PublishSubject } from '../../../lib'; +import { PublishSubject } from '../../../lib/subjects'; import { Observable } from '../../../types'; /** diff --git a/packages/app-api/src/apis/implementations/AppThemeApi/AppThemeSelector.ts b/packages/app-api/src/apis/implementations/AppThemeApi/AppThemeSelector.ts index eb00382601..142d700dd5 100644 --- a/packages/app-api/src/apis/implementations/AppThemeApi/AppThemeSelector.ts +++ b/packages/app-api/src/apis/implementations/AppThemeApi/AppThemeSelector.ts @@ -15,7 +15,7 @@ */ import { AppThemeApi, AppTheme } from '@backstage/plugin-api'; -import { BehaviorSubject } from '../../../lib'; +import { BehaviorSubject } from '../../../lib/subjects'; import { Observable } from '../../../types'; const STORAGE_KEY = 'theme'; diff --git a/packages/app-api/src/apis/implementations/ErrorApi/ErrorApiForwarder.ts b/packages/app-api/src/apis/implementations/ErrorApi/ErrorApiForwarder.ts index a9841bb660..ef4cd815f3 100644 --- a/packages/app-api/src/apis/implementations/ErrorApi/ErrorApiForwarder.ts +++ b/packages/app-api/src/apis/implementations/ErrorApi/ErrorApiForwarder.ts @@ -14,7 +14,7 @@ * limitations under the License. */ import { ErrorApi, ErrorContext } from '@backstage/plugin-api'; -import { PublishSubject } from '../../../lib'; +import { PublishSubject } from '../../../lib/subjects'; import { Observable } from '../../../types'; /** diff --git a/packages/app-api/src/apis/implementations/OAuthRequestApi/OAuthPendingRequests.ts b/packages/app-api/src/apis/implementations/OAuthRequestApi/OAuthPendingRequests.ts index e616ece5ae..f0710cb466 100644 --- a/packages/app-api/src/apis/implementations/OAuthRequestApi/OAuthPendingRequests.ts +++ b/packages/app-api/src/apis/implementations/OAuthRequestApi/OAuthPendingRequests.ts @@ -14,7 +14,7 @@ * limitations under the License. */ -import { BehaviorSubject } from '../../../lib'; +import { BehaviorSubject } from '../../../lib/subjects'; import { Observable } from '../../../types'; type RequestQueueEntry = { diff --git a/packages/app-api/src/apis/implementations/OAuthRequestApi/OAuthRequestManager.ts b/packages/app-api/src/apis/implementations/OAuthRequestApi/OAuthRequestManager.ts index d643b1d060..b7c50f7696 100644 --- a/packages/app-api/src/apis/implementations/OAuthRequestApi/OAuthRequestManager.ts +++ b/packages/app-api/src/apis/implementations/OAuthRequestApi/OAuthRequestManager.ts @@ -21,7 +21,7 @@ import { AuthRequesterOptions, } from '@backstage/plugin-api'; import { OAuthPendingRequests, PendingRequest } from './OAuthPendingRequests'; -import { BehaviorSubject } from '../../../lib'; +import { BehaviorSubject } from '../../../lib/subjects'; import { Observable } from '../../../types'; /** diff --git a/packages/app-api/src/apis/system/ApiFactoryRegistry.test.ts b/packages/app-api/src/apis/system/ApiFactoryRegistry.test.ts index 7baa0f5f99..3537f0549a 100644 --- a/packages/app-api/src/apis/system/ApiFactoryRegistry.test.ts +++ b/packages/app-api/src/apis/system/ApiFactoryRegistry.test.ts @@ -85,7 +85,7 @@ describe('ApiFactoryRegistry', () => { expect(registry.register('app', factory2)).toBe(true); expect(registry.get(ref1)).toEqual(factory2); expect(registry.get(ref2)).toEqual(factory2); - expect(registry.getAllApis()).toEqual(new Set([ref2])); - expect(registry.getAllApis()).not.toEqual(new Set([ref1])); + expect(Array.from(registry.getAllApis())[0]).toBe(ref2); + expect(Array.from(registry.getAllApis())[0]).not.toBe(ref1); }); }); diff --git a/packages/app-api/src/apis/system/ApiProvider.test.tsx b/packages/app-api/src/apis/system/ApiProvider.test.tsx index 2cef83ee53..7786d3176d 100644 --- a/packages/app-api/src/apis/system/ApiProvider.test.tsx +++ b/packages/app-api/src/apis/system/ApiProvider.test.tsx @@ -116,11 +116,11 @@ describe('ApiProvider', () => { withLogCollector(['error'], () => { expect(() => { render(); - }).toThrow(/^No ApiProvider available in react context. /); + }).toThrow(/^No provider available for api-context context/); }).error, ).toEqual([ expect.stringMatching( - /^Error: Uncaught \[Error: No ApiProvider available in react context. /, + /^Error: Uncaught \[Error: No provider available for api-context context/, ), expect.stringMatching( /^The above error occurred in the component/, @@ -131,11 +131,11 @@ describe('ApiProvider', () => { withLogCollector(['error'], () => { expect(() => { render(); - }).toThrow(/^No ApiProvider available in react context. /); + }).toThrow(/^No provider available for api-context context/); }).error, ).toEqual([ expect.stringMatching( - /^Error: Uncaught \[Error: No ApiProvider available in react context. /, + /^Error: Uncaught \[Error: No provider available for api-context context/, ), expect.stringMatching( /^The above error occurred in the component/, diff --git a/packages/app-api/src/app/App.test.tsx b/packages/app-api/src/app/App.test.tsx index 22f2dedc11..19cc4ed348 100644 --- a/packages/app-api/src/app/App.test.tsx +++ b/packages/app-api/src/app/App.test.tsx @@ -14,6 +14,7 @@ * limitations under the License. */ +import { LocalStorageFeatureFlags } from '../apis'; import { renderWithEffects, withLogCollector } from '@backstage/test-utils'; import { lightTheme } from '@backstage/theme'; import { render, screen } from '@testing-library/react'; @@ -22,6 +23,9 @@ import { BrowserRouter, Routes } from 'react-router-dom'; import { createRoutableExtension } from '../extensions'; import { defaultAppIcons } from './icons'; import { + configApiRef, + createApiFactory, + featureFlagsApiRef, createPlugin, useRouteRef, createExternalRouteRef, @@ -92,6 +96,12 @@ describe('Integration Test', () => { const plugin1 = createPlugin({ id: 'blob', + // Both absolute and sub route refs should be assignable to the plugin routes + routes: { + ref1: plugin1RouteRef, + ref2: plugin2RouteRef, + ref3: subRouteRef1, + }, externalRoutes: { extRouteRef1, extRouteRef2, @@ -200,6 +210,9 @@ describe('Integration Test', () => { expect(screen.getByText('extLink2: /foo/a')).toBeInTheDocument(); expect(screen.getByText('extLink3: /sub1')).toBeInTheDocument(); expect(screen.getByText('extLink4: /foo/b')).toBeInTheDocument(); + + // Plugins should be discovered through element tree + expect(app.getPlugins()).toEqual([plugin1, plugin2]); }); it('runs happy paths without optional routes', async () => { @@ -245,6 +258,67 @@ describe('Integration Test', () => { expect(screen.getByText('extLink4: ')).toBeInTheDocument(); }); + it('should wait for the config to load before calling feature flags', async () => { + const storageFlags = new LocalStorageFeatureFlags(); + jest.spyOn(storageFlags, 'registerFlag'); + + const apis = [ + createApiFactory({ + api: featureFlagsApiRef, + deps: { configApi: configApiRef }, + factory() { + return storageFlags; + }, + }), + ]; + + const app = new PrivateAppImpl({ + apis, + defaultApis: [], + themes: [ + { + id: 'light', + title: 'Light Theme', + variant: 'light', + theme: lightTheme, + }, + ], + icons: defaultAppIcons, + plugins: [ + createPlugin({ + id: 'test', + register: p => p.featureFlags.register('name'), + }), + ], + components, + bindRoutes: ({ bind }) => { + bind(plugin1.externalRoutes, { + extRouteRef1: plugin1RouteRef, + extRouteRef2: plugin2RouteRef, + }); + }, + }); + + const Provider = app.getProvider(); + const Router = app.getRouter(); + + await renderWithEffects( + + + + + + + + , + ); + + expect(storageFlags.registerFlag).toHaveBeenCalledWith({ + name: 'name', + pluginId: 'test', + }); + }); + it('should throw some error when the route has duplicate params', () => { const app = new PrivateAppImpl({ apis: [], diff --git a/packages/app-api/src/app/App.tsx b/packages/app-api/src/app/App.tsx index bd6a8d6e31..abd6b5a8f4 100644 --- a/packages/app-api/src/app/App.tsx +++ b/packages/app-api/src/app/App.tsx @@ -14,10 +14,12 @@ * limitations under the License. */ +import { Config } from '@backstage/config'; import React, { ComponentType, PropsWithChildren, ReactElement, + useEffect, useMemo, useState, } from 'react'; @@ -44,6 +46,7 @@ import { identityApiRef, BackstagePlugin, RouteRef, + SubRouteRef, ExternalRouteRef, } from '@backstage/plugin-api'; import { ApiFactoryRegistry, ApiResolver } from '../apis/system'; @@ -52,6 +55,7 @@ import { routeElementDiscoverer, traverseElementTree, } from '../extensions/traversal'; +import { pluginCollector } from '../plugins/collectors'; import { routeObjectCollector, routeParentCollector, @@ -73,15 +77,13 @@ import { SignInResult, } from './types'; -export function generateBoundRoutes( - bindRoutes: AppOptions['bindRoutes'], -): Map { - const result = new Map(); +export function generateBoundRoutes(bindRoutes: AppOptions['bindRoutes']) { + const result = new Map(); if (bindRoutes) { const bind: AppRouteBinder = ( externalRoutes, - targetRoutes: { [name: string]: RouteRef }, + targetRoutes: { [name: string]: RouteRef | SubRouteRef }, ) => { for (const [key, value] of Object.entries(targetRoutes)) { const externalRoute = externalRoutes[key]; @@ -168,7 +170,7 @@ export class PrivateAppImpl implements BackstageApp { private readonly apis: Iterable; private readonly icons: NonNullable; - private readonly plugins: BackstagePlugin[]; + private readonly plugins: Set>; private readonly components: AppComponents; private readonly themes: AppTheme[]; private readonly configLoader?: AppConfigLoader; @@ -180,7 +182,7 @@ export class PrivateAppImpl implements BackstageApp { constructor(options: FullAppOptions) { this.apis = options.apis; this.icons = options.icons; - this.plugins = options.plugins; + this.plugins = new Set(options.plugins); this.components = options.components; this.themes = options.themes; this.configLoader = options.configLoader; @@ -189,7 +191,7 @@ export class PrivateAppImpl implements BackstageApp { } getPlugins(): BackstagePlugin[] { - return this.plugins; + return Array.from(this.plugins); } getSystemIcon(key: string): IconComponent | undefined { @@ -202,25 +204,6 @@ export class PrivateAppImpl implements BackstageApp { getProvider(): ComponentType<{}> { const appContext = new AppContextImpl(this); - const apiHolder = this.getApiHolder(); - - const featureFlagsApi = this.getApiHolder().get(featureFlagsApiRef)!; - - for (const plugin of this.plugins.values()) { - for (const output of plugin.output()) { - switch (output.type) { - case 'feature-flag': { - featureFlagsApi.registerFlag({ - name: output.name, - pluginId: plugin.getId(), - }); - break; - } - default: - break; - } - } - } const Provider = ({ children }: PropsWithChildren<{}>) => { const appThemeApi = useMemo( @@ -236,11 +219,22 @@ export class PrivateAppImpl implements BackstageApp { routePaths: routePathCollector, routeParents: routeParentCollector, routeObjects: routeObjectCollector, + collectedPlugins: pluginCollector, }, }); validateRoutes(result.routePaths, result.routeParents); + // TODO(Rugvip): Restructure the public API so that we can get an immediate view of + // the app, rather than having to wait for the provider to render. + // For now we need to push the additional plugins we find during + // collection and then make sure we initialize things afterwards. + result.collectedPlugins.forEach(plugin => this.plugins.add(plugin)); + this.verifyPlugins(this.plugins); + + // Initialize APIs once all plugins are available + this.getApiHolder(); + return result; }, [children]); @@ -250,15 +244,41 @@ export class PrivateAppImpl implements BackstageApp { appThemeApi, ); + const hasConfigApi = 'api' in loadedConfig; + if (hasConfigApi) { + const { api } = loadedConfig as { api: Config }; + this.configApi = api; + } + + useEffect(() => { + if (hasConfigApi) { + const featureFlagsApi = this.getApiHolder().get(featureFlagsApiRef)!; + + for (const plugin of this.plugins.values()) { + for (const output of plugin.output()) { + switch (output.type) { + case 'feature-flag': { + featureFlagsApi.registerFlag({ + name: output.name, + pluginId: plugin.getId(), + }); + break; + } + default: + break; + } + } + } + } + }, [hasConfigApi, loadedConfig]); + if ('node' in loadedConfig) { // Loading or error return loadedConfig.node; } - this.configApi = loadedConfig.api; - return ( - + ) { const pluginIds = new Set(); - for (const plugin of this.plugins) { + for (const plugin of plugins) { const id = plugin.getId(); if (pluginIds.has(id)) { throw new Error(`Duplicate plugin found '${id}'`); diff --git a/packages/app-api/src/app/createApp.tsx b/packages/app-api/src/app/createApp.tsx index 50fc9bc0f2..445e7495ef 100644 --- a/packages/app-api/src/app/createApp.tsx +++ b/packages/app-api/src/app/createApp.tsx @@ -128,7 +128,7 @@ export function createApp(options?: AppOptions) { ]; const configLoader = options?.configLoader ?? defaultConfigLoader; - const app = new PrivateAppImpl({ + return new PrivateAppImpl({ apis, icons, plugins, @@ -138,8 +138,4 @@ export function createApp(options?: AppOptions) { defaultApis, bindRoutes: options?.bindRoutes, }); - - app.verify(); - - return app; } diff --git a/packages/app-api/src/app/types.ts b/packages/app-api/src/app/types.ts index 089a682246..746248d736 100644 --- a/packages/app-api/src/app/types.ts +++ b/packages/app-api/src/app/types.ts @@ -29,7 +29,7 @@ import { AppConfig } from '@backstage/config'; import { AppIcons } from './icons'; export type BootErrorPageProps = { - step: 'load-config'; + step: 'load-config' | 'load-chunk'; error: Error; }; diff --git a/packages/app-api/src/extensions/extensions.tsx b/packages/app-api/src/extensions/extensions.tsx index 637ef59ff9..97c4ceb32b 100644 --- a/packages/app-api/src/extensions/extensions.tsx +++ b/packages/app-api/src/extensions/extensions.tsx @@ -21,6 +21,7 @@ import { BackstagePlugin, RouteRef, useRouteRef, + useApp, } from '@backstage/plugin-api'; type ComponentLoader = @@ -31,8 +32,11 @@ type ComponentLoader = sync: T; }; +// We do not use ComponentType as the return type, since it doesn't let us convey the children prop. +// ComponentType inserts children as an optional prop whether the inner component accepts it or not, +// making it impossible to make the usage of children type safe. export function createRoutableExtension< - T extends (props: any) => JSX.Element + T extends (props: any) => JSX.Element | null >(options: { component: () => Promise; mountPoint: RouteRef; @@ -41,22 +45,44 @@ export function createRoutableExtension< return createReactExtension({ component: { lazy: () => - component().then(InnerComponent => { - const RoutableExtensionWrapper = ((props: any) => { - // Validate that the routing is wired up correctly in the App.tsx - try { - useRouteRef(mountPoint); - } catch { - throw new Error( - 'Routable extension component was not discovered in the app element tree. ' + - 'Routable extension components may not be rendered by other components and must be ' + - 'directly available as an element within the App provider component.', - ); - } - return ; - }) as T; - return RoutableExtensionWrapper; - }), + component().then( + InnerComponent => { + const RoutableExtensionWrapper: any = (props: any) => { + // Validate that the routing is wired up correctly in the App.tsx + try { + useRouteRef(mountPoint); + } catch (error) { + if (error?.message.startsWith('No path for ')) { + throw new Error( + `Routable extension component with mount point ${mountPoint} was not discovered in the app element tree. ` + + 'Routable extension components may not be rendered by other components and must be ' + + 'directly available as an element within the App provider component.', + ); + } + throw error; + } + return ; + }; + + const componentName = + (InnerComponent as { displayName?: string }).displayName || + InnerComponent.name || + 'LazyComponent'; + + RoutableExtensionWrapper.displayName = `RoutableExtension(${componentName})`; + + return RoutableExtensionWrapper as T; + }, + error => { + const RoutableExtensionWrapper: any = (_: any) => { + const app = useApp(); + const { BootErrorPage } = app.getComponents(); + + return ; + }; + return RoutableExtensionWrapper; + }, + ), }, data: { 'core.mountPoint': mountPoint, @@ -64,15 +90,21 @@ export function createRoutableExtension< }); } +// We do not use ComponentType as the return type, since it doesn't let us convey the children prop. +// ComponentType inserts children as an optional prop whether the inner component accepts it or not, +// making it impossible to make the usage of children type safe. export function createComponentExtension< - T extends (props: any) => JSX.Element + T extends (props: any) => JSX.Element | null >(options: { component: ComponentLoader }): Extension { const { component } = options; return createReactExtension({ component }); } +// We do not use ComponentType as the return type, since it doesn't let us convey the children prop. +// ComponentType inserts children as an optional prop whether the inner component accepts it or not, +// making it impossible to make the usage of children type safe. export function createReactExtension< - T extends (props: any) => JSX.Element + T extends (props: any) => JSX.Element | null >(options: { component: ComponentLoader; data?: Record; diff --git a/packages/app-api/src/extensions/traversal.ts b/packages/app-api/src/extensions/traversal.ts index bdf02d16c8..4431bbd24c 100644 --- a/packages/app-api/src/extensions/traversal.ts +++ b/packages/app-api/src/extensions/traversal.ts @@ -37,7 +37,6 @@ export function traverseElementTree(options: { discoverers: Discoverer[]; collectors: { [name in keyof Results]: Collector }; }): Results { - const visited = new Set(); const collectors: { [name in string]: ReturnType>; } = {}; @@ -74,14 +73,6 @@ export function traverseElementTree(options: { if (!isValidElement(element)) { return; } - if (visited.has(element)) { - const anyType = element?.type as - | { displayName?: string; name?: string } - | undefined; - const name = anyType?.displayName || anyType?.name || String(anyType); - throw new Error(`Visited element ${name} twice`); - } - visited.add(element); const nextContexts: QueueItem['contexts'] = {}; diff --git a/packages/app-api/src/lib/AuthSessionManager/SessionStateTracker.ts b/packages/app-api/src/lib/AuthSessionManager/SessionStateTracker.ts index 390d6c7599..73eb4e7904 100644 --- a/packages/app-api/src/lib/AuthSessionManager/SessionStateTracker.ts +++ b/packages/app-api/src/lib/AuthSessionManager/SessionStateTracker.ts @@ -14,7 +14,7 @@ * limitations under the License. */ -import { BehaviorSubject } from '..'; +import { BehaviorSubject } from '../subjects'; import { SessionState } from '@backstage/plugin-api'; import { Observable } from '../../types'; diff --git a/packages/app-api/src/lib/subjects.ts b/packages/app-api/src/lib/subjects.ts index 9ebde6ebbc..5239e460af 100644 --- a/packages/app-api/src/lib/subjects.ts +++ b/packages/app-api/src/lib/subjects.ts @@ -53,6 +53,10 @@ export class PublishSubject ZenObservable.SubscriptionObserver >(); + [Symbol.observable]() { + return this; + } + get closed() { return this.isClosed; } @@ -148,6 +152,10 @@ export class BehaviorSubject ZenObservable.SubscriptionObserver >(); + [Symbol.observable]() { + return this; + } + get closed() { return this.isClosed; } diff --git a/packages/app-api/src/plugins/collectors.test.tsx b/packages/app-api/src/plugins/collectors.test.tsx new file mode 100644 index 0000000000..d205df9e8b --- /dev/null +++ b/packages/app-api/src/plugins/collectors.test.tsx @@ -0,0 +1,105 @@ +/* + * 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 React, { PropsWithChildren } from 'react'; +import { createPlugin, createRouteRef } from '@backstage/plugin-api'; +import { + createRoutableExtension, + createComponentExtension, +} from '../extensions'; +import { MemoryRouter, Routes, Route } from 'react-router-dom'; +import { + traverseElementTree, + childDiscoverer, + routeElementDiscoverer, +} from '../extensions/traversal'; +import { pluginCollector } from './collectors'; + +const mockConfig = () => ({ id: 'foo' }); +const MockComponent = ({ children }: PropsWithChildren<{ path?: string }>) => ( + <>{children} +); + +const pluginA = createPlugin({ id: 'my-plugin-a' }); +const pluginB = createPlugin({ id: 'my-plugin-b' }); +const pluginC = createPlugin({ id: 'my-plugin-c' }); + +const ref1 = createRouteRef(mockConfig()); +const ref2 = createRouteRef(mockConfig()); + +const Extension1 = pluginA.provide( + createRoutableExtension({ + component: () => Promise.resolve(MockComponent), + mountPoint: ref1, + }), +); +const Extension2 = pluginB.provide( + createRoutableExtension({ + component: () => Promise.resolve(MockComponent), + mountPoint: ref2, + }), +); +const Extension3 = pluginA.provide( + createComponentExtension({ component: { sync: MockComponent } }), +); +const Extension4 = pluginB.provide( + createComponentExtension({ component: { sync: MockComponent } }), +); +const Extension5 = pluginC.provide( + createComponentExtension({ component: { sync: MockComponent } }), +); + +describe('collection', () => { + it('should collect the plugins', () => { + const root = ( + + + +
+ +
+
+ {[]} + Some text here shouldn't be a problem +
+ {null} +
+ +
+ + {false} + {true} + {0} +
+ +
+ } /> +
+ + + ); + + const { plugins } = traverseElementTree({ + root, + discoverers: [childDiscoverer, routeElementDiscoverer], + collectors: { + plugins: pluginCollector, + }, + }); + + expect(plugins).toEqual(new Set([pluginA, pluginB, pluginC])); + }); +}); diff --git a/packages/app-api/src/plugins/collectors.ts b/packages/app-api/src/plugins/collectors.ts new file mode 100644 index 0000000000..10442b757b --- /dev/null +++ b/packages/app-api/src/plugins/collectors.ts @@ -0,0 +1,47 @@ +/* + * 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. + */ +/* + * 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 { BackstagePlugin } from '@backstage/plugin-api'; +import { getComponentData } from '../extensions'; +import { createCollector } from '../extensions/traversal'; + +export const pluginCollector = createCollector( + () => new Set>(), + (acc, node) => { + const plugin = getComponentData>( + node, + 'core.plugin', + ); + if (plugin) { + acc.add(plugin); + } + }, +); diff --git a/packages/app-api/src/plugins/index.ts b/packages/app-api/src/plugins/index.ts new file mode 100644 index 0000000000..95794a7487 --- /dev/null +++ b/packages/app-api/src/plugins/index.ts @@ -0,0 +1,17 @@ +/* + * Copyright 2021 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. + */ + +export { pluginCollector } from './collectors'; diff --git a/packages/app-api/src/routing/RouteResolver.test.ts b/packages/app-api/src/routing/RouteResolver.test.ts index 93daedac1e..0d8b7adc2a 100644 --- a/packages/app-api/src/routing/RouteResolver.test.ts +++ b/packages/app-api/src/routing/RouteResolver.test.ts @@ -23,9 +23,10 @@ import { SubRouteRef, } from '@backstage/plugin-api'; import { RouteResolver } from './RouteResolver'; +import { MATCH_ALL_ROUTE } from './collectors'; const element = () => null; -const rest = { element, caseSensitive: false }; +const rest = { element, caseSensitive: false, children: [MATCH_ALL_ROUTE] }; const ref1 = createRouteRef({ id: 'rr1' }); const ref2 = createRouteRef({ id: 'rr2', params: ['x'] }); @@ -111,6 +112,7 @@ describe('RouteResolver', () => { path: '/my-parent/:x', ...rest, children: [ + MATCH_ALL_ROUTE, { routeRefs: new Set([ref1]), path: '/my-route', ...rest }, ], }, @@ -142,6 +144,59 @@ describe('RouteResolver', () => { ); }); + it('should resolve the most specific match', () => { + const r = new RouteResolver( + new Map([ + [ref1, '/deep'], + [ref2, '/root/:x'], + [ref3, '/sub/:y'], + ]), + new Map([ + [ref3, ref2], + [ref1, ref3], + ]), + [ + { + routeRefs: new Set([ref2]), + path: '/root/:x', + ...rest, + children: [ + MATCH_ALL_ROUTE, + { + routeRefs: new Set([ref3]), + path: '/sub/:y', + ...rest, + children: [ + MATCH_ALL_ROUTE, + { + routeRefs: new Set([ref1]), + path: '/deep', + ...rest, + }, + ], + }, + ], + }, + ], + new Map(), + ); + + expect(r.resolve(ref2, '/')?.({ x: 'x' })).toBe('/root/x'); + expect(r.resolve(ref3, '/root/x')?.({ y: 'y' })).toBe('/root/x/sub/y'); + + expect(() => r.resolve(ref1, '/')?.()).toThrow( + /^Cannot route.*with parent.*as it has parameters$/, + ); + expect(() => r.resolve(ref1, '/root/x')?.()).toThrow( + /^Cannot route.*with parent.*as it has parameters$/, + ); + expect(r.resolve(ref1, '/root/x/sub/y')?.()).toBe('/root/x/sub/y/deep'); + // Without the MATCH_ALL_ROUTE, we wouldn't properly match the route here + expect(r.resolve(ref1, '/root/x/sub/y/any/nested/path/here')?.()).toBe( + '/root/x/sub/y/deep', + ); + }); + it('should resolve an absolute route with multiple parents', () => { const r = new RouteResolver( new Map([ @@ -159,11 +214,13 @@ describe('RouteResolver', () => { path: '/my-grandparent/:y', ...rest, children: [ + MATCH_ALL_ROUTE, { routeRefs: new Set([ref2]), path: '/my-parent/:x', ...rest, children: [ + MATCH_ALL_ROUTE, { routeRefs: new Set([ref1]), path: '/my-route', ...rest }, ], }, diff --git a/packages/app-api/src/routing/collectors.test.tsx b/packages/app-api/src/routing/collectors.test.tsx index 041e109eda..3fe1528ed0 100644 --- a/packages/app-api/src/routing/collectors.test.tsx +++ b/packages/app-api/src/routing/collectors.test.tsx @@ -88,13 +88,26 @@ function sortedEntries(map: Map): [RouteRef, T][] { ); } -function routeObj(path: string, refs: RouteRef[], children: any[] = []) { +function routeObj( + path: string, + refs: RouteRef[], + children: any[] = [], + type: 'mounted' | 'gathered' = 'mounted', +) { return { path: path, caseSensitive: false, - element: null, + element: type, routeRefs: new Set(refs), - children: children, + children: [ + { + path: '/*', + caseSensitive: false, + element: 'match-all', + routeRefs: new Set(), + }, + ...children, + ], }; } @@ -264,8 +277,12 @@ describe('discovery', () => { [ref5, ref3], ]); expect(routeObjects).toEqual([ - routeObj('/foo', [ref1, ref2]), - routeObj('/bar', [ref3], [routeObj('/baz', [ref4, ref5])]), + routeObj('/foo', [ref1, ref2], [], 'gathered'), + routeObj( + '/bar', + [ref3], + [routeObj('/baz', [ref4, ref5], [], 'gathered')], + ), ]); }); @@ -319,6 +336,7 @@ describe('discovery', () => { '/bar', [ref2, ref5], [routeObj('/baz', [ref3], [routeObj('/blop', [ref4])])], + 'gathered', ), ], ), @@ -351,24 +369,4 @@ describe('discovery', () => { }); }).toThrow('Mounted routable extension must have a path'); }); - - it('should not visit the same element twice', () => { - const element = ; - - expect(() => - traverseElementTree({ - root: ( - - {element} - {element} - - ), - discoverers: [childDiscoverer, routeElementDiscoverer], - collectors: { - routes: routePathCollector, - routeParents: routeParentCollector, - }, - }), - ).toThrow(`Visited element Extension(Component) twice`); - }); }); diff --git a/packages/app-api/src/routing/collectors.tsx b/packages/app-api/src/routing/collectors.tsx index d681ff2906..f74dd3cfd6 100644 --- a/packages/app-api/src/routing/collectors.tsx +++ b/packages/app-api/src/routing/collectors.tsx @@ -110,6 +110,17 @@ export const routeParentCollector = createCollector( }, ); +// 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 +// mount points that are as deep in the routing tree as possible. +export const MATCH_ALL_ROUTE: BackstageRouteObject = { + caseSensitive: false, + path: '/*', + element: 'match-all', // These elements aren't used, so we add in a bit of debug information + routeRefs: new Set(), +}; + export const routeObjectCollector = createCollector( () => Array(), (acc, node, parent, parentObj: BackstageRouteObject | undefined) => { @@ -127,9 +138,9 @@ export const routeObjectCollector = createCollector( const newObject: BackstageRouteObject = { caseSensitive, path, - element: null, + element: 'mounted', routeRefs: new Set([routeRef]), - children: [], + children: [MATCH_ALL_ROUTE], }; parentChildren.push(newObject); return newObject; @@ -149,9 +160,9 @@ export const routeObjectCollector = createCollector( const newObject: BackstageRouteObject = { caseSensitive, path, - element: null, + element: 'gathered', routeRefs: new Set(), - children: [], + children: [MATCH_ALL_ROUTE], }; parentChildren.push(newObject); return newObject; diff --git a/packages/app-api/src/types.ts b/packages/app-api/src/types.ts index 13a8a72570..ab0aa56a1b 100644 --- a/packages/app-api/src/types.ts +++ b/packages/app-api/src/types.ts @@ -39,9 +39,17 @@ export type Subscription = { /** * Value indicating whether the subscription is closed. */ - readonly closed: Boolean; + readonly closed: boolean; }; +// Declares the global well-known Symbol.observable +// We get the actual runtime polyfill from zen-observable +declare global { + interface SymbolConstructor { + readonly observable: symbol; + } +} + /** * Observable sequence of values and errors, see TC39. * @@ -51,12 +59,14 @@ export type Subscription = { * using many different observable implementations, such as zen-observable or RxJS 5. */ export type Observable = { + [Symbol.observable](): Observable; + /** * Subscribes to this observable to start receiving new values. */ subscribe(observer: Observer): Subscription; subscribe( - onNext: (value: T) => void, + onNext?: (value: T) => void, onError?: (error: Error) => void, onComplete?: () => void, ): Subscription; diff --git a/packages/plugin-api/src/apis/definitions/OAuthRequestApi.ts b/packages/plugin-api/src/apis/definitions/OAuthRequestApi.ts index 5d79fc0759..8b33ee394b 100644 --- a/packages/plugin-api/src/apis/definitions/OAuthRequestApi.ts +++ b/packages/plugin-api/src/apis/definitions/OAuthRequestApi.ts @@ -14,7 +14,7 @@ * limitations under the License. */ -import { IconComponent } from '../../icons'; +import { IconComponent } from '../../icons/types'; import { Observable } from '../../types'; import { ApiRef, createApiRef } from '../system'; diff --git a/packages/plugin-api/src/app/types.ts b/packages/plugin-api/src/app/types.ts index 7ed251e490..d779f1fadd 100644 --- a/packages/plugin-api/src/app/types.ts +++ b/packages/plugin-api/src/app/types.ts @@ -19,7 +19,7 @@ import { ProfileInfo } from '../apis/definitions'; import { IconComponent } from '../icons'; export type BootErrorPageProps = { - step: 'load-config'; + step: 'load-config' | 'load-chunk'; error: Error; }; diff --git a/packages/plugin-api/src/index.test.ts b/packages/plugin-api/src/index.test.ts index 7d716c7f91..9414a6b730 100644 --- a/packages/plugin-api/src/index.test.ts +++ b/packages/plugin-api/src/index.test.ts @@ -41,6 +41,7 @@ describe('index', () => { useApiHolder: expect.any(Function), useApp: expect.any(Function), useRouteRef: expect.any(Function), + useRouteRefParams: expect.any(Function), withApis: expect.any(Function), createApiRef: expect.any(Function), diff --git a/packages/plugin-api/src/plugin/types.ts b/packages/plugin-api/src/plugin/types.ts index fbaafd344c..55cac1cc6d 100644 --- a/packages/plugin-api/src/plugin/types.ts +++ b/packages/plugin-api/src/plugin/types.ts @@ -14,7 +14,7 @@ * limitations under the License. */ -import { RouteRef, ExternalRouteRef } from '../routing'; +import { RouteRef, SubRouteRef, ExternalRouteRef } from '../routing'; import { AnyApiFactory } from '../apis/system'; export type RouteOptions = { @@ -36,7 +36,7 @@ export type Extension = { expose(plugin: BackstagePlugin): T; }; -export type AnyRoutes = { [name: string]: RouteRef }; +export type AnyRoutes = { [name: string]: RouteRef | SubRouteRef }; export type AnyExternalRoutes = { [name: string]: ExternalRouteRef }; diff --git a/packages/plugin-api/src/routing/index.ts b/packages/plugin-api/src/routing/index.ts index 3ac657ee8e..4e272910ba 100644 --- a/packages/plugin-api/src/routing/index.ts +++ b/packages/plugin-api/src/routing/index.ts @@ -19,3 +19,4 @@ export { createRouteRef } from './RouteRef'; export { createSubRouteRef } from './SubRouteRef'; export { createExternalRouteRef } from './ExternalRouteRef'; export { useRouteRef } from './useRouteRef'; +export { useRouteRefParams } from './useRouteRefParams'; diff --git a/packages/plugin-api/src/routing/useRouteRefParams.test.tsx b/packages/plugin-api/src/routing/useRouteRefParams.test.tsx new file mode 100644 index 0000000000..7c847f95c6 --- /dev/null +++ b/packages/plugin-api/src/routing/useRouteRefParams.test.tsx @@ -0,0 +1,54 @@ +/* + * Copyright 2021 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 } from '@testing-library/react'; +import React from 'react'; +import { MemoryRouter, Route, Routes } from 'react-router-dom'; +import { useRouteRefParams } from './useRouteRefParams'; +import { createRouteRef } from './RouteRef'; + +describe('useRouteRefParams', () => { + it('should provide types params', () => { + const routeRef = createRouteRef({ + id: 'ref1', + params: ['a', 'b'], + }); + + const Page = () => { + const params: { a: string; b: string } = useRouteRefParams(routeRef); + + return ( +
+ {params.a} + {params.b} +
+ ); + }; + + const { getByText } = render( + + + + + + + , + ); + + expect(getByText('foo')).toBeInTheDocument(); + expect(getByText('bar')).toBeInTheDocument(); + }); +}); diff --git a/packages/plugin-api/src/routing/useRouteRefParams.ts b/packages/plugin-api/src/routing/useRouteRefParams.ts new file mode 100644 index 0000000000..7df44eedff --- /dev/null +++ b/packages/plugin-api/src/routing/useRouteRefParams.ts @@ -0,0 +1,24 @@ +/* + * Copyright 2021 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 { useParams } from 'react-router-dom'; +import { RouteRef, AnyParams, SubRouteRef } from './types'; + +export function useRouteRefParams( + _routeRef: RouteRef | SubRouteRef, +): Params { + return useParams() as Params; +}