diff --git a/.changeset/sweet-parents-hug.md b/.changeset/sweet-parents-hug.md new file mode 100644 index 0000000000..7e84e2956a --- /dev/null +++ b/.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"); // ✅ +``` diff --git a/integration/typegen-test.ts b/integration/typegen-test.ts index d9c021446d..3ce1e638c2 100644 --- a/integration/typegen-test.ts +++ b/integration/typegen-test.ts @@ -99,7 +99,7 @@ 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> + type Test = Expect> return null } `, @@ -107,7 +107,7 @@ test.describe("typegen", () => { import type { Expect, Equal } from "../expect-type" import type { Route } from "./+types/optional-then-required" export function loader({ params }: Route.LoaderArgs) { - type Test = Expect> + type Test = Expect> 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> + type Test = Expect> return null } `, @@ -497,6 +497,7 @@ 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"), @@ -504,6 +505,9 @@ test.describe("typegen", () => { 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") diff --git a/packages/react-router-dev/typegen/generate.ts b/packages/react-router-dev/typegen/generate.ts index da847f2d0f..aa094a086a 100644 --- a/packages/react-router-dev/typegen/generate.ts +++ b/packages/react-router-dev/typegen/generate.ts @@ -59,7 +59,7 @@ export function generateRoutes(ctx: Context): Array { // precompute const fileToRoutes = new Map>(); const lineages = new Map>(); - const pages = new Set(); + const allPages = new Set(); const routeToPages = new Map>(); for (const route of Object.values(ctx.config.routes)) { // fileToRoutes @@ -75,9 +75,10 @@ export function generateRoutes(ctx: Context): Array { 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 { routePages = new Set(); routeToPages.set(id, routePages); } - routePages.add(page); + pages.forEach((page) => routePages.add(page)); }); } @@ -107,7 +108,7 @@ export function generateRoutes(ctx: Context): Array { } ` + "\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 + ); +}