`params` argument in `generatePath` utility has incorrect type

[anonpay-sh]

Reproduction

https://stackblitz.com/edit/github-fguzcsck?file=app%2Froutes%2Fhome.tsx

Note that you may need to click in the editor and add then remove a space to trigger a type-checking pass.

System Info

System:
  OS: Linux 5.0 undefined
  CPU: (8) x64 Intel(R) Core(TM) i9-9880H CPU @ 2.30GHz
  Memory: 0 Bytes / 0 Bytes
  Shell: 1.0 - /bin/jsh
Binaries:
  Node: 20.19.1 - /usr/local/bin/node
  Yarn: 1.22.19 - /usr/local/bin/yarn
  npm: 10.8.2 - /usr/local/bin/npm
  pnpm: 8.15.6 - /usr/local/bin/pnpm
npmPackages:
  @react-router/dev: ^7.5.3 => 7.6.3
  @react-router/node: ^7.5.3 => 7.6.3
  @react-router/serve: ^7.5.3 => 7.6.3
  react-router: ^7.5.3 => 7.6.3
  vite: ^6.3.3 => 6.3.5

Used Package Manager

npm

Expected Behavior

As shown as the reproduction, the current runtime behavior of generatePath is as follows:

• generatePath("/users/:id?", { id: "123" }) returns /users/123.
• generatePath("/users/:id?", { id: null }) returns /users.
• generatePath("/users/:id?", { id: undefined }) returns /users.
• generatePath("/users/:id?", {}) returns /users.

Because generatePath uses == null and != null checks in its implementation, it accepts either null or undefined for optional parameters.

react-router/packages/react-router/lib/router/utils.ts

Lines 1091 to 1092 in 0dea762

	const stringify = (p: any) =>
	p == null ? "" : typeof p === "string" ? p : String(p);

react-router/packages/react-router/lib/router/utils.ts

Line 1110 in 0dea762

invariant(optional === "?" || param != null, `Missing ":${key}" param`);

I would expect that its type signature would similarly accommodate all these options.

Actual Behavior

The params argument in generatePath currently has the type params: { [key in PathParam<Path>]: string | null; }, meaning it requires missing parameters to be specified as null, and doesn't allow them to be specified as undefined or omitted entirely.

react-router/packages/react-router/lib/router/utils.ts

Lines 1072 to 1074 in 0dea762

	params: {
	[key in PathParam<Path>]: string | null;
	} = {} as any

This is problematic because it rules out some of the usages that yield valid runtime behavior.

The expression generatePath("/users/:id?", { id: undefined }) currently gives the following type error:

The expression generatePath("/users/:id?", {}) currently gives the following type error:

To match the runtime behavior of generatePath, we should instead be using the type params: { [key in PathParam<Path>]?: string | null | undefined; }. The ? would allow a parameter to be omitted, and the | undefined would allow it to be undefined.

Note that once this change is made, the default value for params could also be simplified from = {} as any to just = {}, since {} would now be a valid value for the specified type.

[brophdawg11]

I agree undefined should be allowed, but I don't think we want to make the key optional because wouldn't that allow you to omit non-optional params? I think requiring you to specify the optional param name with a valuer of null/undefined is nice in that it makes the optionality explicit.

[anonpay-sh]

I like the idea of going with a more explicit approach, but if you don't allow parameters to be omitted, the current default value of {} doesn't really make sense, does it?

react-router/packages/react-router/lib/router/utils.ts

Lines 1072 to 1074 in 0dea762

	params: {
	[key in PathParam<Path>]: string | null;
	} = {} as any

If a non-optional parameter is omitted, you'll still get a runtime error thanks to the following invariant:

react-router/packages/react-router/lib/router/utils.ts

Line 1110 in 0dea762

invariant(optional === "?" || param != null, `Missing ":${key}" param`);

But currently, while I'm allowed to call generatePath("/users/:id?"), which is equivalent to generatePath("/users/:id?", {}), I'm not allowed to explicitly call the latter, which is a bit awkward.

Here are a few options:

Proposal 1: Update the type to params: { [key in PathParam<Path>]: string | null | undefined; }. This is in line with your suggestion, and I think is an improvement over the status quo with no downsides.

Proposal 2a: Update the type to params: { [key in PathParam<Path>]?: string | null | undefined; } and change the default value from = {} as any to just = {}. This has the advantage of bringing the type fully in agreement with the current runtime behavior implied by the default value, but loses the explicit enumeration of parameter values, which we may not want.

Proposal 2b: Update the type to params: { [key in PathParam<Path>]: string | null | undefined; } and drop the default value of = {} as any. This is another way to bring the type fully in agreement with the runtime behavior, and would have the advantage of keeping things explicit, but would be a breaking change.

Perhaps we can start by incorporating proposal 1, then as a follow-up task pursue one of proposal 2a or 2b if there's support for either of them.

[anonpay-sh]

@brophdawg11 Would you like to proceed with any of these proposals? Are you able to make the change? It should be a one- or two-line fix depending on which one we go with.

[brophdawg11]

Have you considered using href by chance? That is the new and improved "link builder" in framework mode that's aware of your route tree and fully typesafe. generatePath has had typesafety bolted on in v6 but it's not perfect (as shown by this issue). The new types in framework mode in v7 (including href) are where we see the type-safety story heading.

href does exactly what we want here so I would be in favor of whatever changes get us in line with that behavior - maybe we could copy of of the internals of the href implementation?

href("/users/:id?"); // ✅
href("/users/:id?", {}); // ✅
href("/users/:id?", { id: "123" }); // ✅
href("/users/:id"); // ❌ Expected 2 arguments but got 1...
href("/users/:id", {}); // ❌ Property "id" is missing ...
href("/users/:id", { id: "123" }); // ✅

[anonpay-sh]

Unfortunately href isn't exactly suitable for my use case, since it appears to require an externally registered Pages type auto-generated by framework mode to give you full type safety, whereas I'm still on declarative mode and am using generatePath in a more lightweight and standalone manner.

I agree that the types you showed in your last post look ideal for a utility like this, but since PathParam doesn't differentiate between optional and required parameters, it's not quite powerful enough to replicate the conditional second argument.

To get full parity, I think we'd need to introduce something like a PathParams utility type that takes in a path template and returns the corresponding params type, e.g. PathParams<"/users/:id?"> = { id?: string } and PathParams<"/users/:id"> = { id: string }.

Does that already exist somewhere? Or could it be extracted from the framework mode code that auto-generates parameter types for pages? If not, I imagine it could be implemented in a manner similar to how PathParam is implemented now.

In the meantime, if it's not too much trouble, it also couldn't hurt to make a one- or two-line update to the existing generatePath type signature following one of my proposals from #14006 (comment) to provide an easy incremental typing improvement.

[brophdawg11]

ah, ok sorry I saw your reproduction was using framework mode and assumed you were as well.

I took a stab at differentiating between required/optional in #14043 so let me run that by some team members and see if that solves the issue. It's not quite as good as href but it should be a pretty decent improvement.

[anonpay-sh]

No worries, good to know about href for future projects using framework mode.

Also appreciate you putting up a PR for this. I think your solution gets us most of the way there.

The main remaining difference seems to be that href has some additional logic that only allows the second argument to be dropped if there are no parameters or if they're all optional, whereas the revised version of generatePath in your PR still preserves the not-quite-valid = {} as any default argument.

But I suppose you can't alter that without breaking backwards compatibility, so I think your PR is as far as we can go if we'd like to stick with a patch change. Maybe something to keep in mind for the next major version bump then.