From 765632fac76ef19dcaff6806a6914982660149b4 Mon Sep 17 00:00:00 2001 From: blam Date: Wed, 9 Jun 2021 12:05:04 +0200 Subject: [PATCH 01/24] feat: introduce collection of feature flags and make `FlatRoutes` treat feature flags as first class citizens MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Johan Haals Co-authored-by: Patrik Oldsberg Co-authored-by: Fredrik Adelöw Signed-off-by: blam --- packages/app/package.json | 2 ++ packages/app/src/App.tsx | 11 ++++--- packages/core-app-api/src/app/App.tsx | 10 ++++++- .../src/routing/FeatureFlagged.tsx | 29 +++++++++++++++++++ .../core-app-api/src/routing/FlatRoutes.tsx | 26 ++++++++++++++--- .../core-app-api/src/routing/collectors.tsx | 11 +++++++ packages/core-app-api/src/routing/index.ts | 1 + 7 files changed, 81 insertions(+), 9 deletions(-) create mode 100644 packages/core-app-api/src/routing/FeatureFlagged.tsx diff --git a/packages/app/package.json b/packages/app/package.json index 1b0b3aa22c..bfd2b458ae 100644 --- a/packages/app/package.json +++ b/packages/app/package.json @@ -8,6 +8,8 @@ "@backstage/cli": "^0.7.0", "@backstage/core": "^0.7.12", "@backstage/integration-react": "^0.1.3", + "@backstage/core-app-api": "^0.1.1", + "@backstage/core-components": "^0.1.1", "@backstage/plugin-api-docs": "^0.4.15", "@backstage/plugin-badges": "^0.2.2", "@backstage/plugin-catalog": "^0.6.2", diff --git a/packages/app/src/App.tsx b/packages/app/src/App.tsx index b14f9a3163..12cbe89f26 100644 --- a/packages/app/src/App.tsx +++ b/packages/app/src/App.tsx @@ -14,13 +14,12 @@ * limitations under the License. */ +import { createApp, FlatRoutes, FeatureFlagged } from '@backstage/core-app-api'; import { AlertDisplay, - createApp, - FlatRoutes, OAuthRequestDialog, SignInPage, -} from '@backstage/core'; +} from '@backstage/core-components'; import { apiDocsPlugin, ApiExplorerPage } from '@backstage/plugin-api-docs'; import { CatalogEntityPage, @@ -65,6 +64,7 @@ const app = createApp({ // Custom icon example alert: AlarmIcon, }, + components: { SignInPage: props => { return ( @@ -114,8 +114,11 @@ const routes = ( path="/tech-radar" element={} /> - } /> + + } /> + } /> + } /> } /> } /> diff --git a/packages/core-app-api/src/app/App.tsx b/packages/core-app-api/src/app/App.tsx index a5ba5c4bca..93b56bd965 100644 --- a/packages/core-app-api/src/app/App.tsx +++ b/packages/core-app-api/src/app/App.tsx @@ -57,6 +57,7 @@ import { } from '../extensions/traversal'; import { pluginCollector } from '../plugins/collectors'; import { + featureFlagCollector, routeObjectCollector, routeParentCollector, routePathCollector, @@ -224,6 +225,7 @@ export class PrivateAppImpl implements BackstageApp { routeParents: routeParentCollector, routeObjects: routeObjectCollector, collectedPlugins: pluginCollector, + featureFlags: featureFlagCollector, }, }); @@ -237,7 +239,13 @@ export class PrivateAppImpl implements BackstageApp { this.verifyPlugins(this.plugins); // Initialize APIs once all plugins are available - this.getApiHolder(); + const apiHolder = this.getApiHolder(); + + // Register feature flags that have been discovered + const featureFlagApi = apiHolder.get(featureFlagsApiRef)!; + for (const name of result.featureFlags) { + featureFlagApi.registerFlag({ name, pluginId: '' }); + } return result; }, [children]); diff --git a/packages/core-app-api/src/routing/FeatureFlagged.tsx b/packages/core-app-api/src/routing/FeatureFlagged.tsx new file mode 100644 index 0000000000..0e28988f17 --- /dev/null +++ b/packages/core-app-api/src/routing/FeatureFlagged.tsx @@ -0,0 +1,29 @@ +/* + * 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 React from 'react'; +import { featureFlagsApiRef, useApi } from '@backstage/core-plugin-api'; + +export type FeatureFlaggedProps = { + flag: string; + children: JSX.Element | null; +}; + +export const FeatureFlagged = ({ children, flag }: FeatureFlaggedProps) => { + const featureFlagApi = useApi(featureFlagsApiRef); + const isEnabled = featureFlagApi.isActive(flag); + return isEnabled ? children :
NAT ENABLED
; +}; diff --git a/packages/core-app-api/src/routing/FlatRoutes.tsx b/packages/core-app-api/src/routing/FlatRoutes.tsx index 54ef092965..9e3a0a9956 100644 --- a/packages/core-app-api/src/routing/FlatRoutes.tsx +++ b/packages/core-app-api/src/routing/FlatRoutes.tsx @@ -16,7 +16,13 @@ import React, { ReactNode, Children, isValidElement, Fragment } from 'react'; import { useRoutes } from 'react-router-dom'; -import { useApp } from '@backstage/core-plugin-api'; +import { + useApi, + useApp, + featureFlagsApiRef, + FeatureFlagsApi, +} from '@backstage/core-plugin-api'; +import { FeatureFlagged, FeatureFlaggedProps } from './FeatureFlagged'; type RouteObject = { path: string; @@ -26,7 +32,10 @@ type RouteObject = { // Similar to the same function from react-router, this collects routes from the // children, but only the first level of routes -function createRoutesFromChildren(childrenNode: ReactNode): RouteObject[] { +function createRoutesFromChildren( + childrenNode: ReactNode, + featureFlagsApi: FeatureFlagsApi, +): RouteObject[] { return Children.toArray(childrenNode).flatMap(child => { if (!isValidElement(child)) { return []; @@ -35,7 +44,15 @@ function createRoutesFromChildren(childrenNode: ReactNode): RouteObject[] { const { children } = child.props; if (child.type === Fragment) { - return createRoutesFromChildren(children); + return createRoutesFromChildren(children, featureFlagsApi); + } + + if (child.type === FeatureFlagged) { + const { flag } = child.props as FeatureFlaggedProps; + if (featureFlagsApi.isActive(flag)) { + return createRoutesFromChildren(children, featureFlagsApi); + } + return []; } let path = child.props.path as string | undefined; @@ -67,8 +84,9 @@ type FlatRoutesProps = { export const FlatRoutes = (props: FlatRoutesProps): JSX.Element | null => { const app = useApp(); + const featureFlagsApi = useApi(featureFlagsApiRef); const { NotFoundErrorPage } = app.getComponents(); - const routes = createRoutesFromChildren(props.children) + const routes = createRoutesFromChildren(props.children, featureFlagsApi) // Routes are sorted to work around a bug where prefixes are unexpectedly matched .sort((a, b) => b.path.localeCompare(a.path)) // We make sure all routes have '/*' appended, except '/' diff --git a/packages/core-app-api/src/routing/collectors.tsx b/packages/core-app-api/src/routing/collectors.tsx index e940a2cada..31c1ab6361 100644 --- a/packages/core-app-api/src/routing/collectors.tsx +++ b/packages/core-app-api/src/routing/collectors.tsx @@ -19,6 +19,7 @@ import { RouteRef } from '@backstage/core-plugin-api'; import { BackstageRouteObject } from './types'; import { getComponentData } from '../extensions'; import { createCollector } from '../extensions/traversal'; +import { FeatureFlagged, FeatureFlaggedProps } from './FeatureFlagged'; function getMountPoint(node: ReactElement): RouteRef | undefined { const element: ReactNode = node.props?.element; @@ -171,3 +172,13 @@ export const routeObjectCollector = createCollector( return parentObj; }, ); + +export const featureFlagCollector = createCollector( + () => new Set(), + (acc, node) => { + if (node.type === FeatureFlagged) { + const { flag } = node.props as FeatureFlaggedProps; + acc.add(flag); + } + }, +); diff --git a/packages/core-app-api/src/routing/index.ts b/packages/core-app-api/src/routing/index.ts index 7982333f4b..7e82eeb25d 100644 --- a/packages/core-app-api/src/routing/index.ts +++ b/packages/core-app-api/src/routing/index.ts @@ -15,3 +15,4 @@ */ export { FlatRoutes } from './FlatRoutes'; +export { FeatureFlagged, FeatureFlaggedProps } from './FeatureFlagged'; From 794af0de8e5664130c6c9875cbc82296be16d0dc Mon Sep 17 00:00:00 2001 From: blam Date: Wed, 9 Jun 2021 15:30:15 +0200 Subject: [PATCH 02/24] feat: more work with the nice API MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Patrik Oldsberg Co-authored-by: Fredrik Adelöw Co-authored-by: Johan Haals Signed-off-by: blam --- .../src/extensions/pennywise.ts | 97 +++++++++++++++++++ .../components/EntityLayout/EntityLayout.tsx | 64 ++++++------ 2 files changed, 125 insertions(+), 36 deletions(-) create mode 100644 packages/core-plugin-api/src/extensions/pennywise.ts diff --git a/packages/core-plugin-api/src/extensions/pennywise.ts b/packages/core-plugin-api/src/extensions/pennywise.ts new file mode 100644 index 0000000000..4574bafb3b --- /dev/null +++ b/packages/core-plugin-api/src/extensions/pennywise.ts @@ -0,0 +1,97 @@ +/* + * 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 React from 'react'; +import { getComponentData } from './componentData'; + +/** + * Returns an array of each component data value for a given key of each + * element in the entire react element tree starting at the provided children. + * + * - This was needed to grab the actual component data once we had narrowed down the set of children + */ +export const useCollectComponentData = ( + children: React.ReactNode, + componentDataKey: string, +) => { + const stack = [children]; + const found: T[] = []; + + while (stack.length) { + const current: React.ReactNode = stack.pop()!; + + React.Children.forEach(current, child => { + if (!React.isValidElement(child)) { + return; + } + + const data = getComponentData(child, componentDataKey); + if (data) { + found.push(data); + } + + if (child.props.children) { + stack.push(child.props.children); + } + }); + } + + return found; +}; + +/** + * Returns an array of all values of the children prop of each element with the entire + * react element tree that has component data for the given key. + * + * - this was needed to collect the children of ScaffolderFieldExtensions elements + */ +export const useCollectChildren = ( + component: React.ReactNode, + componentDataKey: string, +) => { + const stack = [component]; + const found: React.ReactNode[] = []; + + while (stack.length) { + const current: React.ReactNode = stack.pop()!; + + React.Children.forEach(current, child => { + if (!React.isValidElement(child)) { + return; + } + + if (child.props.children) { + if (getComponentData(child, componentDataKey)) { + found.push(child.props.children); + } + stack.push(child.props.children); + } + }); + } + + return found; +}; + +/** + * + * + * TODO: + * support: + * - entity layout route traversal + * - scaffolder field extension enumeration + * - FlatRoutes + * - Respecting feature flags + */ +export function useElementCollection(children: ReactNode) {} diff --git a/plugins/catalog/src/components/EntityLayout/EntityLayout.tsx b/plugins/catalog/src/components/EntityLayout/EntityLayout.tsx index dfd27631f8..68b254ec59 100644 --- a/plugins/catalog/src/components/EntityLayout/EntityLayout.tsx +++ b/plugins/catalog/src/components/EntityLayout/EntityLayout.tsx @@ -28,6 +28,10 @@ import { Page, Progress, RoutedTabs, + FeatureFlagsApi, + getComponentData, + featureFlagsApiRef, + useApi, } from '@backstage/core'; import { EntityContext, @@ -58,46 +62,14 @@ type SubRoute = { tabProps?: TabProps; }; +const dataKey = 'plugin.catalog.entityLayoutRoute'; + const Route: (props: SubRoute) => null = () => null; +attachComponentData(Route, dataKey, true); // This causes all mount points that are discovered within this route to use the path of the route itself attachComponentData(Route, 'core.gatherMountPoints', true); -function createSubRoutesFromChildren( - childrenProps: React.ReactNode, - entity: Entity | undefined, -): SubRoute[] { - // Directly comparing child.type with Route will not work with in - // combination with react-hot-loader in storybook - // https://github.com/gaearon/react-hot-loader/issues/304 - const routeType = ( - -
- - ).type; - - return Children.toArray(childrenProps).flatMap(child => { - if (!isValidElement(child)) { - return []; - } - - if (child.type === Fragment) { - return createSubRoutesFromChildren(child.props.children, entity); - } - - if (child.type !== routeType) { - throw new Error('Child of EntityLayout must be an EntityLayout.Route'); - } - - const { path, title, children, if: condition, tabProps } = child.props; - if (condition && entity && !condition(entity)) { - return []; - } - - return [{ path, title, children, tabProps }]; - }); -} - const EntityLayoutTitle = ({ entity, title, @@ -195,7 +167,27 @@ export const EntityLayout = ({ const { kind, namespace, name } = useEntityCompoundName(); const { entity, loading, error } = useContext(EntityContext); - const routes = createSubRoutesFromChildren(children, entity); + const routes = useElementCollection(children) + .findByComponentData({ + key: dataKey, + withStrictError: 'Child of EntityLayout must be an EntityLayout.Route', + }) + .listElements() // all nodes, element data, maintain structure or not? + .flatMap(({ props }) => { + if (props.condition && entity && !props.condition(entity)) { + return []; + } + + return [ + { + path: props.path, + title: props.title, + children: props.children, + tabProps: props.tabProps, + }, + ]; + }); + const { headerTitle, headerType } = headerProps( kind, namespace, From d7f0b0bed6ae3789f8a7fc281acef74731033ea9 Mon Sep 17 00:00:00 2001 From: blam Date: Wed, 9 Jun 2021 17:00:12 +0200 Subject: [PATCH 03/24] chore: working through beginning API Signed-off-by: blam --- .../app/src/components/catalog/EntityPage.tsx | 17 +++++++---- packages/core-app-api/src/app/App.tsx | 2 +- .../src/routing/FeatureFlagged.tsx | 11 +++++-- .../src/extensions/pennywise.ts | 19 ++++++++++-- plugins/scaffolder/src/components/Router.tsx | 30 +++++++++++++------ 5 files changed, 57 insertions(+), 22 deletions(-) diff --git a/packages/app/src/components/catalog/EntityPage.tsx b/packages/app/src/components/catalog/EntityPage.tsx index f646992c97..61d62136de 100644 --- a/packages/app/src/components/catalog/EntityPage.tsx +++ b/packages/app/src/components/catalog/EntityPage.tsx @@ -108,6 +108,7 @@ import { isTravisciAvailable, } from '@roadiehq/backstage-plugin-travis-ci'; import { EntityCodeCoverageContent } from '@backstage/plugin-code-coverage'; +import { FeatureFlagged } from '@backstage/core-app-api'; const EntityLayoutWrapper = (props: { children?: ReactNode }) => { const [badgesDialogOpen, setBadgesDialogOpen] = useState(false); @@ -282,9 +283,11 @@ const serviceEntityPage = ( {overviewContent} - - {cicdContent} - + + + {cicdContent} + + {errorsContent} @@ -303,9 +306,11 @@ const serviceEntityPage = ( - - - + + + + + diff --git a/packages/core-app-api/src/app/App.tsx b/packages/core-app-api/src/app/App.tsx index 93b56bd965..1d77282635 100644 --- a/packages/core-app-api/src/app/App.tsx +++ b/packages/core-app-api/src/app/App.tsx @@ -244,7 +244,7 @@ export class PrivateAppImpl implements BackstageApp { // Register feature flags that have been discovered const featureFlagApi = apiHolder.get(featureFlagsApiRef)!; for (const name of result.featureFlags) { - featureFlagApi.registerFlag({ name, pluginId: '' }); + featureFlagApi.registerFlag({ name, pluginId: '' }); } return result; diff --git a/packages/core-app-api/src/routing/FeatureFlagged.tsx b/packages/core-app-api/src/routing/FeatureFlagged.tsx index 0e28988f17..0adda78842 100644 --- a/packages/core-app-api/src/routing/FeatureFlagged.tsx +++ b/packages/core-app-api/src/routing/FeatureFlagged.tsx @@ -14,8 +14,11 @@ * limitations under the License. */ -import React from 'react'; -import { featureFlagsApiRef, useApi } from '@backstage/core-plugin-api'; +import { + featureFlagsApiRef, + useApi, + attachComponentData, +} from '@backstage/core-plugin-api'; export type FeatureFlaggedProps = { flag: string; @@ -25,5 +28,7 @@ export type FeatureFlaggedProps = { export const FeatureFlagged = ({ children, flag }: FeatureFlaggedProps) => { const featureFlagApi = useApi(featureFlagsApiRef); const isEnabled = featureFlagApi.isActive(flag); - return isEnabled ? children :
NAT ENABLED
; + return isEnabled ? children : null; }; + +attachComponentData(FeatureFlagged, 'core.featureFlagged', true); diff --git a/packages/core-plugin-api/src/extensions/pennywise.ts b/packages/core-plugin-api/src/extensions/pennywise.ts index 4574bafb3b..ce365a5a0f 100644 --- a/packages/core-plugin-api/src/extensions/pennywise.ts +++ b/packages/core-plugin-api/src/extensions/pennywise.ts @@ -13,7 +13,7 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -import React from 'react'; +import React, { ReactNode } from 'react'; import { getComponentData } from './componentData'; /** @@ -84,7 +84,7 @@ export const useCollectChildren = ( return found; }; -/** +/* * * * TODO: @@ -94,4 +94,17 @@ export const useCollectChildren = ( * - FlatRoutes * - Respecting feature flags */ -export function useElementCollection(children: ReactNode) {} + +class ElementCollection { + constructor(private readonly children: ReactNode) {} + findByComponentData(query: { key: string; withStrictError?: string }) { + const next = applyFilterStuff(this.children); + return new ElementCollection(next); + } + listComponentData(query: { key: string }): T[] {} + // listElements +} + +export function useElementCollection(children: ReactNode) { + return new ElementCollection(children); +} diff --git a/plugins/scaffolder/src/components/Router.tsx b/plugins/scaffolder/src/components/Router.tsx index 5ac663c7ea..ba062f62f7 100644 --- a/plugins/scaffolder/src/components/Router.tsx +++ b/plugins/scaffolder/src/components/Router.tsx @@ -32,16 +32,28 @@ import { collectComponentData, collectChildren } from '../extensions/helpers'; export const Router = () => { const outlet = useOutlet(); - const fieldExtensions = useMemo(() => { - const registeredExtensions = collectComponentData( - collectChildren(outlet, FIELD_EXTENSION_WRAPPER_KEY).flat(), - FIELD_EXTENSION_KEY, - ); + const foundExtensions = useElementCollection(outlet) + .findByComponentData({ + key: FIELD_EXTENSION_WRAPPER_KEY, + }) + .findByComponentData({ + key: FIELD_EXTENSION_KEY, + }) + .listComponentData(); - return registeredExtensions.length - ? registeredExtensions - : DEFAULT_SCAFFOLDER_FIELD_EXTENSIONS; - }, [outlet]); + const fieldExtensions = foundExtensions.length + ? foundExtensions + : DEFAULT_SCAFFOLDER_FIELD_EXTENSIONS; + // const fieldExtensions = useMemo(() => { + // const registeredExtensions = collectComponentData( + // collectChildren(outlet, FIELD_EXTENSION_WRAPPER_KEY).flat(), + // FIELD_EXTENSION_KEY, + // ); + + // return registeredExtensions.length + // ? registeredExtensions + // : DEFAULT_SCAFFOLDER_FIELD_EXTENSIONS; + // }, [outlet]); return ( From 78ccf9ea9831e352e9e79ef5906d4b5d664426bc Mon Sep 17 00:00:00 2001 From: blam Date: Thu, 10 Jun 2021 11:54:14 +0200 Subject: [PATCH 04/24] feat: replace implementations of children introspection with our new thing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Patrik Oldsberg Co-authored-by: Fredrik Adelöw Co-authored-by: Johan Haals Signed-off-by: blam --- .../core-api/src/routing/collectors.test.tsx | 2 +- .../core-app-api/src/routing/FlatRoutes.tsx | 88 ++++------ packages/core-app-api/src/routing/index.ts | 3 +- .../core-plugin-api/src/extensions/index.ts | 1 + .../src/extensions/pennywise.ts | 160 +++++++++--------- .../components/EntityLayout/EntityLayout.tsx | 18 +- plugins/scaffolder/src/components/Router.tsx | 17 +- .../src/extensions/helpers.test.tsx | 107 ------------ plugins/scaffolder/src/extensions/helpers.ts | 73 -------- 9 files changed, 121 insertions(+), 348 deletions(-) delete mode 100644 plugins/scaffolder/src/extensions/helpers.test.tsx delete mode 100644 plugins/scaffolder/src/extensions/helpers.ts diff --git a/packages/core-api/src/routing/collectors.test.tsx b/packages/core-api/src/routing/collectors.test.tsx index 498d1dd9fc..4441d4abf3 100644 --- a/packages/core-api/src/routing/collectors.test.tsx +++ b/packages/core-api/src/routing/collectors.test.tsx @@ -38,7 +38,7 @@ const MockComponent = ({ children }: PropsWithChildren<{ path?: string }>) => ( const plugin = createPlugin({ id: 'my-plugin' }); -const ref1 = createRouteRef({ path: '/foo1', title: 'Foo' }); +/const ref1 = createRouteRef({ path: '/foo1', title: 'Foo' }); const ref2 = createRouteRef({ path: '/foo2', title: 'Foo' }); const ref3 = createRouteRef({ path: '/foo3', title: 'Foo' }); const ref4 = createRouteRef({ path: '/foo4', title: 'Foo' }); diff --git a/packages/core-app-api/src/routing/FlatRoutes.tsx b/packages/core-app-api/src/routing/FlatRoutes.tsx index 9e3a0a9956..c185373dc1 100644 --- a/packages/core-app-api/src/routing/FlatRoutes.tsx +++ b/packages/core-app-api/src/routing/FlatRoutes.tsx @@ -14,79 +14,49 @@ * limitations under the License. */ -import React, { ReactNode, Children, isValidElement, Fragment } from 'react'; +import React, { ReactNode } from 'react'; import { useRoutes } from 'react-router-dom'; -import { - useApi, - useApp, - featureFlagsApiRef, - FeatureFlagsApi, -} from '@backstage/core-plugin-api'; -import { FeatureFlagged, FeatureFlaggedProps } from './FeatureFlagged'; +import { useApp, useElementCollection } from '@backstage/core-plugin-api'; type RouteObject = { path: string; - element: JSX.Element; + element: ReactNode; children?: RouteObject[]; }; -// Similar to the same function from react-router, this collects routes from the -// children, but only the first level of routes -function createRoutesFromChildren( - childrenNode: ReactNode, - featureFlagsApi: FeatureFlagsApi, -): RouteObject[] { - return Children.toArray(childrenNode).flatMap(child => { - if (!isValidElement(child)) { - return []; - } - - const { children } = child.props; - - if (child.type === Fragment) { - return createRoutesFromChildren(children, featureFlagsApi); - } - - if (child.type === FeatureFlagged) { - const { flag } = child.props as FeatureFlaggedProps; - if (featureFlagsApi.isActive(flag)) { - return createRoutesFromChildren(children, featureFlagsApi); - } - return []; - } - - let path = child.props.path as string | undefined; - - // TODO(Rugvip): Work around plugins registering empty paths, remove once deprecated routes are gone - if (path === '') { - return []; - } - path = path?.replace(/\/\*$/, '') ?? '/'; - - return [ - { - path, - element: child, - children: children && [ - { - path: '/*', - element: children, - }, - ], - }, - ]; - }); -} - type FlatRoutesProps = { children: ReactNode; }; export const FlatRoutes = (props: FlatRoutesProps): JSX.Element | null => { const app = useApp(); - const featureFlagsApi = useApi(featureFlagsApiRef); const { NotFoundErrorPage } = app.getComponents(); - const routes = createRoutesFromChildren(props.children, featureFlagsApi) + const routes = useElementCollection(props.children) + .listElements<{ path?: string; children: ReactNode }>() + .flatMap(child => { + let path = child.props.path; + + // TODO(Rugvip): Work around plugins registering empty paths, remove once deprecated routes are gone + if (path === '') { + return []; + } + path = path?.replace(/\/\*$/, '') ?? '/'; + + return [ + { + path, + element: child, + children: child.props.children + ? [ + { + path: '/*', + element: child.props.children, + }, + ] + : undefined, + }, + ]; + }) // Routes are sorted to work around a bug where prefixes are unexpectedly matched .sort((a, b) => b.path.localeCompare(a.path)) // We make sure all routes have '/*' appended, except '/' diff --git a/packages/core-app-api/src/routing/index.ts b/packages/core-app-api/src/routing/index.ts index 7e82eeb25d..b37b51e919 100644 --- a/packages/core-app-api/src/routing/index.ts +++ b/packages/core-app-api/src/routing/index.ts @@ -15,4 +15,5 @@ */ export { FlatRoutes } from './FlatRoutes'; -export { FeatureFlagged, FeatureFlaggedProps } from './FeatureFlagged'; +export { FeatureFlagged } from './FeatureFlagged'; +export type { FeatureFlaggedProps } from './FeatureFlagged'; diff --git a/packages/core-plugin-api/src/extensions/index.ts b/packages/core-plugin-api/src/extensions/index.ts index 26a0c597b1..8d2d6872d9 100644 --- a/packages/core-plugin-api/src/extensions/index.ts +++ b/packages/core-plugin-api/src/extensions/index.ts @@ -20,3 +20,4 @@ export { createRoutableExtension, createComponentExtension, } from './extensions'; +export { useElementCollection } from './pennywise'; diff --git a/packages/core-plugin-api/src/extensions/pennywise.ts b/packages/core-plugin-api/src/extensions/pennywise.ts index ce365a5a0f..d6d92a267e 100644 --- a/packages/core-plugin-api/src/extensions/pennywise.ts +++ b/packages/core-plugin-api/src/extensions/pennywise.ts @@ -13,98 +13,100 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -import React, { ReactNode } from 'react'; +import { + Children, + Fragment, + isValidElement, + ReactNode, + ReactElement, +} from 'react'; import { getComponentData } from './componentData'; +import { useApi, FeatureFlagsApi, featureFlagsApiRef } from '../apis'; -/** - * Returns an array of each component data value for a given key of each - * element in the entire react element tree starting at the provided children. - * - * - This was needed to grab the actual component data once we had narrowed down the set of children - */ -export const useCollectComponentData = ( - children: React.ReactNode, - componentDataKey: string, -) => { - const stack = [children]; - const found: T[] = []; +function selectChildren( + rootNode: ReactNode, + featureFlagsApi: FeatureFlagsApi, + selector?: (element: ReactElement) => boolean, + strictError?: string, +): Array> { + return Children.toArray(rootNode).flatMap(node => { + if (!isValidElement(node)) { + return []; + } - while (stack.length) { - const current: React.ReactNode = stack.pop()!; + const { children } = node.props; - React.Children.forEach(current, child => { - if (!React.isValidElement(child)) { - return; + if (node.type === Fragment) { + return selectChildren(children, featureFlagsApi, selector, strictError); + } + + if (getComponentData(node, 'core.featureFlagged')) { + const { flag } = node.props as { flag: string }; + if (featureFlagsApi.isActive(flag)) { + return selectChildren( + node.props.children, + featureFlagsApi, + selector, + strictError, + ); } + return []; + } - const data = getComponentData(child, componentDataKey); - if (data) { - found.push(data); - } + if (selector === undefined || selector(node)) { + return [node]; + } - if (child.props.children) { - stack.push(child.props.children); - } - }); - } + if (strictError) { + throw new Error(strictError); + } - return found; -}; - -/** - * Returns an array of all values of the children prop of each element with the entire - * react element tree that has component data for the given key. - * - * - this was needed to collect the children of ScaffolderFieldExtensions elements - */ -export const useCollectChildren = ( - component: React.ReactNode, - componentDataKey: string, -) => { - const stack = [component]; - const found: React.ReactNode[] = []; - - while (stack.length) { - const current: React.ReactNode = stack.pop()!; - - React.Children.forEach(current, child => { - if (!React.isValidElement(child)) { - return; - } - - if (child.props.children) { - if (getComponentData(child, componentDataKey)) { - found.push(child.props.children); - } - stack.push(child.props.children); - } - }); - } - - return found; -}; - -/* - * - * - * TODO: - * support: - * - entity layout route traversal - * - scaffolder field extension enumeration - * - FlatRoutes - * - Respecting feature flags - */ + return selectChildren( + node.props.children, + featureFlagsApi, + selector, + strictError, + ); + }); +} class ElementCollection { - constructor(private readonly children: ReactNode) {} + constructor( + private readonly children: ReactNode, + private readonly featureFlagsApi: FeatureFlagsApi, + ) {} + findByComponentData(query: { key: string; withStrictError?: string }) { - const next = applyFilterStuff(this.children); - return new ElementCollection(next); + const selection = selectChildren( + this.children, + this.featureFlagsApi, + node => Boolean(getComponentData(node, query.key)), + query.withStrictError, + ); + return new ElementCollection(selection, this.featureFlagsApi); + } + + listComponentData(query: { key: string }): T[] { + const selection = selectChildren( + this.children, + this.featureFlagsApi, + node => Boolean(getComponentData(node, query.key)), + ); + return selection + .map(node => getComponentData(node, query.key)) + .filter((data: T | undefined): data is T => Boolean(data)); + } + + listElements(): Array< + ReactElement + > { + return selectChildren(this.children, this.featureFlagsApi) as Array< + ReactElement + >; } - listComponentData(query: { key: string }): T[] {} - // listElements } export function useElementCollection(children: ReactNode) { - return new ElementCollection(children); + const featureFlagsApi = useApi(featureFlagsApiRef); + return new ElementCollection(children, featureFlagsApi); } diff --git a/plugins/catalog/src/components/EntityLayout/EntityLayout.tsx b/plugins/catalog/src/components/EntityLayout/EntityLayout.tsx index 68b254ec59..7cd4ab3dd1 100644 --- a/plugins/catalog/src/components/EntityLayout/EntityLayout.tsx +++ b/plugins/catalog/src/components/EntityLayout/EntityLayout.tsx @@ -28,11 +28,8 @@ import { Page, Progress, RoutedTabs, - FeatureFlagsApi, - getComponentData, - featureFlagsApiRef, - useApi, } from '@backstage/core'; +import { useElementCollection } from '@backstage/core-plugin-api'; import { EntityContext, EntityRefLinks, @@ -41,14 +38,7 @@ import { } from '@backstage/plugin-catalog-react'; import { Box, TabProps } from '@material-ui/core'; import { Alert } from '@material-ui/lab'; -import { - Children, - default as React, - Fragment, - isValidElement, - useContext, - useState, -} from 'react'; +import React, { useContext, useState } from 'react'; import { useNavigate } from 'react-router'; import { EntityContextMenu } from '../EntityContextMenu/EntityContextMenu'; import { FavouriteEntity } from '../FavouriteEntity/FavouriteEntity'; @@ -172,9 +162,9 @@ export const EntityLayout = ({ key: dataKey, withStrictError: 'Child of EntityLayout must be an EntityLayout.Route', }) - .listElements() // all nodes, element data, maintain structure or not? + .listElements() // all nodes, element data, maintain structure or not? .flatMap(({ props }) => { - if (props.condition && entity && !props.condition(entity)) { + if (props.if && entity && !props.if(entity)) { return []; } diff --git a/plugins/scaffolder/src/components/Router.tsx b/plugins/scaffolder/src/components/Router.tsx index ba062f62f7..57efd43e74 100644 --- a/plugins/scaffolder/src/components/Router.tsx +++ b/plugins/scaffolder/src/components/Router.tsx @@ -27,7 +27,7 @@ import { FIELD_EXTENSION_KEY, DEFAULT_SCAFFOLDER_FIELD_EXTENSIONS, } from '../extensions'; -import { collectComponentData, collectChildren } from '../extensions/helpers'; +import { useElementCollection } from '@backstage/core-plugin-api'; export const Router = () => { const outlet = useOutlet(); @@ -36,24 +36,13 @@ export const Router = () => { .findByComponentData({ key: FIELD_EXTENSION_WRAPPER_KEY, }) - .findByComponentData({ + .listComponentData({ key: FIELD_EXTENSION_KEY, - }) - .listComponentData(); + }); const fieldExtensions = foundExtensions.length ? foundExtensions : DEFAULT_SCAFFOLDER_FIELD_EXTENSIONS; - // const fieldExtensions = useMemo(() => { - // const registeredExtensions = collectComponentData( - // collectChildren(outlet, FIELD_EXTENSION_WRAPPER_KEY).flat(), - // FIELD_EXTENSION_KEY, - // ); - - // return registeredExtensions.length - // ? registeredExtensions - // : DEFAULT_SCAFFOLDER_FIELD_EXTENSIONS; - // }, [outlet]); return ( diff --git a/plugins/scaffolder/src/extensions/helpers.test.tsx b/plugins/scaffolder/src/extensions/helpers.test.tsx deleted file mode 100644 index ed3b74a210..0000000000 --- a/plugins/scaffolder/src/extensions/helpers.test.tsx +++ /dev/null @@ -1,107 +0,0 @@ -/* - * 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 React from 'react'; -import { collectComponentData, collectChildren } from './helpers'; -import { attachComponentData } from '@backstage/core'; - -describe('Extension Helpers', () => { - const createElementWithComponentData = ({ - type, - data, - }: { - type: string; - data: any; - }) => { - const element: React.ComponentType = () => null; - attachComponentData(element, type, data); - return element; - }; - - describe('collectChildren', () => { - it('should return the children of the component which has the correct componentData flag', () => { - const SearchElement = createElementWithComponentData({ - type: 'find.me', - data: {}, - }); - - const DontCareAboutme = createElementWithComponentData({ - type: 'dont.find.me', - data: {}, - }); - - const child1 = ( -
- hello -
- ); - - const child2 = ( -
-

Hello2

-
- ); - - const testCase = ( -
- - {child1} - {child1} - - {child2} - -

Hello!

- {child1} - -
- ); - - const children = collectChildren(testCase, 'find.me'); - - expect(children).toEqual([[child1, child1], child2, child1]); - }); - }); - - describe('collectComponentData', () => { - it('should return the componentData for particular nodes', () => { - const componentData1 = { help: 'im something' }; - const componentData2 = { help: 'im something else' }; - - const FirstElement = createElementWithComponentData({ - type: 'find.me', - data: componentData1, - }); - - const SecondElement = createElementWithComponentData({ - type: 'dont.find.me', - data: componentData2, - }); - - const testCase = [ - , - , - , - , - ]; - const returnedData = collectComponentData(testCase, 'find.me'); - - expect(returnedData).toEqual([ - componentData1, - componentData1, - componentData1, - ]); - }); - }); -}); diff --git a/plugins/scaffolder/src/extensions/helpers.ts b/plugins/scaffolder/src/extensions/helpers.ts deleted file mode 100644 index 2eac9780e5..0000000000 --- a/plugins/scaffolder/src/extensions/helpers.ts +++ /dev/null @@ -1,73 +0,0 @@ -/* - * 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 React from 'react'; -import { getComponentData } from '@backstage/core'; - -export const collectComponentData = ( - children: React.ReactNode, - componentDataKey: string, -) => { - const stack = [children]; - const found: T[] = []; - - while (stack.length) { - const current: React.ReactNode = stack.pop()!; - - React.Children.forEach(current, child => { - if (!React.isValidElement(child)) { - return; - } - - const data = getComponentData(child, componentDataKey); - if (data) { - found.push(data); - } - - if (child.props.children) { - stack.push(child.props.children); - } - }); - } - - return found; -}; - -export const collectChildren = ( - component: React.ReactNode, - componentDataKey: string, -) => { - const stack = [component]; - const found: React.ReactNode[] = []; - - while (stack.length) { - const current: React.ReactNode = stack.pop()!; - - React.Children.forEach(current, child => { - if (!React.isValidElement(child)) { - return; - } - - if (child.props.children) { - if (getComponentData(child, componentDataKey)) { - found.push(child.props.children); - } - stack.push(child.props.children); - } - }); - } - - return found; -}; From 969c6750affd37412055dda7798b6cb51899baa6 Mon Sep 17 00:00:00 2001 From: blam Date: Thu, 10 Jun 2021 17:01:26 +0200 Subject: [PATCH 05/24] feat: writing some tests and have a nicer api now MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Patrik Oldsberg Co-authored-by: Fredrik Adelöw Co-authored-by: Johan Haals Signed-off-by: blam --- .../core-api/src/routing/collectors.test.tsx | 2 +- packages/core-plugin-api/package.json | 1 + .../src/extensions/componentData.tsx | 2 + .../core-plugin-api/src/extensions/index.ts | 2 +- .../src/extensions/useElementFilter.test.tsx | 249 ++++++++++++++++++ .../{pennywise.ts => useElementFilter.tsx} | 38 +-- plugins/scaffolder/package.json | 7 +- plugins/scaffolder/src/components/Router.tsx | 23 +- 8 files changed, 295 insertions(+), 29 deletions(-) create mode 100644 packages/core-plugin-api/src/extensions/useElementFilter.test.tsx rename packages/core-plugin-api/src/extensions/{pennywise.ts => useElementFilter.tsx} (72%) diff --git a/packages/core-api/src/routing/collectors.test.tsx b/packages/core-api/src/routing/collectors.test.tsx index 4441d4abf3..498d1dd9fc 100644 --- a/packages/core-api/src/routing/collectors.test.tsx +++ b/packages/core-api/src/routing/collectors.test.tsx @@ -38,7 +38,7 @@ const MockComponent = ({ children }: PropsWithChildren<{ path?: string }>) => ( const plugin = createPlugin({ id: 'my-plugin' }); -/const ref1 = createRouteRef({ path: '/foo1', title: 'Foo' }); +const ref1 = createRouteRef({ path: '/foo1', title: 'Foo' }); const ref2 = createRouteRef({ path: '/foo2', title: 'Foo' }); const ref3 = createRouteRef({ path: '/foo3', title: 'Foo' }); const ref4 = createRouteRef({ path: '/foo4', title: 'Foo' }); diff --git a/packages/core-plugin-api/package.json b/packages/core-plugin-api/package.json index 816727ed9e..b1d53e17c0 100644 --- a/packages/core-plugin-api/package.json +++ b/packages/core-plugin-api/package.json @@ -42,6 +42,7 @@ }, "devDependencies": { "@backstage/cli": "^0.7.0", + "@backstage/core-app-api": "^0.1.1", "@backstage/test-utils": "^0.1.13", "@backstage/test-utils-core": "^0.1.1", "@testing-library/jest-dom": "^5.10.1", diff --git a/packages/core-plugin-api/src/extensions/componentData.tsx b/packages/core-plugin-api/src/extensions/componentData.tsx index f835ad38a8..059b8e1304 100644 --- a/packages/core-plugin-api/src/extensions/componentData.tsx +++ b/packages/core-plugin-api/src/extensions/componentData.tsx @@ -50,6 +50,8 @@ export function attachComponentData

( } container.map.set(type, data); + + return component; } export function getComponentData( diff --git a/packages/core-plugin-api/src/extensions/index.ts b/packages/core-plugin-api/src/extensions/index.ts index 8d2d6872d9..71373db1a3 100644 --- a/packages/core-plugin-api/src/extensions/index.ts +++ b/packages/core-plugin-api/src/extensions/index.ts @@ -20,4 +20,4 @@ export { createRoutableExtension, createComponentExtension, } from './extensions'; -export { useElementCollection } from './pennywise'; +export { useElementFilter } from './useElementFilter'; diff --git a/packages/core-plugin-api/src/extensions/useElementFilter.test.tsx b/packages/core-plugin-api/src/extensions/useElementFilter.test.tsx new file mode 100644 index 0000000000..4725de66c2 --- /dev/null +++ b/packages/core-plugin-api/src/extensions/useElementFilter.test.tsx @@ -0,0 +1,249 @@ +/* + * 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 React, { ReactNode } from 'react'; +import { useElementFilter } from './useElementFilter'; +import { renderHook } from '@testing-library/react-hooks'; +import { attachComponentData } from './componentData'; +import { featureFlagsApiRef } from '../apis'; +import { + ApiProvider, + ApiRegistry, + LocalStorageFeatureFlags, +} from '@backstage/core-app-api'; + +const WRAPPING_COMPONENT_KEY = 'core.blob.testing'; +const INNER_COMPONENT_KEY = 'core.blob2.testing'; + +const WrappingComponent = (props: { children: ReactNode }) => null; +attachComponentData(WrappingComponent, WRAPPING_COMPONENT_KEY, { + message: 'hey! im wrapping component data', +}); +const InnerComponent = () => null; +attachComponentData(InnerComponent, INNER_COMPONENT_KEY, { + message: 'hey! im the inner component', +}); +const MockComponent = (props: { children: ReactNode }) => null; + +const FeatureFlagComponent = (props: { children: ReactNode; flag: string }) => + null; +attachComponentData(FeatureFlagComponent, 'core.featureFlagged', true); +const mockFeatureFlagsApi = new LocalStorageFeatureFlags(); +const Wrapper = ({ children }: { children?: React.ReactNode }) => ( + + {children} + +); + +describe('useElementFilter', () => { + it('should select elements based on a component data key', () => { + const tree = ( + + + + + + + + + + + ); + + const { result } = renderHook( + props => + useElementFilter(props.tree, elements => + elements + .selectByComponentData({ key: WRAPPING_COMPONENT_KEY }) + .getElements(), + ), + { + initialProps: { tree }, + wrapper: Wrapper, + }, + ); + + expect(result.current.length).toBe(2); + expect(result.current[0].key).toBe('.$.$first'); + expect(result.current[1].key).toBe('.$.$second'); + }); + + it('should find componentData', () => { + const tree = ( + + + + + + + + + + + ); + + const { result } = renderHook( + props => + useElementFilter(props.tree, elements => + elements.findComponentData({ key: WRAPPING_COMPONENT_KEY }), + ), + { + initialProps: { tree }, + wrapper: Wrapper, + }, + ); + + expect(result.current.length).toBe(2); + expect(result.current[0]).toEqual({ + message: 'hey! im wrapping component data', + }); + expect(result.current[1]).toEqual({ + message: 'hey! im wrapping component data', + }); + }); + + it('can be combined to together to filter the selection', () => { + const tree = ( + + + + + + + + + + + + ); + + const { result } = renderHook( + props => + useElementFilter(props.tree, elements => + elements + .selectByComponentData({ key: WRAPPING_COMPONENT_KEY }) + .findComponentData({ key: INNER_COMPONENT_KEY }), + ), + { + initialProps: { tree }, + wrapper: Wrapper, + }, + ); + + expect(result.current.length).toBe(2); + expect(result.current[0]).toEqual({ + message: 'hey! im the inner component', + }); + expect(result.current[1]).toEqual({ + message: 'hey! im the inner component', + }); + }); + + it('should not discover deeper than the feature gate if the feature flag is disabled', () => { + jest.spyOn(mockFeatureFlagsApi, 'isActive').mockImplementation(() => false); + const tree = ( + + + + + + + + + + + + + + ); + + const { result } = renderHook( + props => + useElementFilter(props.tree, elements => + elements + .selectByComponentData({ key: WRAPPING_COMPONENT_KEY }) + .getElements(), + ), + { + initialProps: { tree }, + wrapper: Wrapper, + }, + ); + + expect(result.current.length).toBe(1); + expect(result.current[0].key).toContain('second'); + }); + + it('should discover components behind a feature flag if the flag is enabled', () => { + jest.spyOn(mockFeatureFlagsApi, 'isActive').mockImplementation(() => true); + const tree = ( + + + + + + + + + + + + + + ); + + const { result } = renderHook( + props => + useElementFilter(props.tree, elements => + elements + .selectByComponentData({ key: WRAPPING_COMPONENT_KEY }) + .getElements(), + ), + { + initialProps: { tree }, + wrapper: Wrapper, + }, + ); + + expect(result.current.length).toBe(2); + }); + + it('should reject with', () => { + const tree = ( + +

Hello

+ + ); + + const { result } = renderHook( + props => + useElementFilter(props.tree, elements => + elements + .selectByComponentData({ + key: WRAPPING_COMPONENT_KEY, + withStrictError: 'Could not find component', + // errorIfNotFullMatch? + }) + .findComponentData({ key: INNER_COMPONENT_KEY }), + ), + { + initialProps: { tree }, + wrapper: Wrapper, + }, + ); + + expect(result.error.message).toEqual('Could not find component'); + }); +}); diff --git a/packages/core-plugin-api/src/extensions/pennywise.ts b/packages/core-plugin-api/src/extensions/useElementFilter.tsx similarity index 72% rename from packages/core-plugin-api/src/extensions/pennywise.ts rename to packages/core-plugin-api/src/extensions/useElementFilter.tsx index d6d92a267e..9ed070fd6c 100644 --- a/packages/core-plugin-api/src/extensions/pennywise.ts +++ b/packages/core-plugin-api/src/extensions/useElementFilter.tsx @@ -19,6 +19,7 @@ import { isValidElement, ReactNode, ReactElement, + useMemo, } from 'react'; import { getComponentData } from './componentData'; import { useApi, FeatureFlagsApi, featureFlagsApiRef } from '../apis'; @@ -34,10 +35,13 @@ function selectChildren( return []; } - const { children } = node.props; - if (node.type === Fragment) { - return selectChildren(children, featureFlagsApi, selector, strictError); + return selectChildren( + node.props.children, + featureFlagsApi, + selector, + strictError, + ); } if (getComponentData(node, 'core.featureFlagged')) { @@ -72,13 +76,13 @@ function selectChildren( class ElementCollection { constructor( - private readonly children: ReactNode, + private readonly node: ReactNode, private readonly featureFlagsApi: FeatureFlagsApi, ) {} - findByComponentData(query: { key: string; withStrictError?: string }) { + selectByComponentData(query: { key: string; withStrictError?: string }) { const selection = selectChildren( - this.children, + this.node, this.featureFlagsApi, node => Boolean(getComponentData(node, query.key)), query.withStrictError, @@ -86,27 +90,31 @@ class ElementCollection { return new ElementCollection(selection, this.featureFlagsApi); } - listComponentData(query: { key: string }): T[] { - const selection = selectChildren( - this.children, - this.featureFlagsApi, - node => Boolean(getComponentData(node, query.key)), + findComponentData(query: { key: string }): T[] { + const selection = selectChildren(this.node, this.featureFlagsApi, node => + Boolean(getComponentData(node, query.key)), ); return selection .map(node => getComponentData(node, query.key)) .filter((data: T | undefined): data is T => Boolean(data)); } - listElements(): Array< + getElements(): Array< ReactElement > { - return selectChildren(this.children, this.featureFlagsApi) as Array< + return selectChildren(this.node, this.featureFlagsApi) as Array< ReactElement >; } } -export function useElementCollection(children: ReactNode) { +export function useElementFilter( + node: ReactNode, + filterFn: (arg: ElementCollection) => T, + dependencies: any[] = [], +) { const featureFlagsApi = useApi(featureFlagsApiRef); - return new ElementCollection(children, featureFlagsApi); + const elements = new ElementCollection(node, featureFlagsApi); + // eslint-disable-next-line react-hooks/exhaustive-deps + return useMemo(() => filterFn(elements), [node, ...dependencies]); } diff --git a/plugins/scaffolder/package.json b/plugins/scaffolder/package.json index a7541a5ddc..3cb046966a 100644 --- a/plugins/scaffolder/package.json +++ b/plugins/scaffolder/package.json @@ -33,11 +33,12 @@ "@backstage/catalog-client": "^0.3.13", "@backstage/catalog-model": "^0.8.2", "@backstage/config": "^0.1.5", - "@backstage/errors": "^0.1.1", "@backstage/core": "^0.7.12", "@backstage/integration": "^0.5.6", "@backstage/integration-react": "^0.1.3", "@backstage/plugin-catalog-react": "^0.2.2", + "@backstage/core-plugin-api": "^0.1.1", + "@backstage/errors": "^0.1.1", "@backstage/theme": "^0.2.8", "@material-ui/core": "^4.11.0", "@material-ui/icons": "^4.9.1", @@ -46,10 +47,10 @@ "@rjsf/material-ui": "^2.4.0", "@types/react": "^16.9", "classnames": "^2.2.6", - "json-schema": "^0.3.0", "git-url-parse": "^11.4.4", "humanize-duration": "^3.25.1", "immer": "^9.0.1", + "json-schema": "^0.3.0", "luxon": "^1.25.0", "react": "^16.13.1", "react-dom": "^16.13.1", @@ -66,9 +67,9 @@ "@backstage/test-utils": "^0.1.13", "@testing-library/jest-dom": "^5.10.1", "@testing-library/react": "^11.2.5", + "@testing-library/react-hooks": "^3.3.0", "@testing-library/user-event": "^13.1.8", "@types/humanize-duration": "^3.18.1", - "@testing-library/react-hooks": "^3.3.0", "@types/jest": "^26.0.7", "@types/node": "^14.14.32", "cross-fetch": "^3.0.6", diff --git a/plugins/scaffolder/src/components/Router.tsx b/plugins/scaffolder/src/components/Router.tsx index 57efd43e74..e72b9a9a1e 100644 --- a/plugins/scaffolder/src/components/Router.tsx +++ b/plugins/scaffolder/src/components/Router.tsx @@ -14,7 +14,7 @@ * limitations under the License. */ -import React, { useMemo } from 'react'; +import React from 'react'; import { Routes, Route, useOutlet } from 'react-router'; import { ScaffolderPage } from './ScaffolderPage'; import { TemplatePage } from './TemplatePage'; @@ -27,18 +27,23 @@ import { FIELD_EXTENSION_KEY, DEFAULT_SCAFFOLDER_FIELD_EXTENSIONS, } from '../extensions'; -import { useElementCollection } from '@backstage/core-plugin-api'; +import { useElementFilter } from '@backstage/core-plugin-api'; export const Router = () => { const outlet = useOutlet(); - const foundExtensions = useElementCollection(outlet) - .findByComponentData({ - key: FIELD_EXTENSION_WRAPPER_KEY, - }) - .listComponentData({ - key: FIELD_EXTENSION_KEY, - }); + const foundExtensions = useElementFilter(outlet, elements => + elements + .selectByComponentData({ + key: FIELD_EXTENSION_WRAPPER_KEY, + }) + .select({ + key: FIELD_EXTENSION_WRAPPER_KEY, + }) + .getComponentData({ + key: FIELD_EXTENSION_KEY, + }), + ); const fieldExtensions = foundExtensions.length ? foundExtensions From 359a118902e6a652f2a2343acfd9cc852d3cf972 Mon Sep 17 00:00:00 2001 From: blam Date: Fri, 11 Jun 2021 10:33:10 +0200 Subject: [PATCH 06/24] chore: reworking some thigns Signed-off-by: blam --- .../core-app-api/src/routing/FlatRoutes.tsx | 66 ++++++++++--------- .../src/extensions/useElementFilter.test.tsx | 3 +- plugins/catalog/package.json | 1 + .../components/EntityLayout/EntityLayout.tsx | 42 ++++++------ plugins/scaffolder/src/components/Router.tsx | 5 +- yarn.lock | 1 + 6 files changed, 60 insertions(+), 58 deletions(-) diff --git a/packages/core-app-api/src/routing/FlatRoutes.tsx b/packages/core-app-api/src/routing/FlatRoutes.tsx index c185373dc1..ad1f764e6c 100644 --- a/packages/core-app-api/src/routing/FlatRoutes.tsx +++ b/packages/core-app-api/src/routing/FlatRoutes.tsx @@ -16,7 +16,7 @@ import React, { ReactNode } from 'react'; import { useRoutes } from 'react-router-dom'; -import { useApp, useElementCollection } from '@backstage/core-plugin-api'; +import { useApp, useElementFilter } from '@backstage/core-plugin-api'; type RouteObject = { path: string; @@ -31,39 +31,41 @@ type FlatRoutesProps = { export const FlatRoutes = (props: FlatRoutesProps): JSX.Element | null => { const app = useApp(); const { NotFoundErrorPage } = app.getComponents(); - const routes = useElementCollection(props.children) - .listElements<{ path?: string; children: ReactNode }>() - .flatMap(child => { - let path = child.props.path; + const routes = useElementFilter(props.children, elements => + elements + .getElements<{ path?: string; children: ReactNode }>() + .flatMap(child => { + let path = child.props.path; - // TODO(Rugvip): Work around plugins registering empty paths, remove once deprecated routes are gone - if (path === '') { - return []; - } - path = path?.replace(/\/\*$/, '') ?? '/'; + // TODO(Rugvip): Work around plugins registering empty paths, remove once deprecated routes are gone + if (path === '') { + return []; + } + path = path?.replace(/\/\*$/, '') ?? '/'; - return [ - { - path, - element: child, - children: child.props.children - ? [ - { - path: '/*', - element: child.props.children, - }, - ] - : undefined, - }, - ]; - }) - // Routes are sorted to work around a bug where prefixes are unexpectedly matched - .sort((a, b) => b.path.localeCompare(a.path)) - // We make sure all routes have '/*' appended, except '/' - .map(obj => { - obj.path = obj.path === '/' ? '/' : `${obj.path}/*`; - return obj; - }); + return [ + { + path, + element: child, + children: child.props.children + ? [ + { + path: '/*', + element: child.props.children, + }, + ] + : undefined, + }, + ]; + }) + // Routes are sorted to work around a bug where prefixes are unexpectedly matched + .sort((a, b) => b.path.localeCompare(a.path)) + // We make sure all routes have '/*' appended, except '/' + .map(obj => { + obj.path = obj.path === '/' ? '/' : `${obj.path}/*`; + return obj; + }), + ); // TODO(Rugvip): Possibly add a way to skip this, like a noNotFoundPage prop routes.push({ diff --git a/packages/core-plugin-api/src/extensions/useElementFilter.test.tsx b/packages/core-plugin-api/src/extensions/useElementFilter.test.tsx index 4725de66c2..23a7fd77a7 100644 --- a/packages/core-plugin-api/src/extensions/useElementFilter.test.tsx +++ b/packages/core-plugin-api/src/extensions/useElementFilter.test.tsx @@ -220,7 +220,7 @@ describe('useElementFilter', () => { expect(result.current.length).toBe(2); }); - it('should reject with', () => { + it('should reject when strict mode is enabled with the correct string', () => { const tree = (

Hello

@@ -234,7 +234,6 @@ describe('useElementFilter', () => { .selectByComponentData({ key: WRAPPING_COMPONENT_KEY, withStrictError: 'Could not find component', - // errorIfNotFullMatch? }) .findComponentData({ key: INNER_COMPONENT_KEY }), ), diff --git a/plugins/catalog/package.json b/plugins/catalog/package.json index ce72d95103..56299462c6 100644 --- a/plugins/catalog/package.json +++ b/plugins/catalog/package.json @@ -33,6 +33,7 @@ "@backstage/catalog-client": "^0.3.13", "@backstage/catalog-model": "^0.8.2", "@backstage/core": "^0.7.12", + "@backstage/core-plugin-api": "^0.1.1", "@backstage/errors": "^0.1.1", "@backstage/integration": "^0.5.6", "@backstage/integration-react": "^0.1.3", diff --git a/plugins/catalog/src/components/EntityLayout/EntityLayout.tsx b/plugins/catalog/src/components/EntityLayout/EntityLayout.tsx index 7cd4ab3dd1..aceb1da6f2 100644 --- a/plugins/catalog/src/components/EntityLayout/EntityLayout.tsx +++ b/plugins/catalog/src/components/EntityLayout/EntityLayout.tsx @@ -29,7 +29,7 @@ import { Progress, RoutedTabs, } from '@backstage/core'; -import { useElementCollection } from '@backstage/core-plugin-api'; +import { useElementFilter } from '@backstage/core-plugin-api'; import { EntityContext, EntityRefLinks, @@ -157,26 +157,28 @@ export const EntityLayout = ({ const { kind, namespace, name } = useEntityCompoundName(); const { entity, loading, error } = useContext(EntityContext); - const routes = useElementCollection(children) - .findByComponentData({ - key: dataKey, - withStrictError: 'Child of EntityLayout must be an EntityLayout.Route', - }) - .listElements() // all nodes, element data, maintain structure or not? - .flatMap(({ props }) => { - if (props.if && entity && !props.if(entity)) { - return []; - } + const routes = useElementFilter(children, elements => + elements + .selectByComponentData({ + key: dataKey, + withStrictError: 'Child of EntityLayout must be an EntityLayout.Route', + }) + .getElements() // all nodes, element data, maintain structure or not? + .flatMap(({ props }) => { + if (props.if && entity && !props.if(entity)) { + return []; + } - return [ - { - path: props.path, - title: props.title, - children: props.children, - tabProps: props.tabProps, - }, - ]; - }); + return [ + { + path: props.path, + title: props.title, + children: props.children, + tabProps: props.tabProps, + }, + ]; + }), + ); const { headerTitle, headerType } = headerProps( kind, diff --git a/plugins/scaffolder/src/components/Router.tsx b/plugins/scaffolder/src/components/Router.tsx index e72b9a9a1e..ef0dd4082e 100644 --- a/plugins/scaffolder/src/components/Router.tsx +++ b/plugins/scaffolder/src/components/Router.tsx @@ -37,10 +37,7 @@ export const Router = () => { .selectByComponentData({ key: FIELD_EXTENSION_WRAPPER_KEY, }) - .select({ - key: FIELD_EXTENSION_WRAPPER_KEY, - }) - .getComponentData({ + .findComponentData({ key: FIELD_EXTENSION_KEY, }), ); diff --git a/yarn.lock b/yarn.lock index 4bf6d53619..8c7cf9ee8f 100644 --- a/yarn.lock +++ b/yarn.lock @@ -1394,6 +1394,7 @@ "@backstage/catalog-client" "^0.3.13" "@backstage/catalog-model" "^0.8.2" "@backstage/core" "^0.7.12" + "@backstage/core-plugin-api" "^0.1.1" "@backstage/errors" "^0.1.1" "@backstage/integration" "^0.5.6" "@backstage/integration-react" "^0.1.3" From 31e067d346c20cd2d21197a3da73e2d603f09a6a Mon Sep 17 00:00:00 2001 From: blam Date: Fri, 11 Jun 2021 10:52:51 +0200 Subject: [PATCH 07/24] chore: actually use the right packages - went and messed up a bit Signed-off-by: blam --- packages/app/package.json | 2 +- packages/core-plugin-api/package.json | 2 +- .../src/extensions/useElementFilter.test.tsx | 48 +++++++++++++++++-- plugins/catalog/package.json | 2 +- plugins/scaffolder/package.json | 2 +- yarn.lock | 2 +- 6 files changed, 50 insertions(+), 8 deletions(-) diff --git a/packages/app/package.json b/packages/app/package.json index bfd2b458ae..76d32b57ad 100644 --- a/packages/app/package.json +++ b/packages/app/package.json @@ -8,7 +8,7 @@ "@backstage/cli": "^0.7.0", "@backstage/core": "^0.7.12", "@backstage/integration-react": "^0.1.3", - "@backstage/core-app-api": "^0.1.1", + "@backstage/core-app-api": "^0.1.2", "@backstage/core-components": "^0.1.1", "@backstage/plugin-api-docs": "^0.4.15", "@backstage/plugin-badges": "^0.2.2", diff --git a/packages/core-plugin-api/package.json b/packages/core-plugin-api/package.json index b1d53e17c0..48706a5877 100644 --- a/packages/core-plugin-api/package.json +++ b/packages/core-plugin-api/package.json @@ -42,7 +42,7 @@ }, "devDependencies": { "@backstage/cli": "^0.7.0", - "@backstage/core-app-api": "^0.1.1", + "@backstage/core-app-api": "^0.1.2", "@backstage/test-utils": "^0.1.13", "@backstage/test-utils-core": "^0.1.1", "@testing-library/jest-dom": "^5.10.1", diff --git a/packages/core-plugin-api/src/extensions/useElementFilter.test.tsx b/packages/core-plugin-api/src/extensions/useElementFilter.test.tsx index 23a7fd77a7..d5c1b0befe 100644 --- a/packages/core-plugin-api/src/extensions/useElementFilter.test.tsx +++ b/packages/core-plugin-api/src/extensions/useElementFilter.test.tsx @@ -27,7 +27,7 @@ import { const WRAPPING_COMPONENT_KEY = 'core.blob.testing'; const INNER_COMPONENT_KEY = 'core.blob2.testing'; -const WrappingComponent = (props: { children: ReactNode }) => null; +const WrappingComponent = (_props: { children: ReactNode }) => null; attachComponentData(WrappingComponent, WRAPPING_COMPONENT_KEY, { message: 'hey! im wrapping component data', }); @@ -35,9 +35,9 @@ const InnerComponent = () => null; attachComponentData(InnerComponent, INNER_COMPONENT_KEY, { message: 'hey! im the inner component', }); -const MockComponent = (props: { children: ReactNode }) => null; +const MockComponent = (_props: { children: ReactNode }) => null; -const FeatureFlagComponent = (props: { children: ReactNode; flag: string }) => +const FeatureFlagComponent = (_props: { children: ReactNode; flag: string }) => null; attachComponentData(FeatureFlagComponent, 'core.featureFlagged', true); const mockFeatureFlagsApi = new LocalStorageFeatureFlags(); @@ -245,4 +245,46 @@ describe('useElementFilter', () => { expect(result.error.message).toEqual('Could not find component'); }); + + it('should support fragments and text node iteration', () => { + jest.spyOn(mockFeatureFlagsApi, 'isActive').mockImplementation(() => true); + const tree = ( + <> + + <> + + + + + + + + hello my name + <> + + + + + + is text + + + + ); + + const { result } = renderHook( + props => + useElementFilter(props.tree, elements => + elements + .selectByComponentData({ key: WRAPPING_COMPONENT_KEY }) + .getElements(), + ), + { + initialProps: { tree }, + wrapper: Wrapper, + }, + ); + + expect(result.current.length).toBe(2); + }); }); diff --git a/plugins/catalog/package.json b/plugins/catalog/package.json index 56299462c6..e1e3b388a1 100644 --- a/plugins/catalog/package.json +++ b/plugins/catalog/package.json @@ -33,7 +33,7 @@ "@backstage/catalog-client": "^0.3.13", "@backstage/catalog-model": "^0.8.2", "@backstage/core": "^0.7.12", - "@backstage/core-plugin-api": "^0.1.1", + "@backstage/core-plugin-api": "^0.1.2", "@backstage/errors": "^0.1.1", "@backstage/integration": "^0.5.6", "@backstage/integration-react": "^0.1.3", diff --git a/plugins/scaffolder/package.json b/plugins/scaffolder/package.json index 3cb046966a..9790073211 100644 --- a/plugins/scaffolder/package.json +++ b/plugins/scaffolder/package.json @@ -37,7 +37,7 @@ "@backstage/integration": "^0.5.6", "@backstage/integration-react": "^0.1.3", "@backstage/plugin-catalog-react": "^0.2.2", - "@backstage/core-plugin-api": "^0.1.1", + "@backstage/core-plugin-api": "^0.1.2", "@backstage/errors": "^0.1.1", "@backstage/theme": "^0.2.8", "@material-ui/core": "^4.11.0", diff --git a/yarn.lock b/yarn.lock index 8c7cf9ee8f..708b96efcc 100644 --- a/yarn.lock +++ b/yarn.lock @@ -1394,7 +1394,7 @@ "@backstage/catalog-client" "^0.3.13" "@backstage/catalog-model" "^0.8.2" "@backstage/core" "^0.7.12" - "@backstage/core-plugin-api" "^0.1.1" + "@backstage/core-plugin-api" "^0.1.2" "@backstage/errors" "^0.1.1" "@backstage/integration" "^0.5.6" "@backstage/integration-react" "^0.1.3" From 124be46a5a35fd46377b649e6b85f18e12affa23 Mon Sep 17 00:00:00 2001 From: blam Date: Fri, 11 Jun 2021 11:31:26 +0200 Subject: [PATCH 08/24] chore: resetting yarn lock Signed-off-by: blam --- yarn.lock | 1 - 1 file changed, 1 deletion(-) diff --git a/yarn.lock b/yarn.lock index 708b96efcc..4bf6d53619 100644 --- a/yarn.lock +++ b/yarn.lock @@ -1394,7 +1394,6 @@ "@backstage/catalog-client" "^0.3.13" "@backstage/catalog-model" "^0.8.2" "@backstage/core" "^0.7.12" - "@backstage/core-plugin-api" "^0.1.2" "@backstage/errors" "^0.1.1" "@backstage/integration" "^0.5.6" "@backstage/integration-react" "^0.1.3" From 8ddd77544fa9ba262feca47c9a453f36284c6560 Mon Sep 17 00:00:00 2001 From: blam Date: Fri, 11 Jun 2021 11:38:30 +0200 Subject: [PATCH 09/24] chore: reworking yarn.lock Signed-off-by: blam --- yarn.lock | 1 + 1 file changed, 1 insertion(+) diff --git a/yarn.lock b/yarn.lock index 4bf6d53619..708b96efcc 100644 --- a/yarn.lock +++ b/yarn.lock @@ -1394,6 +1394,7 @@ "@backstage/catalog-client" "^0.3.13" "@backstage/catalog-model" "^0.8.2" "@backstage/core" "^0.7.12" + "@backstage/core-plugin-api" "^0.1.2" "@backstage/errors" "^0.1.1" "@backstage/integration" "^0.5.6" "@backstage/integration-react" "^0.1.3" From 9eb3729706d5a7a874fb03bb0609b98801dd591b Mon Sep 17 00:00:00 2001 From: blam Date: Fri, 11 Jun 2021 13:16:20 +0200 Subject: [PATCH 10/24] chore: fixing api-docs generation Co-authored-by: Johan Haals Signed-off-by: blam --- packages/core-app-api/api-report.md | 9 +++++++++ packages/core-plugin-api/api-report.md | 4 ++++ .../core-plugin-api/src/extensions/componentData.tsx | 2 -- 3 files changed, 13 insertions(+), 2 deletions(-) diff --git a/packages/core-app-api/api-report.md b/packages/core-app-api/api-report.md index b85c9392d7..2110bede38 100644 --- a/packages/core-app-api/api-report.md +++ b/packages/core-app-api/api-report.md @@ -234,6 +234,15 @@ export type ErrorBoundaryFallbackProps = { resetError: () => void; }; +// @public (undocumented) +export const FeatureFlagged: ({ children, flag }: FeatureFlaggedProps) => JSX.Element | null; + +// @public (undocumented) +export type FeatureFlaggedProps = { + flag: string; + children: JSX.Element | null; +}; + // @public (undocumented) export const FlatRoutes: (props: FlatRoutesProps) => JSX.Element | null; diff --git a/packages/core-plugin-api/api-report.md b/packages/core-plugin-api/api-report.md index a6f196847a..6e8cda3d83 100644 --- a/packages/core-plugin-api/api-report.md +++ b/packages/core-plugin-api/api-report.md @@ -8,6 +8,7 @@ import { BackstageTheme } from '@backstage/theme'; import { ComponentType } from 'react'; import { Config } from '@backstage/config'; import { default as React_2 } from 'react'; +import { ReactElement } from 'react'; import { ReactNode } from 'react'; import { SvgIconProps } from '@material-ui/core'; @@ -514,6 +515,9 @@ export function useApiHolder(): ApiHolder; // @public (undocumented) export const useApp: () => AppContext; +// @public (undocumented) +export function useElementFilter(node: ReactNode, filterFn: (arg: ElementCollection) => T, dependencies?: any[]): T; + // @public (undocumented) export type UserFlags = {}; diff --git a/packages/core-plugin-api/src/extensions/componentData.tsx b/packages/core-plugin-api/src/extensions/componentData.tsx index 059b8e1304..f835ad38a8 100644 --- a/packages/core-plugin-api/src/extensions/componentData.tsx +++ b/packages/core-plugin-api/src/extensions/componentData.tsx @@ -50,8 +50,6 @@ export function attachComponentData

( } container.map.set(type, data); - - return component; } export function getComponentData( From 5f4339b8c597fea8bd76d91b1620347c6a586ef7 Mon Sep 17 00:00:00 2001 From: blam Date: Fri, 11 Jun 2021 13:19:33 +0200 Subject: [PATCH 11/24] chore: added changeset Co-authored-by: Johan Haals Signed-off-by: blam --- .changeset/few-windows-whisper.md | 8 ++++++++ 1 file changed, 8 insertions(+) create mode 100644 .changeset/few-windows-whisper.md diff --git a/.changeset/few-windows-whisper.md b/.changeset/few-windows-whisper.md new file mode 100644 index 0000000000..565e8798a1 --- /dev/null +++ b/.changeset/few-windows-whisper.md @@ -0,0 +1,8 @@ +--- +'@backstage/core-app-api': patch +'@backstage/core-plugin-api': patch +'@backstage/plugin-catalog': patch +'@backstage/plugin-scaffolder': patch +--- + +Adding `FeatureFlag` component and treating `FeatureFlags` as first class citizens to composability API From c92f5dedee4e0e80002f1176444c62cfe880e546 Mon Sep 17 00:00:00 2001 From: blam Date: Sat, 12 Jun 2021 17:35:08 +0200 Subject: [PATCH 12/24] feat: reworking the api in favour of `with` and `without` for wrapping component Signed-off-by: blam --- .../src/routing/FeatureFlagged.test.tsx | 102 ++++++++++++++++++ .../src/routing/FeatureFlagged.tsx | 20 +++- 2 files changed, 118 insertions(+), 4 deletions(-) create mode 100644 packages/core-app-api/src/routing/FeatureFlagged.test.tsx diff --git a/packages/core-app-api/src/routing/FeatureFlagged.test.tsx b/packages/core-app-api/src/routing/FeatureFlagged.test.tsx new file mode 100644 index 0000000000..e20af76ae1 --- /dev/null +++ b/packages/core-app-api/src/routing/FeatureFlagged.test.tsx @@ -0,0 +1,102 @@ +/* + * 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 React from 'react'; +import { FeatureFlagged } from './FeatureFlagged'; +import { render } from '@testing-library/react'; +import { ApiProvider, ApiRegistry, LocalStorageFeatureFlags } from '../apis'; +import { featureFlagsApiRef } from '@backstage/core-plugin-api'; + +const mockFeatureFlagsApi = new LocalStorageFeatureFlags(); +const Wrapper = ({ children }: { children?: React.ReactNode }) => ( + + {children} + +); + +describe('FeatureFlagged', () => { + describe('with', () => { + it('should render contents when the feature flag is enabled', async () => { + jest + .spyOn(mockFeatureFlagsApi, 'isActive') + .mockImplementation(() => true); + + const { queryByText } = render( + +

+ +

BACKSTAGE!

+ +
+ , + ); + + expect(await queryByText('BACKSTAGE!')).toBeInTheDocument(); + }); + it('should not render contents when the feature flag is disabled', async () => { + jest + .spyOn(mockFeatureFlagsApi, 'isActive') + .mockImplementation(() => false); + + const { queryByText } = render( + +
+ +

BACKSTAGE!

+ +
+ , + ); + + expect(await queryByText('BACKSTAGE!')).not.toBeInTheDocument(); + }); + }); + describe('without', () => { + it('should not render contents when the feature flag is enabled', async () => { + jest + .spyOn(mockFeatureFlagsApi, 'isActive') + .mockImplementation(() => true); + + const { queryByText } = render( + +
+ +

BACKSTAGE!

+ +
+ , + ); + + expect(await queryByText('BACKSTAGE!')).not.toBeInTheDocument(); + }); + it('should render contents when the feature flag is disabled', async () => { + jest + .spyOn(mockFeatureFlagsApi, 'isActive') + .mockImplementation(() => false); + + const { queryByText } = render( + +
+ +

BACKSTAGE!

+ +
+ , + ); + + expect(await queryByText('BACKSTAGE!')).toBeInTheDocument(); + }); + }); +}); diff --git a/packages/core-app-api/src/routing/FeatureFlagged.tsx b/packages/core-app-api/src/routing/FeatureFlagged.tsx index 0adda78842..5b20d9aaac 100644 --- a/packages/core-app-api/src/routing/FeatureFlagged.tsx +++ b/packages/core-app-api/src/routing/FeatureFlagged.tsx @@ -20,14 +20,26 @@ import { attachComponentData, } from '@backstage/core-plugin-api'; -export type FeatureFlaggedProps = { - flag: string; +export type FeatureFlaggedWith = { + with: string; +}; + +export type FeatureFlaggedWithout = { + without: string; +}; + +export type FeatureFlaggedSelector = FeatureFlaggedWith | FeatureFlaggedWithout; +export type FeatureFlaggedProps = FeatureFlaggedSelector & { children: JSX.Element | null; }; -export const FeatureFlagged = ({ children, flag }: FeatureFlaggedProps) => { +export const FeatureFlagged = (props: FeatureFlaggedProps) => { + const { children } = props; const featureFlagApi = useApi(featureFlagsApiRef); - const isEnabled = featureFlagApi.isActive(flag); + const isEnabled = + 'with' in props + ? featureFlagApi.isActive(props.with) + : !featureFlagApi.isActive(props.without); return isEnabled ? children : null; }; From 128279fc4f91f9d04cb1287cdf1b5ce7a54acd21 Mon Sep 17 00:00:00 2001 From: blam Date: Sat, 12 Jun 2021 17:41:21 +0200 Subject: [PATCH 13/24] feat: finishing up some other use cases and re-writing some logic with some more tests Signed-off-by: blam --- packages/app/src/App.tsx | 2 +- .../app/src/components/catalog/EntityPage.tsx | 4 +- .../core-app-api/src/routing/collectors.tsx | 4 +- .../src/extensions/useElementFilter.test.tsx | 207 ++++++++++++------ .../src/extensions/useElementFilter.tsx | 8 +- packages/core-plugin-api/src/index.test.ts | 1 + 6 files changed, 158 insertions(+), 68 deletions(-) diff --git a/packages/app/src/App.tsx b/packages/app/src/App.tsx index 12cbe89f26..de11afeed4 100644 --- a/packages/app/src/App.tsx +++ b/packages/app/src/App.tsx @@ -114,7 +114,7 @@ const routes = ( path="/tech-radar" element={} /> - + } /> } /> diff --git a/packages/app/src/components/catalog/EntityPage.tsx b/packages/app/src/components/catalog/EntityPage.tsx index 61d62136de..41d2d6f8c2 100644 --- a/packages/app/src/components/catalog/EntityPage.tsx +++ b/packages/app/src/components/catalog/EntityPage.tsx @@ -283,7 +283,7 @@ const serviceEntityPage = ( {overviewContent} - + {cicdContent} @@ -306,7 +306,7 @@ const serviceEntityPage = ( - + diff --git a/packages/core-app-api/src/routing/collectors.tsx b/packages/core-app-api/src/routing/collectors.tsx index 31c1ab6361..b9ca45f54a 100644 --- a/packages/core-app-api/src/routing/collectors.tsx +++ b/packages/core-app-api/src/routing/collectors.tsx @@ -177,8 +177,8 @@ export const featureFlagCollector = createCollector( () => new Set(), (acc, node) => { if (node.type === FeatureFlagged) { - const { flag } = node.props as FeatureFlaggedProps; - acc.add(flag); + const props = node.props as FeatureFlaggedProps; + acc.add('with' in props ? props.with : props.without); } }, ); diff --git a/packages/core-plugin-api/src/extensions/useElementFilter.test.tsx b/packages/core-plugin-api/src/extensions/useElementFilter.test.tsx index d5c1b0befe..bb2bcb06a5 100644 --- a/packages/core-plugin-api/src/extensions/useElementFilter.test.tsx +++ b/packages/core-plugin-api/src/extensions/useElementFilter.test.tsx @@ -37,8 +37,11 @@ attachComponentData(InnerComponent, INNER_COMPONENT_KEY, { }); const MockComponent = (_props: { children: ReactNode }) => null; -const FeatureFlagComponent = (_props: { children: ReactNode; flag: string }) => - null; +const FeatureFlagComponent = (_props: { + children: ReactNode; + with?: string; + without?: string; +}) => null; attachComponentData(FeatureFlagComponent, 'core.featureFlagged', true); const mockFeatureFlagsApi = new LocalStorageFeatureFlags(); const Wrapper = ({ children }: { children?: React.ReactNode }) => ( @@ -151,73 +154,155 @@ describe('useElementFilter', () => { }); }); - it('should not discover deeper than the feature gate if the feature flag is disabled', () => { - jest.spyOn(mockFeatureFlagsApi, 'isActive').mockImplementation(() => false); - const tree = ( - - - + describe('FeatureFlags', () => { + describe('with', () => { + it('should not discover deeper than the feature gate if the feature flag is disabled', () => { + jest + .spyOn(mockFeatureFlagsApi, 'isActive') + .mockImplementation(() => false); + const tree = ( + + + + + + + + + + + - - - - + + ); + + const { result } = renderHook( + props => + useElementFilter(props.tree, elements => + elements + .selectByComponentData({ key: WRAPPING_COMPONENT_KEY }) + .getElements(), + ), + { + initialProps: { tree }, + wrapper: Wrapper, + }, + ); + + expect(result.current.length).toBe(1); + expect(result.current[0].key).toContain('second'); + }); + + it('should discover components behind a feature flag if the flag is enabled', () => { + jest + .spyOn(mockFeatureFlagsApi, 'isActive') + .mockImplementation(() => true); + const tree = ( + + + + + + + + + + + - - - - - ); + + ); - const { result } = renderHook( - props => - useElementFilter(props.tree, elements => - elements - .selectByComponentData({ key: WRAPPING_COMPONENT_KEY }) - .getElements(), - ), - { - initialProps: { tree }, - wrapper: Wrapper, - }, - ); + const { result } = renderHook( + props => + useElementFilter(props.tree, elements => + elements + .selectByComponentData({ key: WRAPPING_COMPONENT_KEY }) + .getElements(), + ), + { + initialProps: { tree }, + wrapper: Wrapper, + }, + ); - expect(result.current.length).toBe(1); - expect(result.current[0].key).toContain('second'); - }); + expect(result.current.length).toBe(2); + }); + }); - it('should discover components behind a feature flag if the flag is enabled', () => { - jest.spyOn(mockFeatureFlagsApi, 'isActive').mockImplementation(() => true); - const tree = ( - - - + describe('without', () => { + it('should discover deeper than the feature gate if the feature flag is disabled', () => { + jest + .spyOn(mockFeatureFlagsApi, 'isActive') + .mockImplementation(() => false); + const tree = ( + + + + + + + + + + + - - - - + + ); + + const { result } = renderHook( + props => + useElementFilter(props.tree, elements => + elements + .selectByComponentData({ key: WRAPPING_COMPONENT_KEY }) + .getElements(), + ), + { + initialProps: { tree }, + wrapper: Wrapper, + }, + ); + + expect(result.current.length).toBe(2); + }); + + it('should not discover components behind a feature flag if the flag is enabled', () => { + jest + .spyOn(mockFeatureFlagsApi, 'isActive') + .mockImplementation(() => true); + const tree = ( + + + + + + + + + + + - - - - - ); + + ); - const { result } = renderHook( - props => - useElementFilter(props.tree, elements => - elements - .selectByComponentData({ key: WRAPPING_COMPONENT_KEY }) - .getElements(), - ), - { - initialProps: { tree }, - wrapper: Wrapper, - }, - ); + const { result } = renderHook( + props => + useElementFilter(props.tree, elements => + elements + .selectByComponentData({ key: WRAPPING_COMPONENT_KEY }) + .getElements(), + ), + { + initialProps: { tree }, + wrapper: Wrapper, + }, + ); - expect(result.current.length).toBe(2); + expect(result.current.length).toBe(1); + }); + }); }); it('should reject when strict mode is enabled with the correct string', () => { @@ -252,7 +337,7 @@ describe('useElementFilter', () => { <> <> - + diff --git a/packages/core-plugin-api/src/extensions/useElementFilter.tsx b/packages/core-plugin-api/src/extensions/useElementFilter.tsx index 9ed070fd6c..b33c23e422 100644 --- a/packages/core-plugin-api/src/extensions/useElementFilter.tsx +++ b/packages/core-plugin-api/src/extensions/useElementFilter.tsx @@ -45,8 +45,12 @@ function selectChildren( } if (getComponentData(node, 'core.featureFlagged')) { - const { flag } = node.props as { flag: string }; - if (featureFlagsApi.isActive(flag)) { + const props = node.props as { with: string } | { without: string }; + const isEnabled = + 'with' in props + ? featureFlagsApi.isActive(props.with) + : !featureFlagsApi.isActive(props.without); + if (isEnabled) { return selectChildren( node.props.children, featureFlagsApi, diff --git a/packages/core-plugin-api/src/index.test.ts b/packages/core-plugin-api/src/index.test.ts index 9414a6b730..5d29ead619 100644 --- a/packages/core-plugin-api/src/index.test.ts +++ b/packages/core-plugin-api/src/index.test.ts @@ -40,6 +40,7 @@ describe('index', () => { useApi: expect.any(Function), useApiHolder: expect.any(Function), useApp: expect.any(Function), + useElementFilter: expect.any(Function), useRouteRef: expect.any(Function), useRouteRefParams: expect.any(Function), withApis: expect.any(Function), From 6109f32b788438f08ed9dc240e50a0c208ccd775 Mon Sep 17 00:00:00 2001 From: blam Date: Sat, 12 Jun 2021 17:43:18 +0200 Subject: [PATCH 14/24] chore: fixing up api-docs exporting Signed-off-by: blam --- packages/core-app-api/api-report.md | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/packages/core-app-api/api-report.md b/packages/core-app-api/api-report.md index 2110bede38..029b0faff8 100644 --- a/packages/core-app-api/api-report.md +++ b/packages/core-app-api/api-report.md @@ -235,11 +235,10 @@ export type ErrorBoundaryFallbackProps = { }; // @public (undocumented) -export const FeatureFlagged: ({ children, flag }: FeatureFlaggedProps) => JSX.Element | null; +export const FeatureFlagged: (props: FeatureFlaggedProps) => JSX.Element | null; // @public (undocumented) -export type FeatureFlaggedProps = { - flag: string; +export type FeatureFlaggedProps = FeatureFlaggedSelector & { children: JSX.Element | null; }; From 00815e520b78e2658ec061407fed1773b0cca3d0 Mon Sep 17 00:00:00 2001 From: blam Date: Sun, 13 Jun 2021 04:36:06 +0200 Subject: [PATCH 15/24] chore: fixing api-docs and simplifying types a little Signed-off-by: blam --- packages/core-app-api/api-report.md | 8 ++++++-- packages/core-app-api/src/index.test.ts | 1 + .../core-app-api/src/routing/FeatureFlagged.tsx | 17 ++++------------- 3 files changed, 11 insertions(+), 15 deletions(-) diff --git a/packages/core-app-api/api-report.md b/packages/core-app-api/api-report.md index 029b0faff8..9b68d2a10d 100644 --- a/packages/core-app-api/api-report.md +++ b/packages/core-app-api/api-report.md @@ -238,9 +238,13 @@ export type ErrorBoundaryFallbackProps = { export const FeatureFlagged: (props: FeatureFlaggedProps) => JSX.Element | null; // @public (undocumented) -export type FeatureFlaggedProps = FeatureFlaggedSelector & { +export type FeatureFlaggedProps = { children: JSX.Element | null; -}; +} & ({ + with: string; +} | { + without: string; +}); // @public (undocumented) export const FlatRoutes: (props: FlatRoutesProps) => JSX.Element | null; diff --git a/packages/core-app-api/src/index.test.ts b/packages/core-app-api/src/index.test.ts index bec176fda8..654c547fca 100644 --- a/packages/core-app-api/src/index.test.ts +++ b/packages/core-app-api/src/index.test.ts @@ -37,6 +37,7 @@ describe('index', () => { ConfigReader: expect.any(Function), ErrorAlerter: expect.any(Function), ErrorApiForwarder: expect.any(Function), + FeatureFlagged: expect.any(Function), GithubAuth: expect.any(Function), GitlabAuth: expect.any(Function), GoogleAuth: expect.any(Function), diff --git a/packages/core-app-api/src/routing/FeatureFlagged.tsx b/packages/core-app-api/src/routing/FeatureFlagged.tsx index 5b20d9aaac..809eb7c69c 100644 --- a/packages/core-app-api/src/routing/FeatureFlagged.tsx +++ b/packages/core-app-api/src/routing/FeatureFlagged.tsx @@ -20,19 +20,10 @@ import { attachComponentData, } from '@backstage/core-plugin-api'; -export type FeatureFlaggedWith = { - with: string; -}; - -export type FeatureFlaggedWithout = { - without: string; -}; - -export type FeatureFlaggedSelector = FeatureFlaggedWith | FeatureFlaggedWithout; -export type FeatureFlaggedProps = FeatureFlaggedSelector & { - children: JSX.Element | null; -}; - +export type FeatureFlaggedProps = { children: JSX.Element | null } & ( + | { with: string } + | { without: string } +); export const FeatureFlagged = (props: FeatureFlaggedProps) => { const { children } = props; const featureFlagApi = useApi(featureFlagsApiRef); From fdb79d1562f5475d1f9458c95c750c15bb64e4c0 Mon Sep 17 00:00:00 2001 From: blam Date: Mon, 14 Jun 2021 10:11:13 +0200 Subject: [PATCH 16/24] chore: fixing tests for FlatRoutes Signed-off-by: blam --- .../src/routing/FlatRoutes.test.tsx | 33 ++++++++++++------- 1 file changed, 22 insertions(+), 11 deletions(-) diff --git a/packages/core-app-api/src/routing/FlatRoutes.test.tsx b/packages/core-app-api/src/routing/FlatRoutes.test.tsx index a9b83d1016..3ed5d5eb3a 100644 --- a/packages/core-app-api/src/routing/FlatRoutes.test.tsx +++ b/packages/core-app-api/src/routing/FlatRoutes.test.tsx @@ -17,25 +17,36 @@ import { render, RenderResult } from '@testing-library/react'; import React, { ReactNode } from 'react'; import { MemoryRouter, Route, Routes, useOutlet } from 'react-router-dom'; +import { ApiProvider, ApiRegistry, LocalStorageFeatureFlags } from '../apis'; +import { featureFlagsApiRef } from '@backstage/core-plugin-api'; import { AppContext } from '../app'; import { AppContextProvider } from '../app/AppContext'; import { FlatRoutes } from './FlatRoutes'; +const mockFeatureFlagsApi = new LocalStorageFeatureFlags(); +const Wrapper = ({ children }: { children?: React.ReactNode }) => ( + + {children} + +); + function makeRouteRenderer(node: ReactNode) { let rendered: RenderResult | undefined = undefined; return (path: string) => { const content = ( - ({ - NotFoundErrorPage: () => <>Not Found, - }), - } as unknown) as AppContext - } - > - - + + ({ + NotFoundErrorPage: () => <>Not Found, + }), + } as unknown) as AppContext + } + > + + + ); if (rendered) { rendered.unmount(); From 04fa2df35247dce218e05a59b158b0fc30447b98 Mon Sep 17 00:00:00 2001 From: blam Date: Mon, 14 Jun 2021 10:18:37 +0200 Subject: [PATCH 17/24] chore: wait for the config api to be loaded first before registering feature featureFlags Signed-off-by: blam --- packages/core-app-api/src/app/App.tsx | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/packages/core-app-api/src/app/App.tsx b/packages/core-app-api/src/app/App.tsx index 1d77282635..ecf06955b0 100644 --- a/packages/core-app-api/src/app/App.tsx +++ b/packages/core-app-api/src/app/App.tsx @@ -216,7 +216,12 @@ export class PrivateAppImpl implements BackstageApp { [], ); - const { routePaths, routeParents, routeObjects } = useMemo(() => { + const { + routePaths, + routeParents, + routeObjects, + featureFlags, + } = useMemo(() => { const result = traverseElementTree({ root: children, discoverers: [childDiscoverer, routeElementDiscoverer], @@ -239,14 +244,7 @@ export class PrivateAppImpl implements BackstageApp { this.verifyPlugins(this.plugins); // Initialize APIs once all plugins are available - const apiHolder = this.getApiHolder(); - - // Register feature flags that have been discovered - const featureFlagApi = apiHolder.get(featureFlagsApiRef)!; - for (const name of result.featureFlags) { - featureFlagApi.registerFlag({ name, pluginId: '' }); - } - + this.getApiHolder(); return result; }, [children]); @@ -281,8 +279,14 @@ export class PrivateAppImpl implements BackstageApp { } } } + + // Go through the featureFlags returned from the traversal and + // register those now the configApi has been loaded + for (const name of featureFlags) { + featureFlagsApi.registerFlag({ name, pluginId: '' }); + } } - }, [hasConfigApi, loadedConfig]); + }, [hasConfigApi, loadedConfig, featureFlags]); if ('node' in loadedConfig) { // Loading or error From f547a1473e70ca5c2b9a3803af70363e04d08631 Mon Sep 17 00:00:00 2001 From: blam Date: Tue, 15 Jun 2021 11:59:29 +0200 Subject: [PATCH 18/24] feat: update the usage of the EntitySwitch to use the new traversal composability API MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Fredrik Adelöw Co-authored-by: Johan Haals Signed-off-by: blam --- plugins/catalog/package.json | 1 + .../EntitySwitch/EntitySwitch.test.tsx | 101 +++++++++++------- .../components/EntitySwitch/EntitySwitch.tsx | 49 ++++----- 3 files changed, 83 insertions(+), 68 deletions(-) diff --git a/plugins/catalog/package.json b/plugins/catalog/package.json index e1e3b388a1..16e756eabe 100644 --- a/plugins/catalog/package.json +++ b/plugins/catalog/package.json @@ -55,6 +55,7 @@ }, "devDependencies": { "@backstage/cli": "^0.7.0", + "@backstage/core-app-api": "^0.1.2", "@backstage/dev-utils": "^0.1.17", "@backstage/test-utils": "^0.1.13", "@testing-library/jest-dom": "^5.10.1", diff --git a/plugins/catalog/src/components/EntitySwitch/EntitySwitch.test.tsx b/plugins/catalog/src/components/EntitySwitch/EntitySwitch.test.tsx index 20b9aaba4f..5b21266d44 100644 --- a/plugins/catalog/src/components/EntitySwitch/EntitySwitch.test.tsx +++ b/plugins/catalog/src/components/EntitySwitch/EntitySwitch.test.tsx @@ -20,6 +20,19 @@ import { render } from '@testing-library/react'; import React from 'react'; import { isKind } from './conditions'; import { EntitySwitch } from './EntitySwitch'; +import { featureFlagsApiRef } from '@backstage/core-plugin-api'; +import { + LocalStorageFeatureFlags, + ApiProvider, + ApiRegistry, +} from '@backstage/core-app-api'; + +const mockFeatureFlagsApi = new LocalStorageFeatureFlags(); +const Wrapper = ({ children }: { children?: React.ReactNode }) => ( + + {children} + +); describe('EntitySwitch', () => { it('should switch child when entity switches', () => { @@ -32,15 +45,17 @@ describe('EntitySwitch', () => { ); const rendered = render( - - {content} - , + + + {content} + + , ); expect(rendered.queryByText('A')).toBeInTheDocument(); @@ -48,15 +63,17 @@ describe('EntitySwitch', () => { expect(rendered.queryByText('C')).not.toBeInTheDocument(); rendered.rerender( - - {content} - , + + + {content} + + , ); expect(rendered.queryByText('A')).not.toBeInTheDocument(); @@ -64,15 +81,17 @@ describe('EntitySwitch', () => { expect(rendered.queryByText('C')).not.toBeInTheDocument(); rendered.rerender( - - {content} - , + + + {content} + + , ); expect(rendered.queryByText('A')).not.toBeInTheDocument(); @@ -88,24 +107,28 @@ describe('EntitySwitch', () => { }; const rendered = render( - - - - - - , + + + + + + + + , ); expect(rendered.queryByText('A')).toBeInTheDocument(); expect(rendered.queryByText('B')).not.toBeInTheDocument(); rendered.rerender( - - - - - - , + + + + + + + + , ); expect(rendered.queryByText('A')).not.toBeInTheDocument(); diff --git a/plugins/catalog/src/components/EntitySwitch/EntitySwitch.tsx b/plugins/catalog/src/components/EntitySwitch/EntitySwitch.tsx index a3ede42288..4b12a7249b 100644 --- a/plugins/catalog/src/components/EntitySwitch/EntitySwitch.tsx +++ b/plugins/catalog/src/components/EntitySwitch/EntitySwitch.tsx @@ -16,49 +16,40 @@ import { Entity } from '@backstage/catalog-model'; import { useEntity } from '@backstage/plugin-catalog-react'; +import { PropsWithChildren, ReactNode } from 'react'; import { - Children, - Fragment, - isValidElement, - PropsWithChildren, - ReactNode, - useMemo, -} from 'react'; + attachComponentData, + useElementFilter, +} from '@backstage/core-plugin-api'; + +const ENTITY_SWITCH_KEY = 'core.backstage.entitySwitch'; const EntitySwitchCase = (_: { if?: (entity: Entity) => boolean; children: ReactNode; }) => null; +attachComponentData(EntitySwitchCase, ENTITY_SWITCH_KEY, true); + type SwitchCase = { if?: (entity: Entity) => boolean; children: JSX.Element; }; -function createSwitchCasesFromChildren(childrenNode: ReactNode): SwitchCase[] { - return Children.toArray(childrenNode).flatMap(child => { - if (!isValidElement(child)) { - return []; - } - - if (child.type === Fragment) { - return createSwitchCasesFromChildren(child.props.children); - } - - if (child.type !== EntitySwitchCase) { - throw new Error(`Child of EntitySwitch is not an EntitySwitch.Case`); - } - - const { if: condition, children } = child.props; - return [{ if: condition, children }]; - }); -} - export const EntitySwitch = ({ children }: PropsWithChildren<{}>) => { const { entity } = useEntity(); - const switchCases = useMemo(() => createSwitchCasesFromChildren(children), [ - children, - ]); + const switchCases = useElementFilter(children, collection => + collection + .selectByComponentData({ + key: ENTITY_SWITCH_KEY, + withStrictError: 'Child of EntitySwitch is not an EntitySwitch.Case', + }) + .getElements() + .flatMap((element: React.ReactElement) => { + const { if: condition, children: elementsChildren } = element.props; + return [{ if: condition, children: elementsChildren }]; + }), + ); const matchingCase = switchCases.find(switchCase => switchCase.if ? switchCase.if(entity) : true, From 34900b0ca6e860512ee6bdf1d939c2495df1bbe3 Mon Sep 17 00:00:00 2001 From: blam Date: Tue, 15 Jun 2021 12:00:39 +0200 Subject: [PATCH 19/24] chore: removing the test usages in the app, and fixing the typings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Fredrik Adelöw Co-authored-by: Johan Haals Signed-off-by: blam --- packages/app/src/App.tsx | 4 +--- packages/app/src/components/catalog/EntityPage.tsx | 8 +++----- packages/core-app-api/src/routing/FeatureFlagged.tsx | 6 ++++-- 3 files changed, 8 insertions(+), 10 deletions(-) diff --git a/packages/app/src/App.tsx b/packages/app/src/App.tsx index de11afeed4..a7ed41d2a4 100644 --- a/packages/app/src/App.tsx +++ b/packages/app/src/App.tsx @@ -114,9 +114,7 @@ const routes = ( path="/tech-radar" element={} /> - - } /> - + } /> } /> } /> diff --git a/packages/app/src/components/catalog/EntityPage.tsx b/packages/app/src/components/catalog/EntityPage.tsx index 41d2d6f8c2..d1c5d44f85 100644 --- a/packages/app/src/components/catalog/EntityPage.tsx +++ b/packages/app/src/components/catalog/EntityPage.tsx @@ -306,11 +306,9 @@ const serviceEntityPage = ( - - - - - + + + diff --git a/packages/core-app-api/src/routing/FeatureFlagged.tsx b/packages/core-app-api/src/routing/FeatureFlagged.tsx index 809eb7c69c..40ef445f92 100644 --- a/packages/core-app-api/src/routing/FeatureFlagged.tsx +++ b/packages/core-app-api/src/routing/FeatureFlagged.tsx @@ -19,11 +19,13 @@ import { useApi, attachComponentData, } from '@backstage/core-plugin-api'; +import React, { ReactNode } from 'react'; -export type FeatureFlaggedProps = { children: JSX.Element | null } & ( +export type FeatureFlaggedProps = { children: ReactNode } & ( | { with: string } | { without: string } ); + export const FeatureFlagged = (props: FeatureFlaggedProps) => { const { children } = props; const featureFlagApi = useApi(featureFlagsApiRef); @@ -31,7 +33,7 @@ export const FeatureFlagged = (props: FeatureFlaggedProps) => { 'with' in props ? featureFlagApi.isActive(props.with) : !featureFlagApi.isActive(props.without); - return isEnabled ? children : null; + return <>{isEnabled ? children : null}; }; attachComponentData(FeatureFlagged, 'core.featureFlagged', true); From 14770c4f3ab8140addb76d7fd7007b8afd65399c Mon Sep 17 00:00:00 2001 From: blam Date: Tue, 15 Jun 2021 12:22:26 +0200 Subject: [PATCH 20/24] chore: tidy up the app and fix tsc warnings Signed-off-by: blam --- packages/app/src/App.tsx | 2 +- packages/app/src/components/catalog/EntityPage.tsx | 9 +++------ 2 files changed, 4 insertions(+), 7 deletions(-) diff --git a/packages/app/src/App.tsx b/packages/app/src/App.tsx index a7ed41d2a4..cc65cd65b3 100644 --- a/packages/app/src/App.tsx +++ b/packages/app/src/App.tsx @@ -14,7 +14,7 @@ * limitations under the License. */ -import { createApp, FlatRoutes, FeatureFlagged } from '@backstage/core-app-api'; +import { createApp, FlatRoutes } from '@backstage/core-app-api'; import { AlertDisplay, OAuthRequestDialog, diff --git a/packages/app/src/components/catalog/EntityPage.tsx b/packages/app/src/components/catalog/EntityPage.tsx index d1c5d44f85..f646992c97 100644 --- a/packages/app/src/components/catalog/EntityPage.tsx +++ b/packages/app/src/components/catalog/EntityPage.tsx @@ -108,7 +108,6 @@ import { isTravisciAvailable, } from '@roadiehq/backstage-plugin-travis-ci'; import { EntityCodeCoverageContent } from '@backstage/plugin-code-coverage'; -import { FeatureFlagged } from '@backstage/core-app-api'; const EntityLayoutWrapper = (props: { children?: ReactNode }) => { const [badgesDialogOpen, setBadgesDialogOpen] = useState(false); @@ -283,11 +282,9 @@ const serviceEntityPage = ( {overviewContent} - - - {cicdContent} - - + + {cicdContent} + {errorsContent} From d00a3fcd95ba855aa23154b381d54c6b1e00e488 Mon Sep 17 00:00:00 2001 From: blam Date: Tue, 15 Jun 2021 12:24:36 +0200 Subject: [PATCH 21/24] chore: updated API report Signed-off-by: blam --- packages/core-app-api/api-report.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/core-app-api/api-report.md b/packages/core-app-api/api-report.md index 9b68d2a10d..5aa709121a 100644 --- a/packages/core-app-api/api-report.md +++ b/packages/core-app-api/api-report.md @@ -235,11 +235,11 @@ export type ErrorBoundaryFallbackProps = { }; // @public (undocumented) -export const FeatureFlagged: (props: FeatureFlaggedProps) => JSX.Element | null; +export const FeatureFlagged: (props: FeatureFlaggedProps) => JSX.Element; // @public (undocumented) export type FeatureFlaggedProps = { - children: JSX.Element | null; + children: ReactNode; } & ({ with: string; } | { From 4d63ce7c1b6214121abca9ba0e318470292a0a00 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Wed, 16 Jun 2021 17:42:06 +0200 Subject: [PATCH 22/24] core-plugin-api: make useElementFilter filter for undefined component data instead of falsy Signed-off-by: Patrik Oldsberg --- .../src/extensions/useElementFilter.tsx | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/packages/core-plugin-api/src/extensions/useElementFilter.tsx b/packages/core-plugin-api/src/extensions/useElementFilter.tsx index b33c23e422..fef777cb95 100644 --- a/packages/core-plugin-api/src/extensions/useElementFilter.tsx +++ b/packages/core-plugin-api/src/extensions/useElementFilter.tsx @@ -88,19 +88,21 @@ class ElementCollection { const selection = selectChildren( this.node, this.featureFlagsApi, - node => Boolean(getComponentData(node, query.key)), + node => getComponentData(node, query.key) !== undefined, query.withStrictError, ); return new ElementCollection(selection, this.featureFlagsApi); } findComponentData(query: { key: string }): T[] { - const selection = selectChildren(this.node, this.featureFlagsApi, node => - Boolean(getComponentData(node, query.key)), + const selection = selectChildren( + this.node, + this.featureFlagsApi, + node => getComponentData(node, query.key) !== undefined, ); return selection .map(node => getComponentData(node, query.key)) - .filter((data: T | undefined): data is T => Boolean(data)); + .filter((data: T | undefined): data is T => data !== undefined); } getElements(): Array< From 814b3d0dc278e138898041f3d573acd7fc2a45d5 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Wed, 16 Jun 2021 17:51:54 +0200 Subject: [PATCH 23/24] core-plugin-api: document useElementFilter Signed-off-by: Patrik Oldsberg --- .../src/extensions/useElementFilter.test.tsx | 4 +- .../src/extensions/useElementFilter.tsx | 39 +++++++++++++++++++ 2 files changed, 42 insertions(+), 1 deletion(-) diff --git a/packages/core-plugin-api/src/extensions/useElementFilter.test.tsx b/packages/core-plugin-api/src/extensions/useElementFilter.test.tsx index bb2bcb06a5..73fbb9e5a3 100644 --- a/packages/core-plugin-api/src/extensions/useElementFilter.test.tsx +++ b/packages/core-plugin-api/src/extensions/useElementFilter.test.tsx @@ -59,7 +59,9 @@ describe('useElementFilter', () => { - + + + diff --git a/packages/core-plugin-api/src/extensions/useElementFilter.tsx b/packages/core-plugin-api/src/extensions/useElementFilter.tsx index fef777cb95..e79d2ba508 100644 --- a/packages/core-plugin-api/src/extensions/useElementFilter.tsx +++ b/packages/core-plugin-api/src/extensions/useElementFilter.tsx @@ -84,6 +84,22 @@ class ElementCollection { private readonly featureFlagsApi: FeatureFlagsApi, ) {} + /** + * Narrows the set of selected components by doing a deep traversal and + * only including those that have defined component data for the given `key`. + * + * Whether an element in the tree has component data set for the given key + * is determined by whether `getComponentData` returns undefined. + * + * The traversal does not continue deeper past elements that match the criteria, + * and it also includes the root children in the selection, meaning that if the, + * of all the currently selected elements contain data for the given key, this + * method is a no-op. + * + * If `withStrictError` is set, the resulting selection must be a full match, meaning + * there may be no elements that were excluded in the selection. If the selection + * is not a clean match, an error will be throw with `withStrictError` as the message. + */ selectByComponentData(query: { key: string; withStrictError?: string }) { const selection = selectChildren( this.node, @@ -94,6 +110,10 @@ class ElementCollection { return new ElementCollection(selection, this.featureFlagsApi); } + /** + * Finds all elements using the same criteria as `selectByComponentData`, but + * returns the actual component data of each of those elements instead. + */ findComponentData(query: { key: string }): T[] { const selection = selectChildren( this.node, @@ -105,6 +125,9 @@ class ElementCollection { .filter((data: T | undefined): data is T => data !== undefined); } + /** + * Returns all of the elements currently selected by this collection. + */ getElements(): Array< ReactElement > { @@ -114,6 +137,22 @@ class ElementCollection { } } +/** + * useElementFilter is a utility that helps you narrow down and retrieve data + * from a React element tree, typically operating on the `children` property + * passed in to a component. A common use-case is to construct declarative APIs + * where a React component defines its behavior based on its children, such as + * the relationship between `Routes` and `Route` in `react-router`. + * + * The purpose of this hook is similar to `React.Children.map`, and it expands upon + * it to also handle traversal of fragments and Backstage specific things like the + * `FeatureFlagged` component. + * + * The return value of the hook is computed by the provided filter function, but + * with added memoization based on the input `node`. If further memoization + * dependencies are used in the filter function, they should be added to the + * third `dependencies` argument, just like `useMemo`, `useEffect`, etc. + */ export function useElementFilter( node: ReactNode, filterFn: (arg: ElementCollection) => T, From 64b53d4829e6b398cccb29f6625c95b212eb4386 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Wed, 16 Jun 2021 18:31:12 +0200 Subject: [PATCH 24/24] core-plugin-api: export ElementCollection interface + document Signed-off-by: Patrik Oldsberg --- packages/core-plugin-api/api-report.md | 16 +++++- .../core-plugin-api/src/extensions/index.ts | 1 + .../src/extensions/useElementFilter.tsx | 53 +++++++++++++------ 3 files changed, 54 insertions(+), 16 deletions(-) diff --git a/packages/core-plugin-api/api-report.md b/packages/core-plugin-api/api-report.md index 6e8cda3d83..90ea1f5e46 100644 --- a/packages/core-plugin-api/api-report.md +++ b/packages/core-plugin-api/api-report.md @@ -228,6 +228,20 @@ export type DiscoveryApi = { // @public (undocumented) export const discoveryApiRef: ApiRef; +// @public +export interface ElementCollection { + findComponentData(query: { + key: string; + }): T[]; + getElements(): Array>; + selectByComponentData(query: { + key: string; + withStrictError?: string; + }): ElementCollection; +} + // @public export type ErrorApi = { post(error: Error_2, context?: ErrorContext): void; @@ -515,7 +529,7 @@ export function useApiHolder(): ApiHolder; // @public (undocumented) export const useApp: () => AppContext; -// @public (undocumented) +// @public export function useElementFilter(node: ReactNode, filterFn: (arg: ElementCollection) => T, dependencies?: any[]): T; // @public (undocumented) diff --git a/packages/core-plugin-api/src/extensions/index.ts b/packages/core-plugin-api/src/extensions/index.ts index 71373db1a3..0ee1447b4e 100644 --- a/packages/core-plugin-api/src/extensions/index.ts +++ b/packages/core-plugin-api/src/extensions/index.ts @@ -21,3 +21,4 @@ export { createComponentExtension, } from './extensions'; export { useElementFilter } from './useElementFilter'; +export type { ElementCollection } from './useElementFilter'; diff --git a/packages/core-plugin-api/src/extensions/useElementFilter.tsx b/packages/core-plugin-api/src/extensions/useElementFilter.tsx index e79d2ba508..0549472ab2 100644 --- a/packages/core-plugin-api/src/extensions/useElementFilter.tsx +++ b/packages/core-plugin-api/src/extensions/useElementFilter.tsx @@ -78,12 +78,17 @@ function selectChildren( }); } -class ElementCollection { - constructor( - private readonly node: ReactNode, - private readonly featureFlagsApi: FeatureFlagsApi, - ) {} - +/** + * A querying interface tailored to traversing a set of selected React elements + * and extracting data. + * + * Methods prefixed with `selectBy` are used to narrow the set of selected elements. + * + * Methods prefixed with `find` return concrete data using a deep traversal of the set. + * + * Methods prefixed with `get` return concrete data using a shallow traversal of the set. + */ +export interface ElementCollection { /** * Narrows the set of selected components by doing a deep traversal and * only including those that have defined component data for the given `key`. @@ -100,6 +105,31 @@ class ElementCollection { * there may be no elements that were excluded in the selection. If the selection * is not a clean match, an error will be throw with `withStrictError` as the message. */ + selectByComponentData(query: { + key: string; + withStrictError?: string; + }): ElementCollection; + + /** + * Finds all elements using the same criteria as `selectByComponentData`, but + * returns the actual component data of each of those elements instead. + */ + findComponentData(query: { key: string }): T[]; + + /** + * Returns all of the elements currently selected by this collection. + */ + getElements(): Array< + ReactElement + >; +} + +class Collection implements ElementCollection { + constructor( + private readonly node: ReactNode, + private readonly featureFlagsApi: FeatureFlagsApi, + ) {} + selectByComponentData(query: { key: string; withStrictError?: string }) { const selection = selectChildren( this.node, @@ -107,13 +137,9 @@ class ElementCollection { node => getComponentData(node, query.key) !== undefined, query.withStrictError, ); - return new ElementCollection(selection, this.featureFlagsApi); + return new Collection(selection, this.featureFlagsApi); } - /** - * Finds all elements using the same criteria as `selectByComponentData`, but - * returns the actual component data of each of those elements instead. - */ findComponentData(query: { key: string }): T[] { const selection = selectChildren( this.node, @@ -125,9 +151,6 @@ class ElementCollection { .filter((data: T | undefined): data is T => data !== undefined); } - /** - * Returns all of the elements currently selected by this collection. - */ getElements(): Array< ReactElement > { @@ -159,7 +182,7 @@ export function useElementFilter( dependencies: any[] = [], ) { const featureFlagsApi = useApi(featureFlagsApiRef); - const elements = new ElementCollection(node, featureFlagsApi); + const elements = new Collection(node, featureFlagsApi); // eslint-disable-next-line react-hooks/exhaustive-deps return useMemo(() => filterFn(elements), [node, ...dependencies]); }