Prepare for removing the private prop

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<AuthenticatedRouteProps> = (
-  props
-) => {
-  const {
-    private: isPrivate,
-    unauthenticated,
-    roles,
-    whileLoadingAuth,
-    children,
-  } = props
+
+export const AuthenticatedRoute: React.FC<AuthenticatedRouteProps> = ({
+  unauthenticated,
+  roles,
+  whileLoadingAuth,
+  children,
+}) => {
   const routerState = useRouterState()
   const {
     loading: authLoading,
@@ -34,14 +30,7 @@ export const AuthenticatedRoute: React.FC<AuthenticatedRouteProps> = (
   }, [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 {

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,
           }),
         ]),
       },

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<RouterProps> = ({
     whileLoadingPage,
     wrappers = [],
     setProps,
+    isPrivate,
     setId,
   } = pathRouteMap[activeRoutePath]
 
@@ -191,6 +192,7 @@ const LocationAwareRouter: React.FC<RouterProps> = ({
                 />
               }
               setProps={setProps}
+              isPrivate={isPrivate}
             />
           )}
         </PageLoadingContextProvider>
@@ -202,70 +204,75 @@ const LocationAwareRouter: React.FC<RouterProps> = ({
 interface WrappedPageProps {
   wrappers: Wrappers
   routeLoaderElement: ReactNode
-  setProps: Record<any, any>[]
+  setProps: Record<string, any>[]
+  isPrivate?: boolean
 }
 
 /**
  * This is effectively a Set (without auth-related code)
  *
- * This means that the <Set> and <Private> components become "virtual"
+ * This means that the <Set> and <PrivateSet> components become "virtual"
  * i.e. they are never actually Rendered, but their props are extracted by the
  * analyze routes function.
  *
  * This is so that we can have all the information up front in the routes-manifest
  * 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 (
-              <AuthenticatedRoute {...propsFromSet}>
-                {children}
-              </AuthenticatedRoute>
-            )
-          }
+    // @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 (
+            <AuthenticatedRoute
+              {...propsFromSet}
+              unauthenticated={propsFromSet.unauthenticated}
+            >
+              {children}
+            </AuthenticatedRoute>
+          )
         }
-      })
+
+        wrappersWithAuthMaybe = [AuthComponent, ...wrappersWithAuthMaybe]
+      }
+    })
 
     if (wrappersWithAuthMaybe.length > 0) {
       // If wrappers exist e.g. [a,b,c] -> <a><b><c><routeLoader></c></b></a> 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<ReactNode | undefined>(
+        (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

packages/router/src/util.ts

@@ -466,6 +466,7 @@ interface AnalyzedRoute {
   wrappers: Wrappers
   setProps: Record<any, any>[]
   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<string, unknown>[]
     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,
               },
             ],
           })