From 937b09d135e76431c15f5ec6f1d418c4d9433b8c Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Wed, 13 Jul 2022 14:02:21 +0200 Subject: [PATCH] core-app-api: completed rr stable tests + improved v2 error messaging Co-authored-by: blam Signed-off-by: Patrik Oldsberg --- .../src/routing/collectors.stable.test.tsx | 225 ++++++++++++------ .../core-app-api/src/routing/collectors.tsx | 31 ++- 2 files changed, 176 insertions(+), 80 deletions(-) diff --git a/packages/core-app-api/src/routing/collectors.stable.test.tsx b/packages/core-app-api/src/routing/collectors.stable.test.tsx index b37faae54c..6c2767621f 100644 --- a/packages/core-app-api/src/routing/collectors.stable.test.tsx +++ b/packages/core-app-api/src/routing/collectors.stable.test.tsx @@ -14,10 +14,14 @@ * limitations under the License. */ -import React, { PropsWithChildren } from 'react'; +import React, { ComponentType, PropsWithChildren } from 'react'; import { routingV2Collector } from './collectors'; -import { traverseElementTree, childDiscoverer } from '../extensions/traversal'; +import { + traverseElementTree, + childDiscoverer, + routeElementDiscoverer, +} from '../extensions/traversal'; import { createRoutableExtension, createRouteRef, @@ -134,9 +138,13 @@ describe('discovery', () => { const root = ( + } /> }>
- }> + {[, 'a string']}} + >
Some text here shouldn't be a problem @@ -161,7 +169,7 @@ describe('discovery', () => { const { routing } = traverseElementTree({ root, - discoverers: [childDiscoverer], + discoverers: [childDiscoverer, routeElementDiscoverer], collectors: { routing: routingV2Collector, }, @@ -205,19 +213,14 @@ describe('discovery', () => { const root = ( - - - - - - } - /> - }> - } /> - + }> + + } /> + + + }> + } /> + } /> @@ -225,17 +228,17 @@ describe('discovery', () => { const { routing } = traverseElementTree({ root, - discoverers: [childDiscoverer], + discoverers: [childDiscoverer, routeElementDiscoverer], collectors: { routing: routingV2Collector, }, }); expect(sortedEntries(routing.paths)).toEqual([ - [ref1, '/foo'], - [ref2, '/bar/:id'], - [ref3, '/baz'], - [ref4, '/divsoup'], - [ref5, '/blop'], + [ref1, 'foo'], + [ref2, 'bar/:id'], + [ref3, 'baz'], + [ref4, 'divsoup'], + [ref5, 'blop'], ]); expect(sortedEntries(routing.parents)).toEqual([ [ref1, undefined], @@ -250,37 +253,37 @@ describe('discovery', () => { const root = ( - +
HELLO - - + }> + - + ); const { routing } = traverseElementTree({ root, - discoverers: [childDiscoverer], + discoverers: [childDiscoverer, routeElementDiscoverer], collectors: { routing: routingV2Collector, }, }); expect(sortedEntries(routing.paths)).toEqual([ - [ref1, '/foo'], - [ref2, '/foo'], - [ref3, '/bar'], - [ref4, '/baz'], - [ref5, '/baz'], + [ref1, 'foo'], + [ref2, 'foo'], + [ref3, 'bar'], + [ref4, 'baz'], + [ref5, 'baz'], ]); expect(sortedEntries(routing.parents)).toEqual([ [ref1, undefined], @@ -290,11 +293,13 @@ describe('discovery', () => { [ref5, ref3], ]); expect(routing.objects).toEqual([ - routeObj('/foo', [ref1, ref2], [], 'gathered'), + routeObj('foo', [ref1, ref2], [], 'gathered'), routeObj( - '/bar', + 'bar', [ref3], - [routeObj('/baz', [ref4, ref5], [], 'gathered')], + [routeObj('baz', [ref4, ref5], [], 'gathered')], + undefined, + plugin, ), ]); }); @@ -303,80 +308,160 @@ describe('discovery', () => { const root = ( - - + }> + - - - + + }> + } /> + + - + ); const { routing } = traverseElementTree({ root, - discoverers: [childDiscoverer], + discoverers: [childDiscoverer, routeElementDiscoverer], collectors: { routing: routingV2Collector, }, }); expect(sortedEntries(routing.paths)).toEqual([ - [ref1, '/foo'], - [ref2, '/bar'], - [ref3, '/baz'], - [ref4, '/blop'], - [ref5, '/bar'], + [ref1, 'foo'], + [ref2, 'bar'], + [ref3, 'baz'], + [ref4, 'blop'], + [ref5, 'bar'], ]); expect(sortedEntries(routing.parents)).toEqual([ [ref1, undefined], [ref2, ref1], - [ref3, ref1], + [ref3, ref2], [ref4, ref3], [ref5, ref1], ]); expect(routing.objects).toEqual([ routeObj( - '/foo', + 'foo', [ref1], [ routeObj( - '/bar', + 'bar', [ref2, ref5], - [routeObj('/baz', [ref3], [routeObj('/blop', [ref4])])], + [ + routeObj( + 'baz', + [ref3], + [routeObj('blop', [ref4], undefined, undefined, plugin)], + undefined, + plugin, + ), + ], 'gathered', ), ], + undefined, + plugin, ), ]); }); - it('should stop gathering mount points after encountering explicit path', () => { - const root = ( - - - - - - - - - - - - ); - + it('should throw when you provide path property on an extension', () => { expect(() => { traverseElementTree({ - root, - discoverers: [childDiscoverer], + root: , + discoverers: [childDiscoverer, routeElementDiscoverer], collectors: { routing: routingV2Collector, }, }); - }).toThrow('Mounted routable extension must have a path'); + }).toThrow( + 'Path property may not be set directly on a routable extension "Extension(Extension1)"', + ); + }); + + it('should throw when element prop is not a string', () => { + const Div = 'div' as unknown as ComponentType<{ path: boolean }>; + expect(() => { + traverseElementTree({ + root:
, + discoverers: [childDiscoverer, routeElementDiscoverer], + collectors: { + routing: routingV2Collector, + }, + }); + }).toThrow('Element path must be a string at "div"'); + }); + + it('should throw when the mount point gatherers have an element prop', () => { + const AnyAggregationComponent = AggregationComponent as any; + expect(() => { + traverseElementTree({ + root: } />, + discoverers: [childDiscoverer, routeElementDiscoverer], + collectors: { + routing: routingV2Collector, + }, + }); + }).toThrow( + 'Mount point gatherers may not have an element prop "AggregationComponent"', + ); + }); + + it('should throw elements within element prop contains a path', () => { + expect(() => { + traverseElementTree({ + root: } />, + discoverers: [childDiscoverer, routeElementDiscoverer], + collectors: { + routing: routingV2Collector, + }, + }); + }).toThrow( + 'Elements within the element prop tree may not have paths, found "bar"', + ); + }); + + it('should throw when a routable extension does not have a path set', () => { + expect(() => { + traverseElementTree({ + root: , + discoverers: [childDiscoverer, routeElementDiscoverer], + collectors: { + routing: routingV2Collector, + }, + }); + }).toThrow( + 'Routable extension "Extension(Extension3)" with mount point "routeRef{type=absolute,id=ref3}" must be assigned a path', + ); + }); + + it('should throw when Route elements contain multiple routable extensions', () => { + expect(() => { + traverseElementTree({ + root: ( + + + + + } + /> + ), + discoverers: [childDiscoverer, routeElementDiscoverer], + collectors: { + routing: routingV2Collector, + }, + }); + }).toThrow( + 'Route element with path "foo" may not contain multiple routable extensions', + ); }); }); diff --git a/packages/core-app-api/src/routing/collectors.tsx b/packages/core-app-api/src/routing/collectors.tsx index 95fa3694f1..81a263eb70 100644 --- a/packages/core-app-api/src/routing/collectors.tsx +++ b/packages/core-app-api/src/routing/collectors.tsx @@ -36,10 +36,13 @@ export const MATCH_ALL_ROUTE: BackstageRouteObject = { }; function stringifyNode(node: ReactNode): string { - if (!isValidElement(node)) { - return String(node); + const anyNode = node as { type?: { displayName?: string; name?: string } }; + if (anyNode?.type) { + return ( + anyNode.type.displayName ?? anyNode.type.name ?? String(anyNode.type) + ); } - return (node.type as { displayName?: string })?.displayName ?? String(node); + return String(anyNode); } interface RoutingV2CollectorContext { @@ -61,7 +64,7 @@ function collectSubTree( if (element.props.path) { throw new Error( - 'Elements within the element prop tree may not contain paths', + `Elements within the element prop tree may not have paths, found "${element.props.path}"`, ); } @@ -89,7 +92,9 @@ export const routingV2Collector = createCollector( const mountPoint = getComponentData(node, 'core.mountPoint'); if (mountPoint && path) { throw new Error( - 'Path property may not be set directly on a routable extension', + `Path property may not be set directly on a routable extension "${stringifyNode( + node, + )}"`, ); } @@ -106,14 +111,20 @@ export const routingV2Collector = createCollector( if (path) { if (typeof path !== 'string') { - throw new Error('Element path must be a string'); + throw new Error( + `Element path must be a string at "${stringifyNode(node)}"`, + ); } const elementProp = node.props.element; if (getComponentData(node, 'core.gatherMountPoints')) { if (elementProp) { - throw new Error('Mount point gatherers may not have an element prop'); + throw new Error( + `Mount point gatherers may not have an element prop "${stringifyNode( + node, + )}"`, + ); } const newObj = { @@ -138,7 +149,7 @@ export const routingV2Collector = createCollector( const [extension, ...others] = collectSubTree(elementProp); if (others.length > 0) { throw new Error( - 'Route element may not contain multiple routable extensions', + `Route element with path "${path}" may not contain multiple routable extensions`, ); } if (!extension) { @@ -170,9 +181,9 @@ export const routingV2Collector = createCollector( if (mountPoint) { if (!ctx?.gatherPath) { throw new Error( - `Routable extension ${stringifyNode( + `Routable extension "${stringifyNode( node, - )} with mount point ${mountPoint} must be assigned a path`, + )}" with mount point "${mountPoint}" must be assigned a path`, ); }