router icon indicating copy to clipboard operation
router copied to clipboard

params became a required property for string typed paths after v1.0.8

Open KurtGokhan opened this issue 1 year ago • 6 comments

Describe the bug

See this line:

router.matchRoute({ to: '/posts' as string });

I am deliberately casting the path to string to show the issue. Because sometimes the path is a variable and you don't know its exact type. This used to work before v1.0.8.

It works if it is written like:

router.matchRoute({ to: '/posts' as string, params: {} });

The problem is, these two codes do the same thing. If params accept {}, it should also accept undefined.

Your Example Website or App

https://stackblitz.com/edit/tanstack-router-u6suse?file=src%2Fmain.tsx

Steps to Reproduce the Bug or Issue

Visit the stackblitz example above and see the typescript error

Expected behavior

If params accept {}, it should also accept undefined. It should be optional to write {}

Screenshots or Videos

No response

Platform

  • Version >= 1.0.8

Additional context

Although I used matchRoute API above, this is also a problem in other APIs. You can try passing a string typed path to Link and observe the same issue.

image

KurtGokhan avatar Jan 04 '24 17:01 KurtGokhan

When you pass in a string instead of a typed string literal, router does not know the route at compile time. Thusparams will be the union of all possible path params of all routes. In your example, this is:

{} | {} | { postId: string; }

The reasoning behind this is: You need to find out the route at runtime and then decide which value should be used as params. At runtime, the route could be postRoute, and then you would need to supply postId.

If params would accept e.g. undefined, what would happen for postRoute?

schiller-manuel avatar Jan 04 '24 20:01 schiller-manuel

Per your logic, I shouldn't be able to pass {}, because it would be invalid for postRoute, but I can.

What I am coming to is, whenever params accepts {}, it should also accept undefined, because they are essentially doing the same thing (replaces nothing).

{} | {} | { postId: string; } can be assigned {}, so assigning undefined should also be possible.

Also, to be exactly sure that all possible paths are satisfied, we should pass an intersection of all possible path params, not an union. So it should be {} & {} & { postId: string; }. But that would be a DX disaster. We should be pragmatic about it.

KurtGokhan avatar Jan 04 '24 21:01 KurtGokhan

Per your logic, I shouldn't be able to pass {}, because it would be invalid for postRoute, but I can.

No, you need to deliberately choose which of the union members you want to pass in. Allowing undefined would mean you can simply ignore the search params for any route...

schiller-manuel avatar Jan 04 '24 22:01 schiller-manuel

@KurtGokhan stated that before 1.0.8, this worked, and while that's true, it was working when it shouldn't have. We tightened up the screws on some things that were loose.

The rules around to are pretty simple:

  • to must be constrained to a string
  • to's actual type value can resolve to an individual route id, to: '/posts' or to multiple unique route ids, to: '/posts' | '/posts/$postId'
  • to can also resolved to a relative navigation path, which should also end up at a route ID
  • Whatever routes to resolves will be considered for other props, including params

Under these circumstances, I don't see a problem since if to actually does resolve to string, then you must truly have a circumstance where every single route technically is a possibility, in which case, you would want to at least supply params: true. I personally feel though that this outcome is usually per lack of an as const somewhere that would allow your to value to be more constrained.

Despite it erroneously working a few versions ago (we are still fixing some things), a simple as any to let the compiler know that you know better can go a long way.

tannerlinsley avatar Jan 05 '24 17:01 tannerlinsley

More context on why I don't think we should do anything here:

to: string is technically an allowed constraint so we can also support relative path traversal, so we can't get away from that, but while it is a constraint, it's not really a supported resolved type for to, since as @schiller-manuel mentioned, it causes the destination route to be all of the routes, merged together.

If we loosen up the types on the resulting params to accept nothing when to resolved to string, then we're opening up a huge type safety loss for vast majority of people that will inevitably miss an as const or write some reusable function for navigate that ultimately passes a widened to: string type and unwittingly sees zero errors.

tannerlinsley avatar Jan 05 '24 17:01 tannerlinsley

I get your point. But trying to achieve perfect type safety seems futile to me. People will eventually need escape-hatches and string seems like the best type for that. This is especially true for some APIs like matchRoute where you are usually working with a dynamic variable rather than a constant path. (Why would I use matchRoute when I know exactly which route I am trying to match?)

I am ok with keeping it as i. I agree it is somewhat safer to require the user to explicitly type-out the params.

to: string is technically an allowed constraint so we can also support relative path traversal, so we can't get away from that, but while it is a constraint, it's not really a supported resolved type for to, since as @schiller-manuel mentioned, it causes the destination route to be all of the routes, merged together.

Wouldn't string be a valid type for path if I define a catch-all route though?

KurtGokhan avatar Jan 05 '24 19:01 KurtGokhan