From f877061abc2258d90d49c87a14bc4153939cf32f Mon Sep 17 00:00:00 2001 From: Brian Fletcher Date: Wed, 7 Dec 2022 14:45:47 +0000 Subject: [PATCH] some code review comments addressed Signed-off-by: Brian Fletcher --- .../examples/acme/team-a-group.yaml | 2 +- .../src/service/createRouter.ts | 1 + .../EntityRefLink/EntityRefLink.test.tsx | 79 ++++++------------- .../EntityRefLink/EntityRefLink.tsx | 54 ++++++------- .../EntityRefLink/EntityRefLinks.test.tsx | 37 +++------ .../components/EntityTable/presets.test.tsx | 39 +++------ plugins/playlist/package.json | 3 +- .../PlaylistCard/PlaylistCard.test.tsx | 38 ++++----- .../techdocs-addons-test-utils/package.json | 4 +- .../src/test-utils.tsx | 7 -- .../package.json | 5 +- plugins/techdocs/package.json | 3 +- .../TechDocsReaderPage.test.tsx | 8 +- yarn.lock | 23 ++++-- 14 files changed, 106 insertions(+), 197 deletions(-) diff --git a/packages/catalog-model/examples/acme/team-a-group.yaml b/packages/catalog-model/examples/acme/team-a-group.yaml index 6f2be963b7..e343209d5f 100644 --- a/packages/catalog-model/examples/acme/team-a-group.yaml +++ b/packages/catalog-model/examples/acme/team-a-group.yaml @@ -21,7 +21,7 @@ spec: # Intentional no displayName for testing email: breanna-davison@example.com picture: https://avatars.dicebear.com/api/avataaars/breanna-davison@example.com.svg?background=%23fff - memberOf: [team-a, team-not-exist] + memberOf: [team-a] --- apiVersion: backstage.io/v1alpha1 kind: User diff --git a/plugins/catalog-backend/src/service/createRouter.ts b/plugins/catalog-backend/src/service/createRouter.ts index 4680dbb899..e317771b92 100644 --- a/plugins/catalog-backend/src/service/createRouter.ts +++ b/plugins/catalog-backend/src/service/createRouter.ts @@ -147,6 +147,7 @@ export async function createRouter( res.status(204).end(); }) .get('/entities/by-name/:kind/:namespace/:name', async (req, res) => { + await new Promise(resolve => setTimeout(resolve, 3000)); const { kind, namespace, name } = req.params; const { entities } = await entitiesCatalog.entities({ filter: basicEntityFilter({ diff --git a/plugins/catalog-react/src/components/EntityRefLink/EntityRefLink.test.tsx b/plugins/catalog-react/src/components/EntityRefLink/EntityRefLink.test.tsx index 71ff25c746..b0a6b46bf5 100644 --- a/plugins/catalog-react/src/components/EntityRefLink/EntityRefLink.test.tsx +++ b/plugins/catalog-react/src/components/EntityRefLink/EntityRefLink.test.tsx @@ -14,20 +14,11 @@ * limitations under the License. */ -import { renderInTestApp, TestApiRegistry } from '@backstage/test-utils'; +import { renderInTestApp } from '@backstage/test-utils'; import { screen } from '@testing-library/react'; import React from 'react'; import { entityRouteRef } from '../../routes'; import { EntityRefLink } from './EntityRefLink'; -import { catalogApiRef } from '../../api'; -import { CatalogApi } from '@backstage/catalog-client'; -import { ApiProvider } from '@backstage/core-app-api'; - -const catalogApi: jest.Mocked = { - getEntityByRef: jest.fn(), -} as any; - -const apis = TestApiRegistry.from([catalogApiRef, catalogApi]); describe('', () => { it('renders link for entity in default namespace', async () => { @@ -43,16 +34,11 @@ describe('', () => { lifecycle: 'production', }, }; - await renderInTestApp( - - - , - { - mountedRoutes: { - '/catalog/:namespace/:kind/:name/*': entityRouteRef, - }, + await renderInTestApp(, { + mountedRoutes: { + '/catalog/:namespace/:kind/:name/*': entityRouteRef, }, - ); + }); expect(screen.getByText('component:software')).toHaveAttribute( 'href', @@ -74,16 +60,11 @@ describe('', () => { lifecycle: 'production', }, }; - await renderInTestApp( - - - , - { - mountedRoutes: { - '/catalog/:namespace/:kind/:name/*': entityRouteRef, - }, + await renderInTestApp(, { + mountedRoutes: { + '/catalog/:namespace/:kind/:name/*': entityRouteRef, }, - ); + }); expect(screen.getByText('component:test/software')).toHaveAttribute( 'href', '/catalog/test/component/software', @@ -105,9 +86,7 @@ describe('', () => { }, }; await renderInTestApp( - - - , + , { mountedRoutes: { '/catalog/:namespace/:kind/:name/*': entityRouteRef, @@ -126,16 +105,11 @@ describe('', () => { namespace: 'default', name: 'software', }; - await renderInTestApp( - - - , - { - mountedRoutes: { - '/catalog/:namespace/:kind/:name/*': entityRouteRef, - }, + await renderInTestApp(, { + mountedRoutes: { + '/catalog/:namespace/:kind/:name/*': entityRouteRef, }, - ); + }); expect(screen.getByText('component:software')).toHaveAttribute( 'href', '/catalog/default/component/software', @@ -148,16 +122,11 @@ describe('', () => { namespace: 'test', name: 'software', }; - await renderInTestApp( - - - , - { - mountedRoutes: { - '/catalog/:namespace/:kind/:name/*': entityRouteRef, - }, + await renderInTestApp(, { + mountedRoutes: { + '/catalog/:namespace/:kind/:name/*': entityRouteRef, }, - ); + }); expect(screen.getByText('component:test/software')).toHaveAttribute( 'href', '/catalog/test/component/software', @@ -171,9 +140,7 @@ describe('', () => { name: 'software', }; await renderInTestApp( - - - , + , { mountedRoutes: { '/catalog/:namespace/:kind/:name/*': entityRouteRef, @@ -193,11 +160,9 @@ describe('', () => { name: 'software', }; await renderInTestApp( - - - Custom Children - - , + + Custom Children + , { mountedRoutes: { '/catalog/:namespace/:kind/:name/*': entityRouteRef, diff --git a/plugins/catalog-react/src/components/EntityRefLink/EntityRefLink.tsx b/plugins/catalog-react/src/components/EntityRefLink/EntityRefLink.tsx index c1d699421e..7cde1662cf 100644 --- a/plugins/catalog-react/src/components/EntityRefLink/EntityRefLink.tsx +++ b/plugins/catalog-react/src/components/EntityRefLink/EntityRefLink.tsx @@ -25,8 +25,8 @@ import { import React, { forwardRef } from 'react'; import { entityRouteRef } from '../../routes'; import { humanizeEntityRef } from './humanize'; -import { Link, LinkProps } from '@backstage/core-components'; -import { useApi, useRouteRef } from '@backstage/core-plugin-api'; +import { Link, LinkProps, Progress } from '@backstage/core-components'; +import { useApiHolder, useRouteRef } from '@backstage/core-plugin-api'; import { Button, Tooltip, @@ -80,7 +80,7 @@ export const PeekAheadPopover = ({ }: PeekAheadPopoverProps) => { const entityRoute = useRouteRef(entityRouteRef); const classes = useStyles(); - const catalogApi = useApi(catalogApiRef); + const apiHolder = useApiHolder(); const { value: entity, @@ -88,18 +88,17 @@ export const PeekAheadPopover = ({ error, } = useAsync(async () => { if (popupState.isOpen) { - const retrievedEntity = await catalogApi.getEntityByRef(entityRef); - if (!retrievedEntity) { - throw new Error(`${entityRef.name} was not found`); + const catalogApi = apiHolder.get(catalogApiRef); + if (catalogApi) { + const retrievedEntity = await catalogApi.getEntityByRef(entityRef); + if (!retrievedEntity) { + throw new Error(`${entityRef.name} was not found`); + } + return retrievedEntity; } - return retrievedEntity; } return undefined; - }, [popupState]); - - if (loading) { - return null; - } + }, [popupState.isOpen, apiHolder, entityRef]); return ( + {loading && } - {entityRef.namespace} + {entityRef.namespace} {entityRef.name} - {entityRef.kind} + {entityRef.kind} {error && {error.message}} {entity && ( @@ -201,25 +201,17 @@ export const EntityRefLink = forwardRef( { defaultKind }, ); - const link = ( - - {children} - {!children && (title ?? formattedEntityRefTitle)} - - ); - return ( <> - {title ? ( - {link} - ) : ( - link - )} + + {children} + {!children && (title ?? formattedEntityRefTitle)} + = { - getEntityByRef: jest.fn(), -} as any; - -const apis = TestApiRegistry.from([catalogApiRef, catalogApi]); describe('', () => { it('renders a single link', async () => { @@ -38,16 +29,11 @@ describe('', () => { name: 'software', }, ]; - await renderInTestApp( - - - , - { - mountedRoutes: { - '/catalog/:namespace/:kind/:name/*': entityRouteRef, - }, + await renderInTestApp(, { + mountedRoutes: { + '/catalog/:namespace/:kind/:name/*': entityRouteRef, }, - ); + }); expect(screen.getByText('component:software')).toHaveAttribute( 'href', '/catalog/default/component/software', @@ -67,16 +53,11 @@ describe('', () => { name: 'interface', }, ]; - await renderInTestApp( - - - , - { - mountedRoutes: { - '/catalog/:namespace/:kind/:name/*': entityRouteRef, - }, + await renderInTestApp(, { + mountedRoutes: { + '/catalog/:namespace/:kind/:name/*': entityRouteRef, }, - ); + }); expect(screen.getByText(',')).toBeInTheDocument(); expect(screen.getByText('component:software')).toHaveAttribute( 'href', diff --git a/plugins/catalog-react/src/components/EntityTable/presets.test.tsx b/plugins/catalog-react/src/components/EntityTable/presets.test.tsx index 8b8d729287..82075df5e9 100644 --- a/plugins/catalog-react/src/components/EntityTable/presets.test.tsx +++ b/plugins/catalog-react/src/components/EntityTable/presets.test.tsx @@ -20,21 +20,12 @@ import { RELATION_PART_OF, SystemEntity, } from '@backstage/catalog-model'; -import { renderInTestApp, TestApiRegistry } from '@backstage/test-utils'; +import { renderInTestApp } from '@backstage/test-utils'; import { waitFor, screen } from '@testing-library/react'; import React from 'react'; import { entityRouteRef } from '../../routes'; import { EntityTable } from './EntityTable'; import { componentEntityColumns, systemEntityColumns } from './presets'; -import { catalogApiRef } from '../../api'; -import { CatalogApi } from '@backstage/catalog-client'; -import { ApiProvider } from '@backstage/core-app-api'; - -const catalogApi: jest.Mocked = { - getEntityByRef: jest.fn(), -} as any; - -const apis = TestApiRegistry.from([catalogApiRef, catalogApi]); describe('systemEntityColumns', () => { it('shows systems', async () => { @@ -64,14 +55,12 @@ describe('systemEntityColumns', () => { ]; await renderInTestApp( - - EMPTY} - columns={systemEntityColumns} - /> - , + EMPTY} + columns={systemEntityColumns} + />, { mountedRoutes: { '/catalog/:namespace/:kind/:name/*': entityRouteRef, @@ -118,14 +107,12 @@ describe('componentEntityColumns', () => { ]; await renderInTestApp( - - EMPTY} - columns={componentEntityColumns} - /> - , + EMPTY} + columns={componentEntityColumns} + />, { mountedRoutes: { '/catalog/:namespace/:kind/:name/*': entityRouteRef, diff --git a/plugins/playlist/package.json b/plugins/playlist/package.json index d0fd141bb3..a899812568 100644 --- a/plugins/playlist/package.json +++ b/plugins/playlist/package.json @@ -1,6 +1,6 @@ { "name": "@backstage/plugin-playlist", - "version": "0.1.3-next.1", + "version": "0.1.3-next.2", "main": "src/index.ts", "types": "src/index.ts", "license": "Apache-2.0", @@ -52,7 +52,6 @@ "react-router-dom": "6.0.0-beta.0 || ^6.3.0" }, "devDependencies": { - "@backstage/catalog-client": "workspace:^", "@backstage/cli": "workspace:^", "@backstage/core-app-api": "workspace:^", "@backstage/dev-utils": "workspace:^", diff --git a/plugins/playlist/src/components/PlaylistCard/PlaylistCard.test.tsx b/plugins/playlist/src/components/PlaylistCard/PlaylistCard.test.tsx index 82b5eb564a..177cc23b81 100644 --- a/plugins/playlist/src/components/PlaylistCard/PlaylistCard.test.tsx +++ b/plugins/playlist/src/components/PlaylistCard/PlaylistCard.test.tsx @@ -14,19 +14,11 @@ * limitations under the License. */ -import { entityRouteRef, catalogApiRef } from '@backstage/plugin-catalog-react'; -import { renderInTestApp, TestApiRegistry } from '@backstage/test-utils'; +import { entityRouteRef } from '@backstage/plugin-catalog-react'; +import { renderInTestApp } from '@backstage/test-utils'; import { lightTheme } from '@backstage/theme'; import { ThemeProvider } from '@material-ui/core'; import React from 'react'; -import { CatalogApi } from '@backstage/catalog-client'; -import { ApiProvider } from '@backstage/core-app-api'; - -const catalogApi: jest.Mocked = { - getEntityByRef: jest.fn(), -} as any; - -const apis = TestApiRegistry.from([catalogApiRef, catalogApi]); import { rootRouteRef } from '../../routes'; import { PlaylistCard } from './PlaylistCard'; @@ -35,20 +27,18 @@ describe('', () => { it('renders playlist info', async () => { const rendered = await renderInTestApp( - - - + , { mountedRoutes: { diff --git a/plugins/techdocs-addons-test-utils/package.json b/plugins/techdocs-addons-test-utils/package.json index 907adfbb65..5521a4f73c 100644 --- a/plugins/techdocs-addons-test-utils/package.json +++ b/plugins/techdocs-addons-test-utils/package.json @@ -1,6 +1,6 @@ { "name": "@backstage/plugin-techdocs-addons-test-utils", - "version": "1.0.7-next.1", + "version": "1.0.7-next.2", "main": "src/index.ts", "types": "src/index.ts", "license": "Apache-2.0", @@ -32,13 +32,11 @@ "postpack": "backstage-cli package postpack" }, "dependencies": { - "@backstage/catalog-client": "workspace:^", "@backstage/core-app-api": "workspace:^", "@backstage/core-components": "workspace:^", "@backstage/core-plugin-api": "workspace:^", "@backstage/integration-react": "workspace:^", "@backstage/plugin-catalog": "workspace:^", - "@backstage/plugin-catalog-react": "workspace:^", "@backstage/plugin-search-react": "workspace:^", "@backstage/plugin-techdocs": "workspace:^", "@backstage/plugin-techdocs-react": "workspace:^", diff --git a/plugins/techdocs-addons-test-utils/src/test-utils.tsx b/plugins/techdocs-addons-test-utils/src/test-utils.tsx index 8611bddad4..0d65e7e51e 100644 --- a/plugins/techdocs-addons-test-utils/src/test-utils.tsx +++ b/plugins/techdocs-addons-test-utils/src/test-utils.tsx @@ -22,8 +22,6 @@ import { screen } from 'testing-library__dom'; import { renderToStaticMarkup } from 'react-dom/server'; import { Route } from 'react-router-dom'; import { act, render } from '@testing-library/react'; -import { CatalogApi } from '@backstage/catalog-client'; -import { catalogApiRef } from '@backstage/plugin-catalog-react'; import { wrapInTestApp, TestApiProvider } from '@backstage/test-utils'; import { FlatRoutes } from '@backstage/core-app-api'; @@ -61,10 +59,6 @@ const scmIntegrationsApi = { fromConfig: jest.fn().mockReturnValue({}), }; -const catalogApi: jest.Mocked = { - getEntityByRef: jest.fn(), -} as any; - /** @ignore */ type TechDocsAddonTesterTestApiPair = TApi extends infer TImpl ? readonly [ApiRef, Partial] @@ -204,7 +198,6 @@ export class TechDocsAddonTester { [techdocsStorageApiRef, techdocsStorageApi], [searchApiRef, searchApi], [scmIntegrationsApiRef, scmIntegrationsApi], - [catalogApiRef, catalogApi], ...this.options.apis, ]; diff --git a/plugins/techdocs-module-addons-contrib/package.json b/plugins/techdocs-module-addons-contrib/package.json index 3eec1080b1..258bab028e 100644 --- a/plugins/techdocs-module-addons-contrib/package.json +++ b/plugins/techdocs-module-addons-contrib/package.json @@ -1,7 +1,7 @@ { "name": "@backstage/plugin-techdocs-module-addons-contrib", "description": "Plugin module for contributed TechDocs Addons", - "version": "1.0.7-next.1", + "version": "1.0.7-next.2", "main": "src/index.ts", "types": "src/index.ts", "license": "Apache-2.0", @@ -42,7 +42,7 @@ "@material-ui/core": "^4.9.13", "@material-ui/icons": "^4.9.1", "@material-ui/lab": "4.0.0-alpha.57", - "@react-hookz/web": "^19.0.0", + "@react-hookz/web": "^20.0.0", "git-url-parse": "^13.0.0", "react-use": "^17.2.4" }, @@ -50,7 +50,6 @@ "react": "^16.13.1 || ^17.0.0" }, "devDependencies": { - "@backstage/catalog-client": "workspace:^", "@backstage/cli": "workspace:^", "@backstage/core-app-api": "workspace:^", "@backstage/dev-utils": "workspace:^", diff --git a/plugins/techdocs/package.json b/plugins/techdocs/package.json index 32f470a4d0..4a0fcf1689 100644 --- a/plugins/techdocs/package.json +++ b/plugins/techdocs/package.json @@ -1,7 +1,7 @@ { "name": "@backstage/plugin-techdocs", "description": "The Backstage plugin that renders technical documentation for your components", - "version": "1.4.1-next.1", + "version": "1.4.1-next.2", "main": "src/index.ts", "types": "src/index.ts", "license": "Apache-2.0", @@ -65,7 +65,6 @@ "react-router-dom": "6.0.0-beta.0 || ^6.3.0" }, "devDependencies": { - "@backstage/catalog-client": "workspace:^", "@backstage/cli": "workspace:^", "@backstage/core-app-api": "workspace:^", "@backstage/dev-utils": "workspace:^", diff --git a/plugins/techdocs/src/reader/components/TechDocsReaderPage/TechDocsReaderPage.test.tsx b/plugins/techdocs/src/reader/components/TechDocsReaderPage/TechDocsReaderPage.test.tsx index 813ee173dc..4b23075908 100644 --- a/plugins/techdocs/src/reader/components/TechDocsReaderPage/TechDocsReaderPage.test.tsx +++ b/plugins/techdocs/src/reader/components/TechDocsReaderPage/TechDocsReaderPage.test.tsx @@ -17,10 +17,9 @@ import React from 'react'; import { act } from '@testing-library/react'; import { ThemeProvider } from '@material-ui/core'; import { scmIntegrationsApiRef } from '@backstage/integration-react'; -import { CatalogApi } from '@backstage/catalog-client'; import { lightTheme } from '@backstage/theme'; -import { catalogApiRef, entityRouteRef } from '@backstage/plugin-catalog-react'; +import { entityRouteRef } from '@backstage/plugin-catalog-react'; import { renderInTestApp, TestApiProvider } from '@backstage/test-utils'; import { techdocsApiRef, techdocsStorageApiRef } from '../../../api'; @@ -67,10 +66,6 @@ const techdocsStorageApiMock: jest.Mocked = { syncEntityDocs: jest.fn(), }; -const catalogApi: jest.Mocked = { - getEntityByRef: jest.fn(), -} as any; - const Wrapper = ({ children }: { children: React.ReactNode }) => { return ( @@ -79,7 +74,6 @@ const Wrapper = ({ children }: { children: React.ReactNode }) => { [scmIntegrationsApiRef, {}], [techdocsApiRef, techdocsApiMock], [techdocsStorageApiRef, techdocsStorageApiMock], - [catalogApiRef, catalogApi], ]} > {children} diff --git a/yarn.lock b/yarn.lock index eb9be9a409..ef0593ee99 100644 --- a/yarn.lock +++ b/yarn.lock @@ -7189,7 +7189,6 @@ __metadata: version: 0.0.0-use.local resolution: "@backstage/plugin-playlist@workspace:plugins/playlist" dependencies: - "@backstage/catalog-client": "workspace:^" "@backstage/catalog-model": "workspace:^" "@backstage/cli": "workspace:^" "@backstage/core-app-api": "workspace:^" @@ -8059,7 +8058,6 @@ __metadata: version: 0.0.0-use.local resolution: "@backstage/plugin-techdocs-addons-test-utils@workspace:plugins/techdocs-addons-test-utils" dependencies: - "@backstage/catalog-client": "workspace:^" "@backstage/cli": "workspace:^" "@backstage/core-app-api": "workspace:^" "@backstage/core-components": "workspace:^" @@ -8067,7 +8065,6 @@ __metadata: "@backstage/dev-utils": "workspace:^" "@backstage/integration-react": "workspace:^" "@backstage/plugin-catalog": "workspace:^" - "@backstage/plugin-catalog-react": "workspace:^" "@backstage/plugin-search-react": "workspace:^" "@backstage/plugin-techdocs": "workspace:^" "@backstage/plugin-techdocs-react": "workspace:^" @@ -8129,7 +8126,6 @@ __metadata: version: 0.0.0-use.local resolution: "@backstage/plugin-techdocs-module-addons-contrib@workspace:plugins/techdocs-module-addons-contrib" dependencies: - "@backstage/catalog-client": "workspace:^" "@backstage/cli": "workspace:^" "@backstage/core-app-api": "workspace:^" "@backstage/core-components": "workspace:^" @@ -8144,7 +8140,7 @@ __metadata: "@material-ui/core": ^4.9.13 "@material-ui/icons": ^4.9.1 "@material-ui/lab": 4.0.0-alpha.57 - "@react-hookz/web": ^19.0.0 + "@react-hookz/web": ^20.0.0 "@testing-library/jest-dom": ^5.10.1 "@testing-library/react": ^12.1.3 "@testing-library/user-event": ^14.0.0 @@ -8233,7 +8229,6 @@ __metadata: version: 0.0.0-use.local resolution: "@backstage/plugin-techdocs@workspace:plugins/techdocs" dependencies: - "@backstage/catalog-client": "workspace:^" "@backstage/catalog-model": "workspace:^" "@backstage/cli": "workspace:^" "@backstage/config": "workspace:^" @@ -12454,6 +12449,22 @@ __metadata: languageName: node linkType: hard +"@react-hookz/web@npm:^20.0.0": + version: 20.0.0 + resolution: "@react-hookz/web@npm:20.0.0" + dependencies: + "@react-hookz/deep-equal": ^1.0.3 + peerDependencies: + js-cookie: ^3.0.1 + react: ^16.8 || ^17 || ^18 + react-dom: ^16.8 || ^17 || ^18 + peerDependenciesMeta: + js-cookie: + optional: true + checksum: 475d03cdd9a9131b7094549602c8a3ab52ad33e99d7ef408ba2ee7897bfdf332ab5228f46187b9a4c42535b4aaaf808e2c9c882949b2ec32d5ad42d53487a723 + languageName: node + linkType: hard + "@remix-run/router@npm:1.0.4": version: 1.0.4 resolution: "@remix-run/router@npm:1.0.4"