From 7271ed37620633f3bf82646bf65bbc82d8bc1718 Mon Sep 17 00:00:00 2001 From: Vincenzo Scamporlino Date: Tue, 16 Jan 2024 13:13:02 +0100 Subject: [PATCH 1/6] core-compact-api: convertLegacyApp throws in case of invalid elements are found Signed-off-by: Vincenzo Scamporlino --- .../src/collectLegacyRoutes.test.tsx | 54 +++++++++++++++++++ .../src/collectLegacyRoutes.tsx | 10 ++-- 2 files changed, 61 insertions(+), 3 deletions(-) diff --git a/packages/core-compat-api/src/collectLegacyRoutes.test.tsx b/packages/core-compat-api/src/collectLegacyRoutes.test.tsx index 240aa33e2c..b99d581983 100644 --- a/packages/core-compat-api/src/collectLegacyRoutes.test.tsx +++ b/packages/core-compat-api/src/collectLegacyRoutes.test.tsx @@ -279,4 +279,58 @@ describe('collectLegacyRoutes', () => { screen.findByText('plugins: test'), ).resolves.toBeInTheDocument(); }); + + it('should throw if invalid Route has been detected', async () => { + const plugin = createPlugin({ + id: 'test', + }); + const routeRef = createRouteRef({ id: 'test' }); + const Page = plugin.provide( + createRoutableExtension({ + name: 'Test', + mountPoint: routeRef, + component: () => + Promise.resolve(() => { + const app = useApp(); + return
plugins: {app.getPlugins().map(p => p.getId())}
; + }), + }), + ); + + expect(() => + collectLegacyRoutes( + + } /> + } /> +
+ , + ), + ).toThrow(/Invalid has no path', async () => { + const plugin = createPlugin({ + id: 'test', + }); + const routeRef = createRouteRef({ id: 'test' }); + const Page = plugin.provide( + createRoutableExtension({ + name: 'Test', + mountPoint: routeRef, + component: () => + Promise.resolve(() => { + const app = useApp(); + return
plugins: {app.getPlugins().map(p => p.getId())}
; + }), + }), + ); + + expect(() => + collectLegacyRoutes( + + } /> + , + ), + ).toThrow(/ element with invalid path/); + }); }); diff --git a/packages/core-compat-api/src/collectLegacyRoutes.tsx b/packages/core-compat-api/src/collectLegacyRoutes.tsx index 4d560b81d5..847c0805cb 100644 --- a/packages/core-compat-api/src/collectLegacyRoutes.tsx +++ b/packages/core-compat-api/src/collectLegacyRoutes.tsx @@ -173,9 +173,8 @@ export function collectLegacyRoutes( (route: ReactNode) => { // TODO(freben): Handle feature flag and permissions framework wrapper elements if (!React.isValidElement(route) || route.type !== Route) { - return; + throw new Error('Invalid element has been detected.'); } - const routeElement = route.props.element; const path: string | undefined = route.props.path; const plugin = getComponentData( @@ -186,7 +185,12 @@ export function collectLegacyRoutes( routeElement, 'core.mountPoint', ); - if (!plugin || !path) { + if (path === undefined) { + throw new Error( + ` element with invalid path has been detected. Please make sure to pass the path's props`, + ); + } + if (!plugin) { return; } From 1184990aad38983137be735d874d86460f87e2f1 Mon Sep 17 00:00:00 2001 From: Vincenzo Scamporlino Date: Tue, 16 Jan 2024 13:14:27 +0100 Subject: [PATCH 2/6] core-compact-api: add collectLegacyRoutes changeset Signed-off-by: Vincenzo Scamporlino --- .changeset/wet-emus-work.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/wet-emus-work.md diff --git a/.changeset/wet-emus-work.md b/.changeset/wet-emus-work.md new file mode 100644 index 0000000000..413670a325 --- /dev/null +++ b/.changeset/wet-emus-work.md @@ -0,0 +1,5 @@ +--- +'@backstage/core-compat-api': patch +--- + +collectLegacyRoutes throws in case invalid element is found From 4212a53acd84b637ccb8e08cabf540cdac6a3f0b Mon Sep 17 00:00:00 2001 From: Vincenzo Scamporlino Date: Mon, 22 Jan 2024 08:01:04 +0100 Subject: [PATCH 3/6] Apply suggestions from code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Fredrik Adelöw Signed-off-by: Vincenzo Scamporlino --- packages/core-compat-api/src/collectLegacyRoutes.test.tsx | 3 +-- packages/core-compat-api/src/collectLegacyRoutes.tsx | 4 ++-- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/packages/core-compat-api/src/collectLegacyRoutes.test.tsx b/packages/core-compat-api/src/collectLegacyRoutes.test.tsx index b99d581983..c8d3822ae7 100644 --- a/packages/core-compat-api/src/collectLegacyRoutes.test.tsx +++ b/packages/core-compat-api/src/collectLegacyRoutes.test.tsx @@ -289,8 +289,7 @@ describe('collectLegacyRoutes', () => { createRoutableExtension({ name: 'Test', mountPoint: routeRef, - component: () => - Promise.resolve(() => { + component: async () => () => { const app = useApp(); return
plugins: {app.getPlugins().map(p => p.getId())}
; }), diff --git a/packages/core-compat-api/src/collectLegacyRoutes.tsx b/packages/core-compat-api/src/collectLegacyRoutes.tsx index 847c0805cb..1217f9531d 100644 --- a/packages/core-compat-api/src/collectLegacyRoutes.tsx +++ b/packages/core-compat-api/src/collectLegacyRoutes.tsx @@ -173,7 +173,7 @@ export function collectLegacyRoutes( (route: ReactNode) => { // TODO(freben): Handle feature flag and permissions framework wrapper elements if (!React.isValidElement(route) || route.type !== Route) { - throw new Error('Invalid element has been detected.'); + throw new Error(`Invalid element inside FlatRoutes, expected Route but found ${route.type}.`); } const routeElement = route.props.element; const path: string | undefined = route.props.path; @@ -187,7 +187,7 @@ export function collectLegacyRoutes( ); if (path === undefined) { throw new Error( - ` element with invalid path has been detected. Please make sure to pass the path's props`, + `Route element inside FlatRoutes had no path prop value given`, ); } if (!plugin) { From 32b4a2eebf2137373f911388dbfb48755ddcbd03 Mon Sep 17 00:00:00 2001 From: Vincenzo Scamporlino Date: Mon, 22 Jan 2024 08:15:01 +0100 Subject: [PATCH 4/6] core-compat-api: throw error if element cannot be converted Signed-off-by: Vincenzo Scamporlino --- .../src/collectLegacyRoutes.tsx | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/packages/core-compat-api/src/collectLegacyRoutes.tsx b/packages/core-compat-api/src/collectLegacyRoutes.tsx index 1217f9531d..f164b400bd 100644 --- a/packages/core-compat-api/src/collectLegacyRoutes.tsx +++ b/packages/core-compat-api/src/collectLegacyRoutes.tsx @@ -172,8 +172,15 @@ export function collectLegacyRoutes( flatRoutesElement.props.children, (route: ReactNode) => { // TODO(freben): Handle feature flag and permissions framework wrapper elements - if (!React.isValidElement(route) || route.type !== Route) { - throw new Error(`Invalid element inside FlatRoutes, expected Route but found ${route.type}.`); + if (!React.isValidElement(route)) { + throw new Error( + `Invalid element inside FlatRoutes, expected Route but found element of type ${typeof route}.`, + ); + } + if (route.type !== Route) { + throw new Error( + `Invalid element inside FlatRoutes, expected Route but found ${route.type}.`, + ); } const routeElement = route.props.element; const path: string | undefined = route.props.path; @@ -185,14 +192,16 @@ export function collectLegacyRoutes( routeElement, 'core.mountPoint', ); + if (!plugin) { + throw new Error( + `Route with path ${path} has en element that can not be converted as it does not belong to a plugin. Make sure that the top-level React element of the element prop is an extension from a Backstage plugin, or remove the Route completely. See for more info`, + ); + } if (path === undefined) { throw new Error( `Route element inside FlatRoutes had no path prop value given`, ); } - if (!plugin) { - return; - } const extensions = getPluginExtensions(plugin); const pageExtensionName = extensions.length ? getUniqueName() : undefined; From 78fabde36213d6ef5691dbfc3b6bd73898daa0f1 Mon Sep 17 00:00:00 2001 From: Vincenzo Scamporlino Date: Mon, 22 Jan 2024 10:05:16 +0100 Subject: [PATCH 5/6] core-compat-api: add link todo Signed-off-by: Vincenzo Scamporlino --- packages/core-compat-api/src/collectLegacyRoutes.tsx | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/core-compat-api/src/collectLegacyRoutes.tsx b/packages/core-compat-api/src/collectLegacyRoutes.tsx index f164b400bd..53fe477769 100644 --- a/packages/core-compat-api/src/collectLegacyRoutes.tsx +++ b/packages/core-compat-api/src/collectLegacyRoutes.tsx @@ -194,7 +194,8 @@ export function collectLegacyRoutes( ); if (!plugin) { throw new Error( - `Route with path ${path} has en element that can not be converted as it does not belong to a plugin. Make sure that the top-level React element of the element prop is an extension from a Backstage plugin, or remove the Route completely. See for more info`, + // TODO(vinzscam): add See for more info + `Route with path ${path} has en element that can not be converted as it does not belong to a plugin. Make sure that the top-level React element of the element prop is an extension from a Backstage plugin, or remove the Route completely.`, ); } if (path === undefined) { From ad0835c05167d8fe12ec28926da01c18b6373d2d Mon Sep 17 00:00:00 2001 From: Vincenzo Scamporlino Date: Mon, 22 Jan 2024 10:05:49 +0100 Subject: [PATCH 6/6] core-compat-api: add tests Signed-off-by: Vincenzo Scamporlino --- .../src/collectLegacyRoutes.test.tsx | 53 ++++++++++++++++--- 1 file changed, 47 insertions(+), 6 deletions(-) diff --git a/packages/core-compat-api/src/collectLegacyRoutes.test.tsx b/packages/core-compat-api/src/collectLegacyRoutes.test.tsx index c8d3822ae7..2c8288b878 100644 --- a/packages/core-compat-api/src/collectLegacyRoutes.test.tsx +++ b/packages/core-compat-api/src/collectLegacyRoutes.test.tsx @@ -27,7 +27,7 @@ import { PuppetDbPage } from '@backstage/plugin-puppetdb'; import { StackstormPage } from '@backstage/plugin-stackstorm'; import { ScoreBoardPage } from '@oriflame/backstage-plugin-score-card'; import React, { Fragment } from 'react'; -import { Route, Routes } from 'react-router-dom'; +import { Navigate, Route, Routes } from 'react-router-dom'; import { collectLegacyRoutes } from './collectLegacyRoutes'; // eslint-disable-next-line @backstage/no-relative-monorepo-imports @@ -290,9 +290,9 @@ describe('collectLegacyRoutes', () => { name: 'Test', mountPoint: routeRef, component: async () => () => { - const app = useApp(); - return
plugins: {app.getPlugins().map(p => p.getId())}
; - }), + const app = useApp(); + return
plugins: {app.getPlugins().map(p => p.getId())}
; + }, }), ); @@ -304,7 +304,36 @@ describe('collectLegacyRoutes', () => {
, ), - ).toThrow(/Invalid { + const plugin = createPlugin({ + id: 'test', + }); + const routeRef = createRouteRef({ id: 'test' }); + const Page = plugin.provide( + createRoutableExtension({ + name: 'Test', + mountPoint: routeRef, + component: async () => () => { + const app = useApp(); + return
plugins: {app.getPlugins().map(p => p.getId())}
; + }, + }), + ); + + expect(() => + collectLegacyRoutes( + + } />a string + , + ), + ).toThrow( + /Invalid element inside FlatRoutes, expected Route but found element of type string./, + ); }); it('should throw if has no path', async () => { @@ -330,6 +359,18 @@ describe('collectLegacyRoutes', () => { } /> , ), - ).toThrow(/ element with invalid path/); + ).toThrow(/Route element inside FlatRoutes had no path prop value given/); + }); + + it('should throw if element cannot be converted', async () => { + expect(() => + collectLegacyRoutes( + + } /> + , + ), + ).toThrow( + /Route with path undefined has en element that can not be converted/, + ); }); });