fix: Strengthen route typings (#9366)

* Add conditional type to RouteObject - RouteObject type should accept either index or children. * Add contributor * Enhance children if index is falsy * Throw error in createRoutesFromChildren + test * Update error message * fix: Stengthen route typings top to bottom * Add test+invariant for index/path combinations * Remove duplicate check * Add invariant for index/path to flattenRoutes * Make children checks consistent * Allow index + path routes * remove data router example changes * Remove createroutesFromChildren test * Fix error message * List type props * add changeset Co-authored-by: Siddhant Gupta <guptasiddhant@icloud.com> Co-authored-by: Siddhant Gupta <siddhant.c.gupta@accenture.com>
remix-run · Sep 30, 2022 · 540f94b · 540f94b
1 parent 434003d
commit 540f94b
Show file tree

Hide file tree

Showing 13 changed files with 196 additions and 60 deletions.
diff --git a/.changeset/swift-beers-sell.md b/.changeset/swift-beers-sell.md
@@ -0,0 +1,8 @@
+---
+"react-router": patch
+"react-router-dom": patch
+"react-router-native": patch
+"@remix-run/router": patch
+---
+
+fix: Strengthen RouteObject/RouteProps types and throw on index routes with children (#9366)
diff --git a/contributors.yml b/contributors.yml
@@ -36,6 +36,7 @@
 - goldins
 - gowthamvbhat
 - GraxMonzo
+- GuptaSiddhant
 - haivuw
 - hernanif1
 - hongji00

diff --git a/packages/react-router-dom/index.tsx b/packages/react-router-dom/index.tsx
@@ -80,6 +80,7 @@ export type {
   DataRouteObject,
   Fetcher,
   Hash,
+  IndexRouteObject,
   IndexRouteProps,
   JsonFunction,
   LayoutRouteProps,
@@ -92,6 +93,7 @@ export type {
   NavigateProps,
   Navigation,
   Navigator,
+  NonIndexRouteObject,
   OutletProps,
   Params,
   ParamParseKey,

diff --git a/packages/react-router-native/index.tsx b/packages/react-router-native/index.tsx
@@ -27,6 +27,7 @@ export type {
   DataRouteObject,
   Fetcher,
   Hash,
+  IndexRouteObject,
   IndexRouteProps,
   JsonFunction,
   LayoutRouteProps,
@@ -39,6 +40,7 @@ export type {
   NavigateProps,
   Navigation,
   Navigator,
+  NonIndexRouteObject,
   OutletProps,
   Params,
   ParamParseKey,

diff --git a/packages/react-router/__tests__/createRoutesFromChildren-test.tsx b/packages/react-router/__tests__/createRoutesFromChildren-test.tsx
@@ -196,4 +196,17 @@ describe("creating routes from JSX", () => {
       ]
     `);
   });
+
+  it("throws when the index route has children", () => {
+    expect(() => {
+      createRoutesFromChildren(
+        <Route path="/">
+          {/* @ts-expect-error */}
+          <Route index>
+            <Route path="users" />
+          </Route>
+        </Route>
+      );
+    }).toThrow("An index route cannot have child routes.");
+  });
 });
diff --git a/packages/react-router/__tests__/index-routes-test.tsx b/packages/react-router/__tests__/index-routes-test.tsx
@@ -8,9 +8,10 @@ describe("index route matching", () => {
           {
             path: "/users",
             children: [
+              // This config is not valid because index routes cannot have children
+              // @ts-expect-error
               {
                 index: true,
-                // This config is not valid because index routes cannot have children
                 children: [{ path: "not-valid" }],
               },
               { path: ":id" },
@@ -19,6 +20,8 @@ describe("index route matching", () => {
         ],
         "/users/mj"
       );
-    }).toThrow("must not have child routes");
+    }).toThrowErrorMatchingInlineSnapshot(
+      `"Index routes must not have child routes. Please remove all child routes from route path \\"/users/\\"."`
+    );
   });
 });
diff --git a/packages/react-router/__tests__/matchRoutes-test.tsx b/packages/react-router/__tests__/matchRoutes-test.tsx
@@ -1,3 +1,4 @@
+import { AgnosticRouteObject } from "@remix-run/router";
 import * as React from "react";
 import type { RouteObject } from "react-router";
 import { matchRoutes } from "react-router";
@@ -43,8 +44,7 @@ describe("matchRoutes", () => {
       { path: "*", element: <h1>Not Found</h1> },
     ],
   };
-
-  let routes = [
+  let routes: RouteObject[] = [
     { path: "/", element: <h1>Root Layout</h1> },
     {
       path: "/home",
@@ -71,7 +71,7 @@ describe("matchRoutes", () => {
   });
 
   it("matches index routes with path over layout", () => {
-    expect(matchRoutes(routes, "/layout")[0].route.index).toBe(true);
+    expect(matchRoutes(routes, "/layout")?.[0].route.index).toBe(true);
     expect(pickPaths(routes, "/layout")).toEqual(["/layout"]);
   });
 

diff --git a/packages/react-router/index.ts b/packages/react-router/index.ts
@@ -64,8 +64,10 @@ import {
 import type {
   DataRouteMatch,
   DataRouteObject,
+  IndexRouteObject,
   Navigator,
   NavigateOptions,
+  NonIndexRouteObject,
   RouteMatch,
   RouteObject,
   RelativeRoutingType,
@@ -116,6 +118,7 @@ export type {
   DataRouteObject,
   Fetcher,
   Hash,
+  IndexRouteObject,
   IndexRouteProps,
   JsonFunction,
   LayoutRouteProps,
@@ -128,6 +131,7 @@ export type {
   NavigateProps,
   Navigation,
   Navigator,
+  NonIndexRouteObject,
   OutletProps,
   Params,
   ParamParseKey,

diff --git a/packages/react-router/lib/components.tsx b/packages/react-router/lib/components.tsx
@@ -1,7 +1,6 @@
 import * as React from "react";
 import type {
   TrackedPromise,
-  HydrationState,
   InitialEntry,
   Location,
   MemoryHistory,
@@ -22,9 +21,11 @@ import { useSyncExternalStore as useSyncExternalStoreShim } from "./use-sync-ext
 
 import type {
   DataRouteObject,
+  IndexRouteObject,
   RouteMatch,
   RouteObject,
   Navigator,
+  NonIndexRouteObject,
   RelativeRoutingType,
 } from "./context";
 import {
@@ -220,49 +221,46 @@ export function Outlet(props: OutletProps): React.ReactElement | null {
   return useOutlet(props.context);
 }
 
-interface DataRouteProps {
-  id?: RouteObject["id"];
-  loader?: RouteObject["loader"];
-  action?: RouteObject["action"];
-  errorElement?: RouteObject["errorElement"];
-  shouldRevalidate?: RouteObject["shouldRevalidate"];
-  handle?: RouteObject["handle"];
-}
-
-export interface RouteProps extends DataRouteProps {
-  caseSensitive?: boolean;
-  children?: React.ReactNode;
-  element?: React.ReactNode | null;
-  index?: boolean;
-  path?: string;
-}
-
-export interface PathRouteProps extends DataRouteProps {
-  caseSensitive?: boolean;
-  children?: React.ReactNode;
-  element?: React.ReactNode | null;
+export interface PathRouteProps {
+  caseSensitive?: NonIndexRouteObject["caseSensitive"];
+  path?: NonIndexRouteObject["path"];
+  id?: NonIndexRouteObject["id"];
+  loader?: NonIndexRouteObject["loader"];
+  action?: NonIndexRouteObject["action"];
+  hasErrorBoundary?: NonIndexRouteObject["hasErrorBoundary"];
+  shouldRevalidate?: NonIndexRouteObject["shouldRevalidate"];
+  handle?: NonIndexRouteObject["handle"];
   index?: false;
-  path: string;
-}
-
-export interface LayoutRouteProps extends DataRouteProps {
   children?: React.ReactNode;
   element?: React.ReactNode | null;
+  errorElement?: React.ReactNode | null;
 }
 
-export interface IndexRouteProps extends DataRouteProps {
-  element?: React.ReactNode | null;
+export interface LayoutRouteProps extends PathRouteProps {}
+
+export interface IndexRouteProps {
+  caseSensitive?: IndexRouteObject["caseSensitive"];
+  path?: IndexRouteObject["path"];
+  id?: IndexRouteObject["id"];
+  loader?: IndexRouteObject["loader"];
+  action?: IndexRouteObject["action"];
+  hasErrorBoundary?: IndexRouteObject["hasErrorBoundary"];
+  shouldRevalidate?: IndexRouteObject["shouldRevalidate"];
+  handle?: IndexRouteObject["handle"];
   index: true;
+  children?: undefined;
+  element?: React.ReactNode | null;
+  errorElement?: React.ReactNode | null;
 }
 
+export type RouteProps = PathRouteProps | LayoutRouteProps | IndexRouteProps;
+
 /**
  * Declares an element that should be rendered at a certain URL path.
  *
  * @see https://reactrouter.com/docs/en/v6/components/route
  */
-export function Route(
-  _props: PathRouteProps | LayoutRouteProps | IndexRouteProps
-): React.ReactElement | null {
+export function Route(_props: RouteProps): React.ReactElement | null {
   invariant(
     false,
     `A <Route> is only ever to be used as the child of <Routes> element, ` +
@@ -569,6 +567,11 @@ export function createRoutesFromChildren(
       }] is not a <Route> component. All component children of <Routes> must be a <Route> or <React.Fragment>`
     );
 
+    invariant(
+      !element.props.index || !element.props.children,
+      "An index route cannot have child routes."
+    );
+
     let treePath = [...parentPath, index];
     let route: RouteObject = {
       id: element.props.id || treePath.join("-"),

diff --git a/packages/react-router/lib/context.ts b/packages/react-router/lib/context.ts
@@ -1,28 +1,55 @@
 import * as React from "react";
 import type {
-  TrackedPromise,
+  AgnosticRouteMatch,
+  AgnosticIndexRouteObject,
+  AgnosticNonIndexRouteObject,
   History,
   Location,
   Router,
   StaticHandlerContext,
   To,
-  AgnosticRouteObject,
-  AgnosticRouteMatch,
+  TrackedPromise,
 } from "@remix-run/router";
 import type { Action as NavigationType } from "@remix-run/router";
 
 // Create react-specific types from the agnostic types in @remix-run/router to
 // export from react-router
-export interface RouteObject extends AgnosticRouteObject {
+export interface IndexRouteObject {
+  caseSensitive?: AgnosticIndexRouteObject["caseSensitive"];
+  path?: AgnosticIndexRouteObject["path"];
+  id?: AgnosticIndexRouteObject["id"];
+  loader?: AgnosticIndexRouteObject["loader"];
+  action?: AgnosticIndexRouteObject["action"];
+  hasErrorBoundary: AgnosticIndexRouteObject["hasErrorBoundary"];
+  shouldRevalidate?: AgnosticIndexRouteObject["shouldRevalidate"];
+  handle?: AgnosticIndexRouteObject["handle"];
+  index: true;
+  children?: undefined;
+  element?: React.ReactNode | null;
+  errorElement?: React.ReactNode | null;
+}
+
+export interface NonIndexRouteObject {
+  caseSensitive?: AgnosticNonIndexRouteObject["caseSensitive"];
+  path?: AgnosticNonIndexRouteObject["path"];
+  id?: AgnosticNonIndexRouteObject["id"];
+  loader?: AgnosticNonIndexRouteObject["loader"];
+  action?: AgnosticNonIndexRouteObject["action"];
+  hasErrorBoundary: AgnosticNonIndexRouteObject["hasErrorBoundary"];
+  shouldRevalidate?: AgnosticNonIndexRouteObject["shouldRevalidate"];
+  handle?: AgnosticNonIndexRouteObject["handle"];
+  index?: false;
   children?: RouteObject[];
   element?: React.ReactNode | null;
   errorElement?: React.ReactNode | null;
 }
 
-export interface DataRouteObject extends RouteObject {
+export type RouteObject = IndexRouteObject | NonIndexRouteObject;
+
+export type DataRouteObject = RouteObject & {
   children?: DataRouteObject[];
   id: string;
-}
+};
 
 export interface RouteMatch<
   ParamKey extends string = string,

diff --git a/packages/router/__tests__/router-test.ts b/packages/router/__tests__/router-test.ts
@@ -28,7 +28,7 @@ import {
 } from "../index";
 
 // Private API
-import type { TrackedPromise } from "../utils";
+import type { AgnosticRouteObject, TrackedPromise } from "../utils";
 import { AbortedDeferredError } from "../utils";
 
 ///////////////////////////////////////////////////////////////////////////////
@@ -1002,6 +1002,28 @@ describe("a router", () => {
       );
     });
 
+    it("throws if it finds index routes with children", async () => {
+      let routes = [
+        // @ts-expect-error
+        {
+          index: true,
+          children: [
+            {
+              path: "nope",
+            },
+          ],
+        },
+      ];
+      expect(() =>
+        createRouter({
+          routes,
+          history: createMemoryHistory(),
+        })
+      ).toThrowErrorMatchingInlineSnapshot(
+        `"Cannot specify children on an index route"`
+      );
+    });
+
     it("supports a basename prop for route matching", async () => {
       let history = createMemoryHistory({
         initialEntries: ["/base/name/path"],

diff --git a/packages/router/index.ts b/packages/router/index.ts
@@ -3,8 +3,12 @@ import { convertRoutesToDataRoutes } from "./utils";
 export type {
   ActionFunction,
   ActionFunctionArgs,
+  AgnosticDataIndexRouteObject,
+  AgnosticDataNonIndexRouteObject,
   AgnosticDataRouteMatch,
   AgnosticDataRouteObject,
+  AgnosticIndexRouteObject,
+  AgnosticNonIndexRouteObject,
   AgnosticRouteMatch,
   AgnosticRouteObject,
   TrackedPromise,