From 724afadbe03861484d017b56b873959cbeb987af Mon Sep 17 00:00:00 2001 From: Daniel Choudhury Date: Tue, 21 Mar 2023 15:21:28 +0700 Subject: [PATCH] Fix wrap and set pass thru props --- packages/router/Todo.todo | 12 +- packages/router/src/Set.tsx | 2 +- .../src/__tests__/analyzeRoutes.test.tsx | 142 +++++++++++++++--- packages/router/src/router.tsx | 59 ++++++-- packages/router/src/util.ts | 51 +++++-- 5 files changed, 225 insertions(+), 41 deletions(-) diff --git a/packages/router/Todo.todo b/packages/router/Todo.todo index 99c7f2777143..3964169ab5b3 100644 --- a/packages/router/Todo.todo +++ b/packages/router/Todo.todo @@ -14,7 +14,17 @@ Restore existing functionality: Props on Sets/Private/etc.: ✔ whileLoading @done(23-03-08 12:10) - ☐ and whileLoadingAuth + ☐ Auth related + Do we even need this? When server rendering, we won't load auth + ☐ error on unauthorized + ☐ whileLoadingAuth component + ☐ redirect on unauthenticated, and error if route not found + ☐ error on missing parameters + ✔ wrap components not rendered @done(23-03-21 15:21) + + +While loading: + ☐ Can we wrap the whileLoadingPage in a component that updates itself with startTransition? Redirects: ☐ Handling the redirect prop in a Route client side diff --git a/packages/router/src/Set.tsx b/packages/router/src/Set.tsx index a82a30633eef..f7ff5ca14673 100644 --- a/packages/router/src/Set.tsx +++ b/packages/router/src/Set.tsx @@ -5,7 +5,7 @@ import { usePageLoadingContext } from './PageLoadingContext' import { routes as namedRoutes } from './router' import { useRouterState } from './router-context' -type WrapperType = ( +export type WrapperType = ( props: WTProps & { children: ReactNode } ) => ReactElement | null diff --git a/packages/router/src/__tests__/analyzeRoutes.test.tsx b/packages/router/src/__tests__/analyzeRoutes.test.tsx index 9650c8754979..bcd86f20e118 100644 --- a/packages/router/src/__tests__/analyzeRoutes.test.tsx +++ b/packages/router/src/__tests__/analyzeRoutes.test.tsx @@ -1,6 +1,7 @@ import React, { isValidElement } from 'react' import { Route, Router } from '../router' +import { Set } from '../Set' import { analyzeRoutes } from '../util' const FakePage = () =>

Fake Page

