Router: Review comment fixes

packages/router/src/private-context.tsx

@@ -2,9 +2,17 @@ import React, { createContext, useCallback, useContext } from 'react'
 
 import { useRouterState } from './router-context'
 
-export interface PrivateState {
+/**
+ * @param isPrivate - Always true for any children wrapped in a `<Private>`
+ *                    component
+ * @param unauthorized - Use this function to check if the user is allowed to
+ *                       go to this route or not
+ * @param unauthorized - Name of the route to go to if not authorized to visit
+ *                       any of the routes in the containing `<Private>` block
+ */
+interface PrivateState {
   isPrivate: boolean
-  allowRender: (role?: string | string[]) => boolean
+  unauthorized: (role?: string | string[]) => boolean
   unauthenticated: string
 }
 
@@ -15,6 +23,7 @@ interface ProviderProps {
   role?: string | string[]
   unauthenticated: string
 }
+
 export const PrivateContextProvider: React.FC<ProviderProps> = ({
   children,
   isPrivate,
@@ -24,13 +33,13 @@ export const PrivateContextProvider: React.FC<ProviderProps> = ({
   const routerState = useRouterState()
   const { isAuthenticated, hasRole } = routerState.useAuth()
 
-  const allowRender = useCallback(() => {
-    return isAuthenticated && (!role || hasRole(role))
+  const unauthorized = useCallback(() => {
+    return !(isAuthenticated && (!role || hasRole(role)))
   }, [isAuthenticated, role, hasRole])
 
   return (
     <PrivateContext.Provider
-      value={{ isPrivate, allowRender, unauthenticated }}
+      value={{ isPrivate, unauthorized, unauthenticated }}
     >
       {children}
     </PrivateContext.Provider>
@@ -39,8 +48,12 @@ export const PrivateContextProvider: React.FC<ProviderProps> = ({
 
 export const usePrivate = () => {
   const context = useContext(PrivateContext)
-  const allowRender = context ? context.allowRender : () => false
+  const unauthorized = context ? context.unauthorized : () => true
   const unauthenticated = context ? context.unauthenticated : ''
 
-  return { isPrivate: !!context?.isPrivate, allowRender, unauthenticated }
+  return {
+    isPrivate: !!context?.isPrivate,
+    unauthorized,
+    unauthenticated
+  }
 }

packages/router/src/router.tsx

@@ -1,6 +1,6 @@
 // The guts of the router implementation.
 
-import React, { ReactChild } from 'react'
+import React from 'react'
 
 import {
   parseSearch,
@@ -49,7 +49,7 @@ interface RouteProps {
   notfound?: boolean
   redirect?: string
   prerender?: boolean
-  whileLoading?: () => ReactChild | null
+  whileLoading?: () => React.ReactElement | null
 }
 
 const Route: React.VFC<RouteProps> = ({
@@ -58,11 +58,11 @@ const Route: React.VFC<RouteProps> = ({
   name,
   redirect,
   notfound,
-  whileLoading,
+  whileLoading = () => null,
 }) => {
   const location = useLocation()
   const routerState = useRouterState()
-  const { isPrivate, allowRender, unauthenticated } = usePrivate()
+  const { isPrivate, unauthorized, unauthenticated } = usePrivate()
 
   if (notfound) {
     // The "notfound" route is handled by <NotFoundChecker>
@@ -90,10 +90,10 @@ const Route: React.VFC<RouteProps> = ({
     const { loading } = routerState.useAuth()
 
     if (loading) {
-      return <>{whileLoading?.() || null}</>
+      return whileLoading()
     }
 
-    if (!allowRender()) {
+    if (unauthorized()) {
       const currentLocation =
         global.location.pathname + encodeURIComponent(global.location.search)
 
@@ -114,7 +114,9 @@ const Route: React.VFC<RouteProps> = ({
   }
 
   if (!page || !name) {
-    return null
+    throw new Error(
+      "A route that's not a redirect or notfound route needs to specify both a `page` and a `name`"
+    )
   }
 
   return (
@@ -217,7 +219,10 @@ const NotFoundChecker: React.FC<{ children: React.ReactNode }> = ({
   let NotFoundPage: PageType | undefined = undefined
   const flatChildArray = flattenAll(children)
 
-  flatChildArray.forEach((child) => {
+  let i = 0;
+  while (i < flatChildArray.length && !foundMatchingRoute) {
+    const child = flatChildArray[i]
+
     if (isRoute(child)) {
       const { path } = child.props
 
@@ -237,7 +242,9 @@ const NotFoundChecker: React.FC<{ children: React.ReactNode }> = ({
         NotFoundPage = child.props.page
       }
     }
-  })
+
+    i++
+  }
 
   if (!foundMatchingRoute && NotFoundPage) {
     return (