fix href optionals (#13595)

Author: pcattori

.changeset/sweet-parents-hug.md

@@ -0,0 +1,30 @@
+---
+"@react-router/dev": patch
+---
+
+Fix `href` for optional segments
+
+Type generation now expands paths with optionals into their corresponding non-optional paths.
+For example, the path `/user/:id?` gets expanded into `/user` and `/user/:id` to more closely model visitable URLs.
+`href` then uses these expanded (non-optional) paths to construct type-safe paths for your app:
+
+```ts
+// original: /user/:id?
+// expanded: /user & /user/:id
+href("/user"); // ✅
+href("/user/:id", { id: 1 }); // ✅
+```
+
+This becomes even more important for static optional paths where there wasn't a good way to indicate whether the optional should be included in the resulting path:
+
+```ts
+// original: /products/:id/detail?
+
+// before
+href("/products/:id/detail?"); // ❌ How can we tell `href` to include or omit `detail?` segment with a complex API?
+
+// now
+// expanded: /products/:id & /products/:id/detail
+href("/product/:id"); // ✅
+href("/product/:id/detail"); // ✅
+```

integration/typegen-test.ts

@@ -99,15 +99,15 @@ test.describe("typegen", () => {
           import type { Expect, Equal } from "../expect-type"
           import type { Route } from "./+types/only-optional"
           export function loader({ params }: Route.LoaderArgs) {
-            type Test = Expect<Equal<typeof params, { id?: string }>>
+            type Test = Expect<Equal<typeof params.id, string | undefined>>
             return null
           }
         `,
         "app/routes/optional-then-required.tsx": tsx`
           import type { Expect, Equal } from "../expect-type"
           import type { Route } from "./+types/optional-then-required"
           export function loader({ params }: Route.LoaderArgs) {
-            type Test = Expect<Equal<typeof params, { id: string }>>
+            type Test = Expect<Equal<typeof params.id,  string>>
             return null
           }
         `,
@@ -116,7 +116,7 @@ test.describe("typegen", () => {
           import type { Route } from "./+types/required-then-optional"
 
           export function loader({ params }: Route.LoaderArgs) {
-            type Test = Expect<Equal<typeof params, { id: string }>>
+            type Test = Expect<Equal<typeof params.id, string>>
             return null
           }
         `,
@@ -497,13 +497,17 @@ test.describe("typegen", () => {
         import { type RouteConfig, route } from "@react-router/dev/routes";
 
         export default [
+          route("optional-static/opt?", "routes/optional-static.tsx"),
           route("no-params", "routes/no-params.tsx"),
           route("required-param/:req", "routes/required-param.tsx"),
           route("optional-param/:opt?", "routes/optional-param.tsx"),
           route("/leading-and-trailing-slash/", "routes/leading-and-trailing-slash.tsx"),
           route("some-other-route", "routes/some-other-route.tsx"),
         ] satisfies RouteConfig;
       `,
+      "app/routes/optional-static.tsx": tsx`
+        export default function Component() {}
+      `,
       "app/routes/no-params.tsx": tsx`
         export default function Component() {}
       `,
@@ -522,13 +526,22 @@ test.describe("typegen", () => {
         // @ts-expect-error
         href("/does-not-exist")
 
+        href("/optional-static")
+        href("/optional-static/opt")
+        // @ts-expect-error
+        href("/optional-static/opt?")
+
         href("/no-params")
 
         // @ts-expect-error
         href("/required-param/:req")
         href("/required-param/:req", { req: "hello" })
 
+        href("/optional-param")
+        href("/optional-param/:opt", { opt: "hello" })
+        // @ts-expect-error
         href("/optional-param/:opt?")
+        // @ts-expect-error
         href("/optional-param/:opt?", { opt: "hello" })
 
         href("/leading-and-trailing-slash")

packages/react-router-dev/typegen/generate.ts

@@ -59,7 +59,7 @@ export function generateRoutes(ctx: Context): Array<VirtualFile> {
   // precompute
   const fileToRoutes = new Map<string, Set<string>>();
   const lineages = new Map<string, Array<RouteManifestEntry>>();
-  const pages = new Set<string>();
+  const allPages = new Set<string>();
   const routeToPages = new Map<string, Set<string>>();
   for (const route of Object.values(ctx.config.routes)) {
     // fileToRoutes
@@ -75,9 +75,10 @@ export function generateRoutes(ctx: Context): Array<VirtualFile> {
     lineages.set(route.id, lineage);
 
     // pages
-    const page = Route.fullpath(lineage);
-    if (!page) continue;
-    pages.add(page);
+    const fullpath = Route.fullpath(lineage);
+    if (!fullpath) continue;
+    const pages = explodeOptionalSegments(fullpath);
+    pages.forEach((page) => allPages.add(page));
 
     // routePages
     lineage.forEach(({ id }) => {
@@ -86,7 +87,7 @@ export function generateRoutes(ctx: Context): Array<VirtualFile> {
         routePages = new Set<string>();
         routeToPages.set(id, routePages);
       }
-      routePages.add(page);
+      pages.forEach((page) => routePages.add(page));
     });
   }
 
@@ -107,7 +108,7 @@ export function generateRoutes(ctx: Context): Array<VirtualFile> {
         }
       ` +
       "\n\n" +
-      Babel.generate(pagesType(pages)).code +
+      Babel.generate(pagesType(allPages)).code +
       "\n\n" +
       Babel.generate(routeFilesType({ fileToRoutes, routeToPages })).code,
   };
@@ -346,3 +347,49 @@ function paramsType(path: string) {
     })
   );
 }
+
+// https://github.com/remix-run/react-router/blob/7a7f4b11ca8b26889ad328ba0ee5a749b0c6939e/packages/react-router/lib/router/utils.ts#L894C1-L937C2
+function explodeOptionalSegments(path: string): string[] {
+  let segments = path.split("/");
+  if (segments.length === 0) return [];
+
+  let [first, ...rest] = segments;
+
+  // Optional path segments are denoted by a trailing `?`
+  let isOptional = first.endsWith("?");
+  // Compute the corresponding required segment: `foo?` -> `foo`
+  let required = first.replace(/\?$/, "");
+
+  if (rest.length === 0) {
+    // Interpret empty string as omitting an optional segment
+    // `["one", "", "three"]` corresponds to omitting `:two` from `/one/:two?/three` -> `/one/three`
+    return isOptional ? [required, ""] : [required];
+  }
+
+  let restExploded = explodeOptionalSegments(rest.join("/"));
+
+  let result: string[] = [];
+
+  // All child paths with the prefix.  Do this for all children before the
+  // optional version for all children, so we get consistent ordering where the
+  // parent optional aspect is preferred as required.  Otherwise, we can get
+  // child sections interspersed where deeper optional segments are higher than
+  // parent optional segments, where for example, /:two would explode _earlier_
+  // then /:one.  By always including the parent as required _for all children_
+  // first, we avoid this issue
+  result.push(
+    ...restExploded.map((subpath) =>
+      subpath === "" ? required : [required, subpath].join("/")
+    )
+  );
+
+  // Then, if this is an optional value, add all child versions without
+  if (isOptional) {
+    result.push(...restExploded);
+  }
+
+  // for absolute paths, ensure `/` instead of empty segment
+  return result.map((exploded) =>
+    path.startsWith("/") && exploded === "" ? "/" : exploded
+  );
+}