@@ -21,12 +22,34 @@ describe('AnalyzeRoutes: with homePage and Children', () => { currentPathName: '/', }) test('Should return namePathMap and hasHomeRoute correctly', () => { - expect(namePathMap).toEqual( + expect(Object.keys(namePathMap)).toEqual([ + 'hello', + 'world', + 'recipeById', + 'home', + ]) + + expect(namePathMap.hello).toEqual( + expect.objectContaining({ + name: 'hello', + page: FakePage, + path: '/hello', + }) + ) + + expect(namePathMap.world).toEqual( expect.objectContaining({ - '/world': { name: 'world', path: '/world' }, - '/hello': { name: 'hello', path: '/hello' }, - '/recipe/{id}': { name: 'recipeById', path: '/recipe/{id}' }, - '/': { name: 'home', path: '/' }, + name: 'world', + page: FakePage, + path: '/world', + }) + ) + + expect(namePathMap.recipeById).toEqual( + expect.objectContaining({ + name: 'recipeById', + page: FakePage, + path: '/recipe/{id}', }) ) @@ -55,24 +78,101 @@ describe('AnalyzeRoutes: with homePage and Children', () => { expect(activeRouteName).toBeDefined() expect(activeRouteName).toBe('recipeById') }) -}) -test('No home Route', () => { - const CheckRoutes = ( - - - - - ) + test('No home Route', () => { + const CheckRoutes = ( + + + + + ) - const { namePathMap, namedRoutesMap, hasHomeRoute } = analyzeRoutes( - CheckRoutes.props.children, - { + const { namePathMap, namedRoutesMap, hasHomeRoute } = analyzeRoutes( + CheckRoutes.props.children, + { + currentPathName: '/', + } + ) + + expect(Object.keys(namedRoutesMap).length).toEqual(2) + expect(Object.keys(namePathMap).length).toEqual(2) + expect(hasHomeRoute).toBe(false) + }) + + test('Creates setWrapper map', () => { + const WrapperX = ({ children }) => ( + <> +

WrapperA

+ {children} + + ) + + const WrapperY = ({ children }) => ( + <> +

WrapperY

+ {children} + + ) + + const Simple = ( + + + + + + + + + + ) + + const { namePathMap } = analyzeRoutes(Simple.props.children, { currentPathName: '/', - } - ) + }) - expect(Object.keys(namedRoutesMap).length).toEqual(2) - expect(Object.keys(namePathMap).length).toEqual(2) - expect(hasHomeRoute).toBe(false) + expect(namePathMap.routeA).toEqual( + expect.objectContaining({ + redirect: null, + name: 'routeA', + path: '/a', + whileLoadingPage: undefined, + wrappers: [WrapperX], + // Props passed through from set + setProps: { + id: 'set-one', + passThruProp: 'bazinga', + }, + }) + ) + + expect(namePathMap.routeB).toEqual( + expect.objectContaining({ + redirect: null, + name: 'routeB', + path: '/b', + whileLoadingPage: undefined, + wrappers: [WrapperX, WrapperY], // both wrappers + setProps: { + id: 'set-two', + theme: 'blue', + passThruProp: 'bazinga', // from the first set + }, + }) + ) + + expect(namePathMap.routeC).toEqual( + expect.objectContaining({ + redirect: null, + name: 'routeC', + path: '/c', + whileLoadingPage: undefined, + wrappers: [WrapperX, WrapperY], // both wrappers + setProps: { + id: 'set-two', + theme: 'blue', + passThruProp: 'bazinga', // from the first set + }, + }) + ) + }) }) diff --git a/packages/router/src/router.tsx b/packages/router/src/router.tsx index 39f33c8b1f9b..c370678f3348 100644 --- a/packages/router/src/router.tsx +++ b/packages/router/src/router.tsx @@ -1,4 +1,4 @@ -import React, { ReactNode, ReactElement, useMemo } from 'react' +import React, { ReactNode, ReactElement, useMemo, memo } from 'react' import { ActiveRouteLoader } from './active-route-loader' import { Redirect } from './links' @@ -98,6 +98,8 @@ const LocationAwareRouter: React.FC = ({ () => analyzeRoutes(children, { currentPathName: location.pathname, + // @TODO We haven't handled this with SSR/Streaming yet. + // May need a babel plugin to extract userParamTypes from Routes.tsx userParamTypes: paramTypes, }), [location.pathname, children, paramTypes] @@ -143,8 +145,15 @@ const LocationAwareRouter: React.FC = ({ return null } - const { path, page, name, redirect, whileLoadingPage } = - namePathMap[activeRouteName] + const { + path, + page, + name, + redirect, + whileLoadingPage, + wrappers = [], + setProps, + } = namePathMap[activeRouteName] if (!path) { throw new Error(`Route "${name}" needs to specify a path`) @@ -166,12 +175,19 @@ const LocationAwareRouter: React.FC = ({ {redirect && } {!redirect && page && ( - + } + setProps={setProps} /> )} @@ -179,6 +195,31 @@ const LocationAwareRouter: React.FC = ({ ) } +interface WrappedPageProps { + wrappers: ReactNode[] + routeLoaderElement: ReactNode + setProps: Record +} + +const WrappedPage = memo( + ({ wrappers, routeLoaderElement, setProps }: WrappedPageProps) => { + if (wrappers.length > 0) { + // If wrappers exist e.g. [a,b,c] -> + return wrappers.reduceRight((acc, wrapper) => { + return React.createElement( + wrapper as any, + { + ...setProps, + }, + acc ? acc : routeLoaderElement + ) + }, undefined) as any + } + + return routeLoaderElement + } +) + export { Router, Route, diff --git a/packages/router/src/util.ts b/packages/router/src/util.ts index d00acb5ffd10..6a36cd3ce764 100644 --- a/packages/router/src/util.ts +++ b/packages/router/src/util.ts @@ -449,6 +449,8 @@ interface AnalyzedRoute { whileLoadingPage?: WhileLoadingPage page: PageType | null redirect: string | null + wrappers: ReactNode[] + setProps: Record } export function analyzeRoutes( @@ -462,10 +464,19 @@ export function analyzeRoutes( let NotFoundPage: PageType | undefined let activeRouteName: string | undefined - const recurseThroughRouter = ( - nodes: ReturnType, + interface RecurseParams { + nodes: ReturnType whileLoadingPageFromSet?: WhileLoadingPage - ) => { + wrappersFromSet?: ReactNode[] + propsFromSet?: Record // we don't know, or care about what props users are passing donw + } + + const recurseThroughRouter = ({ + nodes, + whileLoadingPageFromSet, + wrappersFromSet = [], + propsFromSet = {}, + }: RecurseParams) => { nodes.forEach((node) => { if (isValidRoute(node)) { // Just for readability @@ -475,6 +486,8 @@ export function analyzeRoutes( if (isNotFoundRoute(route)) { NotFoundPage = route.props.page // Dont add notFound routes to the maps, and exit early + // @TODO: We may need to add it to the map, because you can in + // theory wrap a notfound page in a Set wrapper return } @@ -493,6 +506,8 @@ export function analyzeRoutes( name, path, page: null, // Redirects don't need pages. We set this to null for consistency + wrappers: wrappersFromSet, + setProps: propsFromSet, } } @@ -518,6 +533,8 @@ export function analyzeRoutes( whileLoadingPage: route.props.whileLoadingPage || whileLoadingPageFromSet, page: page, + wrappers: wrappersFromSet, + setProps: propsFromSet, } // e.g. namedRoutesMap.homePage = () => '/home' @@ -527,20 +544,36 @@ export function analyzeRoutes( // @NOTE: A is also a Set if (isSetNode(node)) { - if (node.props.children) { - recurseThroughRouter( - Children.toArray(node.props.children), + const { + children, + whileLoadingPage: whileLoadingPageFromCurrentSet, + wrap: wrapFromCurrentSet, + ...otherPropsFromCurrentSet + } = node.props + + const wrapperComponentsArray = Array.isArray(wrapFromCurrentSet) + ? wrapFromCurrentSet + : [wrapFromCurrentSet] + + if (children) { + recurseThroughRouter({ + nodes: Children.toArray(children), // When there's a whileLoadingPage prop on a Set, we pass it down to all its children // If the parent node was also a Set with whileLoadingPage, we pass it down. The child's whileLoadingPage // will always take precedence over the parent's - node.props.whileLoadingPage || whileLoadingPageFromSet - ) + whileLoadingPageFromSet: + whileLoadingPageFromCurrentSet || whileLoadingPageFromSet, + wrappersFromSet: [...wrappersFromSet, ...wrapperComponentsArray], + propsFromSet: { ...propsFromSet, ...otherPropsFromCurrentSet }, // Current one takes precedence + // @TODO pass this in + // otherPropsFromSet: otherSetProps, + }) } } }) } - recurseThroughRouter(Children.toArray(children)) + recurseThroughRouter({ nodes: Children.toArray(children) }) return { namePathMap,