From 593ea01f6a05192900b6f87ab9e834de9264576a Mon Sep 17 00:00:00 2001 From: Vincenzo Scamporlino Date: Wed, 2 Aug 2023 23:17:08 +0200 Subject: [PATCH 1/5] catalog: fix EntitySwitch unmounting children on entity refresh Signed-off-by: Vincenzo Scamporlino --- .../EntitySwitch/EntitySwitch.test.tsx | 93 ++++++++++++++++++- .../components/EntitySwitch/EntitySwitch.tsx | 10 +- 2 files changed, 95 insertions(+), 8 deletions(-) diff --git a/plugins/catalog/src/components/EntitySwitch/EntitySwitch.test.tsx b/plugins/catalog/src/components/EntitySwitch/EntitySwitch.test.tsx index 13e214d823..3a8795ccb7 100644 --- a/plugins/catalog/src/components/EntitySwitch/EntitySwitch.test.tsx +++ b/plugins/catalog/src/components/EntitySwitch/EntitySwitch.test.tsx @@ -20,7 +20,7 @@ import { EntityProvider, } from '@backstage/plugin-catalog-react'; import { render, screen } from '@testing-library/react'; -import React from 'react'; +import React, { useEffect } from 'react'; import { isKind } from './conditions'; import { EntitySwitch } from './EntitySwitch'; import { featureFlagsApiRef } from '@backstage/core-plugin-api'; @@ -209,6 +209,97 @@ describe('EntitySwitch', () => { expect(screen.queryByText('B')).not.toBeInTheDocument(); }); + it('should display the children in case entity is available and loading is true', () => { + const entity = { metadata: { name: 'mock' }, kind: 'component' } as Entity; + + render( + + + + Component

} + /> + System

} /> + + + , + ); + + expect(screen.queryByText('Component')).toBeInTheDocument(); + expect(screen.queryByText('System')).not.toBeInTheDocument(); + }); + + it(`shouldn't unmount the children on entity refresh`, () => { + const entity = { metadata: { name: 'mock' }, kind: 'component' } as Entity; + + let mountsCount = 0; + function Component() { + useEffect(() => { + ++mountsCount; + }, []); + + return

Component

; + } + + const rendered = render( + + + + } + /> + System

} /> + + + , + ); + + expect(screen.queryByText('Component')).toBeInTheDocument(); + expect(screen.queryByText('System')).not.toBeInTheDocument(); + + expect(mountsCount).toBe(1); + + rendered.rerender( + + + + } + /> + System

} /> + + + , + ); + + expect(screen.queryByText('Component')).toBeInTheDocument(); + expect(screen.queryByText('System')).not.toBeInTheDocument(); + + expect(mountsCount).toBe(1); + + rendered.rerender( + + + + } + /> + System

} /> + + + , + ); + + expect(screen.queryByText('Component')).toBeInTheDocument(); + expect(screen.queryByText('System')).not.toBeInTheDocument(); + + expect(mountsCount).toBe(1); + }); + it('should switch with async condition that is true', async () => { const entity = { metadata: { name: 'mock' }, kind: 'component' } as Entity; diff --git a/plugins/catalog/src/components/EntitySwitch/EntitySwitch.tsx b/plugins/catalog/src/components/EntitySwitch/EntitySwitch.tsx index 3cd806faa5..90de358ef9 100644 --- a/plugins/catalog/src/components/EntitySwitch/EntitySwitch.tsx +++ b/plugins/catalog/src/components/EntitySwitch/EntitySwitch.tsx @@ -64,8 +64,9 @@ export interface EntitySwitchProps { /** @public */ export const EntitySwitch = (props: EntitySwitchProps) => { - const { entity, loading } = useAsyncEntity(); + const { entity } = useAsyncEntity(); const apis = useApiHolder(); + const results = useElementFilter( props.children, collection => @@ -76,11 +77,6 @@ export const EntitySwitch = (props: EntitySwitchProps) => { }) .getElements() .flatMap((element: ReactElement) => { - // Nothing is rendered while loading - if (loading) { - return []; - } - const { if: condition, children: elementsChildren } = element.props as EntitySwitchCase; @@ -100,7 +96,7 @@ export const EntitySwitch = (props: EntitySwitchProps) => { }, ]; }), - [apis, entity, loading], + [apis, entity], ); const hasAsyncCases = results.some( From 136cea792bd4ea7404a0cbfbadc3655c4284fdf2 Mon Sep 17 00:00:00 2001 From: Vincenzo Scamporlino Date: Wed, 2 Aug 2023 23:19:15 +0200 Subject: [PATCH 2/5] changeset Signed-off-by: Vincenzo Scamporlino --- .changeset/twelve-ducks-swim.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/twelve-ducks-swim.md diff --git a/.changeset/twelve-ducks-swim.md b/.changeset/twelve-ducks-swim.md new file mode 100644 index 0000000000..1e44f09be1 --- /dev/null +++ b/.changeset/twelve-ducks-swim.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-catalog': patch +--- + +Fixed an issue causing `EntitySwitch` to unmount its children once entity refresh was invoked From 047e0906364366606bd626a8010e680da6d04d3d Mon Sep 17 00:00:00 2001 From: Vincenzo Scamporlino Date: Wed, 2 Aug 2023 23:42:59 +0200 Subject: [PATCH 3/5] I think unmount should be valid Signed-off-by: Vincenzo Scamporlino --- .github/vale/Vocab/Backstage/accept.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/vale/Vocab/Backstage/accept.txt b/.github/vale/Vocab/Backstage/accept.txt index ed09c73c91..f85139ccbf 100644 --- a/.github/vale/Vocab/Backstage/accept.txt +++ b/.github/vale/Vocab/Backstage/accept.txt @@ -405,6 +405,7 @@ unbreak Unconference unicode unmanaged +unmount unregister unregistering unregistration From 959a6bc83312fd6734797122a4d5b52664645d89 Mon Sep 17 00:00:00 2001 From: Vincenzo Scamporlino Date: Tue, 8 Aug 2023 17:03:57 +0200 Subject: [PATCH 4/5] EntitySwitch: do not render anything in case no entity is present Signed-off-by: Vincenzo Scamporlino --- .../src/components/EntitySwitch/EntitySwitch.tsx | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/plugins/catalog/src/components/EntitySwitch/EntitySwitch.tsx b/plugins/catalog/src/components/EntitySwitch/EntitySwitch.tsx index 90de358ef9..b12495052e 100644 --- a/plugins/catalog/src/components/EntitySwitch/EntitySwitch.tsx +++ b/plugins/catalog/src/components/EntitySwitch/EntitySwitch.tsx @@ -77,18 +77,14 @@ export const EntitySwitch = (props: EntitySwitchProps) => { }) .getElements() .flatMap((element: ReactElement) => { + // If the entity is missing or there is an error, render nothing + if (!entity) { + return []; + } + const { if: condition, children: elementsChildren } = element.props as EntitySwitchCase; - // If the entity is missing or there is an error, render the default page - if (!entity) { - return [ - { - if: condition === undefined, - children: elementsChildren, - }, - ]; - } return [ { if: condition?.(entity, { apis }), From 5ad126667787a1ffd8165deedfbc7f58b2d7125a Mon Sep 17 00:00:00 2001 From: Vincenzo Scamporlino Date: Tue, 8 Aug 2023 17:05:29 +0200 Subject: [PATCH 5/5] EntitySwitch: add missing tests and small improvements Signed-off-by: Vincenzo Scamporlino --- .../EntitySwitch/EntitySwitch.test.tsx | 86 +++++++++++++++++-- 1 file changed, 81 insertions(+), 5 deletions(-) diff --git a/plugins/catalog/src/components/EntitySwitch/EntitySwitch.test.tsx b/plugins/catalog/src/components/EntitySwitch/EntitySwitch.test.tsx index 3a8795ccb7..c93922e938 100644 --- a/plugins/catalog/src/components/EntitySwitch/EntitySwitch.test.tsx +++ b/plugins/catalog/src/components/EntitySwitch/EntitySwitch.test.tsx @@ -19,7 +19,7 @@ import { AsyncEntityProvider, EntityProvider, } from '@backstage/plugin-catalog-react'; -import { render, screen } from '@testing-library/react'; +import { render, screen, waitFor } from '@testing-library/react'; import React, { useEffect } from 'react'; import { isKind } from './conditions'; import { EntitySwitch } from './EntitySwitch'; @@ -35,6 +35,80 @@ const Wrapper = ({ children }: { children?: React.ReactNode }) => ( ); describe('EntitySwitch', () => { + it('should render only the first match', () => { + const content = ( + + + + + + ); + + render( + + + {content} + + , + ); + + expect(screen.getByText('A')).toBeInTheDocument(); + expect(screen.queryByText('B')).not.toBeInTheDocument(); + expect(screen.queryByText('C')).not.toBeInTheDocument(); + }); + + it('should render the fallback if no cases are matching', () => { + const content = ( + + + + + + ); + + render( + + + {content} + + , + ); + + expect(screen.queryByText('A')).not.toBeInTheDocument(); + expect(screen.queryByText('B')).not.toBeInTheDocument(); + expect(screen.getByText('C')).toBeInTheDocument(); + }); + + it('should render only the first fallback in case no cases are matching', () => { + const content = ( + + + + + + + ); + + render( + + + {content} + + , + ); + + expect(screen.queryByText('A')).not.toBeInTheDocument(); + expect(screen.queryByText('B')).not.toBeInTheDocument(); + expect(screen.getByText('C')).toBeInTheDocument(); + expect(screen.queryByText('D')).not.toBeInTheDocument(); + }); + it('should switch child when entity switches', () => { const content = ( @@ -96,7 +170,7 @@ describe('EntitySwitch', () => { expect(screen.queryByText('A')).not.toBeInTheDocument(); expect(screen.queryByText('B')).not.toBeInTheDocument(); - expect(screen.getByText('C')).toBeInTheDocument(); + expect(screen.queryByText('C')).not.toBeInTheDocument(); }); it('should switch child when filters switch', () => { @@ -383,10 +457,10 @@ describe('EntitySwitch', () => { expect(screen.queryByText('A')).not.toBeInTheDocument(); }); - it('should switch with sync condition that throws', async () => { + it('should switch with async condition that throws', async () => { const entity = { metadata: { name: 'mock' }, kind: 'component' } as Entity; - const shouldRender = () => Promise.reject(); + const shouldRender = jest.fn().mockRejectedValue(undefined); render( @@ -399,7 +473,9 @@ describe('EntitySwitch', () => { , ); - await expect(screen.findByText('C')).resolves.toBeInTheDocument(); + await waitFor(() => expect(shouldRender).toHaveBeenCalled()); + + expect(screen.getByText('C')).toBeInTheDocument(); expect(screen.queryByText('A')).not.toBeInTheDocument(); expect(screen.queryByText('B')).not.toBeInTheDocument(); });