diff --git a/packages/router/src/AuthenticatedRoute.tsx b/packages/router/src/AuthenticatedRoute.tsx index a3bdba1726d5..79091ab03260 100644 --- a/packages/router/src/AuthenticatedRoute.tsx +++ b/packages/router/src/AuthenticatedRoute.tsx @@ -8,20 +8,16 @@ import type { GeneratedRoutesMap } from './util' interface AuthenticatedRouteProps { children: React.ReactNode roles?: string | string[] - unauthenticated?: keyof GeneratedRoutesMap + unauthenticated: keyof GeneratedRoutesMap whileLoadingAuth?: () => React.ReactElement | null - private?: boolean } -export const AuthenticatedRoute: React.FC = ( - props -) => { - const { - private: isPrivate, - unauthenticated, - roles, - whileLoadingAuth, - children, - } = props + +export const AuthenticatedRoute: React.FC = ({ + unauthenticated, + roles, + whileLoadingAuth, + children, +}) => { const routerState = useRouterState() const { loading: authLoading, @@ -34,14 +30,7 @@ export const AuthenticatedRoute: React.FC = ( }, [isAuthenticated, roles, hasRole]) // Make sure `wrappers` is always an array with at least one wrapper component - if (isPrivate && unauthorized()) { - if (!unauthenticated) { - throw new Error( - 'Private Sets need to specify what route to redirect unauthorized ' + - 'users to by setting the `unauthenticated` prop' - ) - } - + if (unauthorized()) { if (authLoading) { return whileLoadingAuth?.() || null } else { diff --git a/packages/router/src/__tests__/analyzeRoutes.test.tsx b/packages/router/src/__tests__/analyzeRoutes.test.tsx index ce80136b0975..071837f9b437 100644 --- a/packages/router/src/__tests__/analyzeRoutes.test.tsx +++ b/packages/router/src/__tests__/analyzeRoutes.test.tsx @@ -300,12 +300,8 @@ describe('AnalyzeRoutes: with homePage and Children', () => { page: FakePage, wrappers: [], setId: 1, - setProps: [ - { - private: true, - unauthenticated: 'home', - }, - ], + isPrivate: true, + setProps: [{ unauthenticated: 'home' }], }) }) @@ -331,12 +327,8 @@ describe('AnalyzeRoutes: with homePage and Children', () => { page: FakePage, wrappers: [], setId: 1, - setProps: [ - { - private: true, - unauthenticated: 'home', - }, - ], + isPrivate: true, + setProps: [{ unauthenticated: 'home' }], }) }) @@ -420,11 +412,9 @@ describe('AnalyzeRoutes: with homePage and Children', () => { expect(pathRouteMap).toMatchObject({ '/no-roles-assigned': { redirect: null, + isPrivate: true, setProps: expect.arrayContaining([ - expect.objectContaining({ - unauthenticated: 'home', - private: true, - }), + expect.objectContaining({ unauthenticated: 'home' }), ]), }, }) @@ -435,15 +425,12 @@ describe('AnalyzeRoutes: with homePage and Children', () => { expect(pathRouteMap).toMatchObject({ '/employee': { redirect: null, + isPrivate: true, setProps: expect.arrayContaining([ // Should have the first one, but also.. - expect.objectContaining({ - unauthenticated: 'home', - private: true, - }), + expect.objectContaining({ unauthenticated: 'home' }), // ...the second private set's props expect.objectContaining({ - private: true, unauthenticated: 'noRolesAssigned', roles: ['ADMIN', 'EMPLOYEE'], }), @@ -455,24 +442,19 @@ describe('AnalyzeRoutes: with homePage and Children', () => { expect(pathRouteMap).toMatchObject({ '/admin': { redirect: null, + isPrivate: true, setProps: expect.arrayContaining([ // Should have the first one, but also.. - expect.objectContaining({ - unauthenticated: 'home', - private: true, - }), + expect.objectContaining({ unauthenticated: 'home' }), // ...the second private set's props expect.objectContaining({ - private: true, unauthenticated: 'noRolesAssigned', roles: ['ADMIN', 'EMPLOYEE'], }), - // ...and the third private set's props expect.objectContaining({ unauthenticated: 'employee', roles: 'ADMIN', - private: true, }), ]), }, diff --git a/packages/router/src/router.tsx b/packages/router/src/router.tsx index 4e6c71c7183b..a4272cf28474 100644 --- a/packages/router/src/router.tsx +++ b/packages/router/src/router.tsx @@ -47,7 +47,7 @@ export interface RouteProps { * * Route is now a "virtual" component * it is actually never rendered. All the page loading logic happens in active-route-loader - * and all the validation happens within utlity functions called from the Router + * and all the validation happens within utility functions called from the Router */ function Route(props: RouteProps): JSX.Element function Route(props: RedirectRouteProps): JSX.Element @@ -155,6 +155,7 @@ const LocationAwareRouter: React.FC = ({ whileLoadingPage, wrappers = [], setProps, + isPrivate, setId, } = pathRouteMap[activeRoutePath] @@ -191,6 +192,7 @@ const LocationAwareRouter: React.FC = ({ /> } setProps={setProps} + isPrivate={isPrivate} /> )} @@ -202,13 +204,14 @@ const LocationAwareRouter: React.FC = ({ interface WrappedPageProps { wrappers: Wrappers routeLoaderElement: ReactNode - setProps: Record[] + setProps: Record[] + isPrivate?: boolean } /** * This is effectively a Set (without auth-related code) * - * This means that the and components become "virtual" + * This means that the and components become "virtual" * i.e. they are never actually Rendered, but their props are extracted by the * analyze routes function. * @@ -216,56 +219,60 @@ interface WrappedPageProps { * for SSR, but also so that we only do one loop of all the Routes. */ const WrappedPage = memo( - ({ wrappers, routeLoaderElement, setProps }: WrappedPageProps) => { + ({ wrappers, routeLoaderElement, setProps, isPrivate }: WrappedPageProps) => { // @NOTE: don't mutate the wrappers array, it causes full page re-renders // Instead just create a new array with the AuthenticatedRoute wrapper // we need to pass the setProps from each set to each wrapper let wrappersWithAuthMaybe = wrappers - const reveresedSetProps = [...setProps].reverse() - reveresedSetProps - // @MARK note the reverse() here, because we spread wrappersWithAuthMaybe - .forEach((propsFromSet) => { - if (propsFromSet.private) { - if (!propsFromSet.unauthenticated) { - throw new Error( - 'You must specify an `unauthenticated` route when marking a Route as private' - ) - } - - // @MARK: this component intentionally removes all props except children - // because the .reduce below will apply props inside out - const AuthComponent: React.FC<{ children: ReactNode }> = ({ - children, - }) => { - return ( - - {children} - - ) - } + // @MARK note the reverse() here, because we spread wrappersWithAuthMaybe + ;[...setProps].reverse().forEach((propsFromSet) => { + if (isPrivate) { + if (!propsFromSet.unauthenticated) { + throw new Error( + 'You must specify an `unauthenticated` route when using PrivateSet' + ) + } - wrappersWithAuthMaybe = [AuthComponent, ...wrappersWithAuthMaybe] + // @MARK: this component intentionally removes all props except children + // because the .reduce below will apply props inside out + const AuthComponent: React.FC<{ children: ReactNode }> = ({ + children, + }) => { + return ( + + {children} + + ) } - }) + + wrappersWithAuthMaybe = [AuthComponent, ...wrappersWithAuthMaybe] + } + }) if (wrappersWithAuthMaybe.length > 0) { // If wrappers exist e.g. [a,b,c] -> and returns a single ReactNode // Wrap AuthenticatedRoute this way, because if we mutate the wrappers array itself // it causes full rerenders of the page - return wrappersWithAuthMaybe.reduceRight((acc, wrapper) => { - // Merge props from set, the lowest set props will override the higher ones - const mergedSetProps = setProps.reduce((acc, props) => { - return { ...acc, ...props } - }, {}) + return wrappersWithAuthMaybe.reduceRight( + (acc, wrapper) => { + // Merge props from set, the lowest set props will override the higher ones + const mergedSetProps = setProps.reduce((acc, props) => { + return { ...acc, ...props } + }, {}) - return React.createElement( - wrapper as any, - mergedSetProps, - acc ? acc : routeLoaderElement - ) - }, undefined as ReactNode) + return React.createElement( + wrapper, + mergedSetProps, + acc ? acc : routeLoaderElement + ) + }, + undefined + ) } return routeLoaderElement diff --git a/packages/router/src/util.ts b/packages/router/src/util.ts index a36686cbe6f9..e24c63428a52 100644 --- a/packages/router/src/util.ts +++ b/packages/router/src/util.ts @@ -466,6 +466,7 @@ interface AnalyzedRoute { wrappers: Wrappers setProps: Record[] setId: number + isPrivate: boolean } export function analyzeRoutes( @@ -485,6 +486,7 @@ export function analyzeRoutes( // we don't know, or care about, what props users are passing down propsFromSet?: Record[] setId?: number + isPrivate?: boolean } // Track the number of sets found. @@ -510,6 +512,7 @@ export function analyzeRoutes( whileLoadingPageFromSet, wrappersFromSet = [], propsFromSet: previousSetProps = [], + isPrivate = false, }: RecurseParams) => { nodes.forEach((node) => { if (isValidRoute(node)) { @@ -554,6 +557,7 @@ export function analyzeRoutes( wrappers: wrappersFromSet, setProps: previousSetProps, setId, + isPrivate, } if (name) { @@ -587,6 +591,7 @@ export function analyzeRoutes( wrappers: wrappersFromSet, setProps: previousSetProps, setId, + isPrivate, } // e.g. namedRoutesMap.homePage = () => '/home' @@ -611,15 +616,6 @@ export function analyzeRoutes( : [wrapFromCurrentSet] } - // You cannot make a nested set public if the parent is private - // i.e. the private prop cannot be overridden by a child Set - const privateProps = - isPrivateNode(node) || - isPrivateSetNode(node) || - previousSetProps.some((props) => props.private) - ? { private: true } - : {} - if (children) { recurseThroughRouter({ nodes: Children.toArray(children), @@ -629,14 +625,20 @@ export function analyzeRoutes( whileLoadingPageFromSet: whileLoadingPageFromCurrentSet || whileLoadingPageFromSet, setId, + isPrivate: + isPrivateNode(node) || + isPrivateSetNode(node) || + isPrivate || + // The following two conditions can be removed when we remove + // the deprecated private prop + previousSetProps.some((props) => props.private) || + otherPropsFromCurrentSet.private, wrappersFromSet: [...wrappersFromSet, ...wrapperComponentsArray], propsFromSet: [ ...previousSetProps, { // Current one takes precedence ...otherPropsFromCurrentSet, - // See comment at definition, intentionally at the end - ...privateProps, }, ], })