Make Params and related functions generic

[chaance]

The Params type and functions that consume it should be generic to allow simpler static typing for query params.

// before
let { valid, invalid, literallyAnything } = useParams();

let match = useMatch("profile/:userId");
let userId = match?.params.user; // wrong param, but TS doesn't know that!

// after:
// Property 'invalid' does not exist on type 'Params<"valid" | "key">'
let { valid, invalid } = useParams<"valid" | "key">();

let match = useMatch<"userId">("profile/:userId");
// Property 'user' does not exist on type 'Params<"userId">'
let userId = match?.params.user;

Note, the generic type defaults to string so this doesn't break anyone's existing code, but it will enhance it for those looking for added type awareness.

Resolves #7978

[timdorr]

The main reason I've resisted making useParams generic is because you might not know the context it's called within. For instance, I have these routes for the header of my application (v5, but still the same idea):

<Switch>
  <Route path="/teams/:team/apps/:app" component={PageHeader} />
  <Route path="/teams/:team" component={PageHeader} />
  <Route component={PageHeader} />
</Switch>

I show a breadcrumb menu at the top of the page depending on whether you're in the team, app, or neither context of the site. That makes the :team and :app parameters effectively optional.

With this system for generics, there is no way for me to represent that typing. I would have to override it with a type assertion.

interface TeamAppParams {
  team?: string
  app?: string
}

const { team, app } = useParams() as TeamAppParams

I think my core point is you might not always know the Route definition at your call site, which makes this a dangerous type assertion and could easily lead to hard-to-find bugs.

[chaance]

Good points and I don't necessarily disagree, which I why I think it ought to default to string in all cases. If you add more specific types it's kind of a contract you sign with yourself.

That said, I think you make a good case for the value being potentially undefined here.

[mjackson]

Looks good! 👍

[mjackson]

Can we reuse the Params type in this file instead of declaring our own Record type here? Not a big deal, just curious.

[chaance]

Yeah totally, will make a TODO for this.

[icoloma]

If useParams() used an interface instead of the key names, it would let the user specify their own assumptions about what is nullable for a given URL. With current syntax:

  const { id } = useParams<'id'>();
  doSomething(id); // typescript error, id can be null
  doSomething(id!); // workaround is this, everywhere

I would rather expect a syntax like useState():

  interface MyRouteParams {
    id: string
  };
  const { id } = useParams<MyRouteParams>();
  doSomething(id); // id cannot be null

This takes into account that as a user, I should know what comes in my route and what can and cannot be undefined. useParams() does not know this, and assumes that everything can be undefined.

[timdorr]

The type of id is going to be string | undefined. You can't make the types required (or worse, non-strings), because there's no way for the static compilation system to know that is true. It's a runtime type, based on window.location essentially. If we allow other types, those will invariably lie at some point and cause subtle and hard-to-explain bugs.

[icoloma]

I don't see how that is different from any other serialized form, like reading values from a database or deserializing JSON. Required data may or may not be there, and there is always friction between what the code expects and the restrictions set by the underlying storage system (in this case, the URL).

No objections to the type argument, but can be solved with a supertype:

interface Params {
  [key: string]: string | undefined
}

interface MyParams extends Params {
  foo: string,
  bar: string | undefined,
  baz: int // error
}

[gopeter]

How would you deal with this now?

const { slug } = useParams<'slug'>()
const [{ data, error }] = useQueryData({ variables: { slug } })

Since hooks can't be called conditionally, I can't render something else. The only option I see here is to split it into multiple components, which would drive my crazy 🙈

[gopeter]

I just want to add that I totally understand why you made it string | undefined. But there are places that go beyond Typescript's capabilities. Let's take this example:

<Route path="/projects">
  <Route path="new" element={<ProjectNew />} />
  <Route path=":slug" element={<Project />} />
</Route>

...

function Project() {
  const { slug } = useParams()
  const [{ data }] = useQueryData({ variables: { slug } })

  return <div>Project title: {data.title}</div>
}

Since I know that I do not use Project as a component which can be placed everywhere, I also know that slug won't be undefined. So I'm going to cast my vars with const { slug } = useParams() as { slug: string }, but generics would be more pleasant 😄

[timdorr]

It depends on your hook, of course, but things like SWR get around this by accepting null params as a noop:

useSWR(slug ? `/api/projects/${slug}` : null)

In your case, you could do one of two things:

const [{ data }] = useQueryData({ variables: { slug: slug! } })
// or
const [{ data }] = useQueryData(slug ? { variables: { slug } } : {})

Depends on what is more idiomatic for you.

[gopeter]

I've created typed urql queries with graphql-codegen which does not allow running queries without required arguments. For now I'm happy using const { slug } = useParams() as { slug: string } instead of { variables: { slug: slug! } } 🙂

[timdorr]

You could also do useQueryData({ variables: { slug: slug as string } }) if you prefer that form better.

I suppose one thing we could do is add a second type parameter for the value type. That wouldn't stop folks from doing dumb things like number | object | MyType... etc. but would involve less casting.

[gopeter]

That would be great 😀 I would be happy if you could think about this option.

I did { slug: slug as string } before, but since I'm using the param on other places in my code also, I casted the params itself.