Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Possible useParams TypeScript broken definition #8200

Closed
DragosMocrii opened this issue Nov 4, 2021 · 30 comments
Closed

Possible useParams TypeScript broken definition #8200

DragosMocrii opened this issue Nov 4, 2021 · 30 comments

Comments

@DragosMocrii
Copy link

DragosMocrii commented Nov 4, 2021

import { useParams } from "react-router-dom"

interface JobPageRouteParams {
    jobId: string
}

const MyComponent = () => {
const { jobId } = useParams<JobPageRouteParams>();
}

gives an error: Type 'JobPageRouteParams' does not satisfy the constraint 'string'. .

The definition on useParams is

export declare function useParams<Key extends string = string>(): Readonly<Params<Key>>;

which means that the generic type is supposed to be a string?? Am I missing something here?

@DragosMocrii
Copy link
Author

I looked more into the implementation of useParams, and I can't understand the decision for the current types. Wouldn't the following be more appropriate:

type RouteParams<T> = {
    readonly [key in keyof T]: string | undefined;
};

export function useParams<T extends Record<string, string>>(): Readonly<RouteParams<T>> {
  let { matches } = React.useContext(RouteContext);
  let routeMatch = matches[matches.length - 1];
  return ( routeMatch ? (routeMatch.params) : {} ) as RouteParams<T>;
}

Now, I could use it like this:

type JobPageRouteParams = {
    jobId: string
};

const { jobId } = useParams<JobPageRouteParams>();

@Grapedge
Copy link

Grapedge commented Nov 4, 2021

Yes, but we can now use it like this:

const { code, tab } = useParams<'code' | 'tab'>()
// notice: `code`, `tab` may be string or undefined

Snipaste_2021-11-04_13-50-06

@DragosMocrii
Copy link
Author

Thank you, I was using old tutorials for v5 where the definition was different, and got tunnel vision. I see I can use string literals now.

On another note: v6 now returns the value of a param as string | undefined. What is the correct approach to using the value of a param? Doing a null check seems counterproductive and does not align well with function components. Using the non-null assertion operator seems kinda hacky. Am I missing the better approach?

@mjackson mjackson closed this as completed Nov 4, 2021
@arendvosmaer
Copy link

On another note: v6 now returns the value of a param as string | undefined. What is the correct approach to using the value of a param? Doing a null check seems counterproductive and does not align well with function components. Using the non-null assertion operator seems kinda hacky. Am I missing the better approach?

I'm also curious about this: the behavior used to be that an undefined param would give you a 404, which I think makes sense, as the route doesn't match.

@benneq
Copy link

benneq commented Nov 5, 2021

Those undefined params are bugging me.

I understand that in theory you could access some param key that's not within the route:

<Route path=":foo" />

const { bar } = useParams();
// bar === undefined

If you write code like this, then a check for bar !== undefined won't fix your code, instead it will just make it compile without errors and this will hide the underlying problem.

Though I'd vote to remove the undefined.


As an alternative you could leave the choice to the user which behavior he prefers, with something like this:

function useParams<K extends string, V extends string | undefined = string | undefined>(): {[key in K]: V} {
    // ...
}

const p1 = useParams(); // = {[key: string]: string | undefined}
const p2 = useParams<"foo" | "bar">(); // = {foo: string | undefined, bar: string | undefined}
const p3 = useParams<"foo" | "bar", string>(); // = {foo: string, bar: string}

@DragosMocrii
Copy link
Author

DragosMocrii commented Nov 5, 2021

I ended up using the non-null assertion operator when I want to tell TypeScript to shut up. So

<Route path=":foo" />

const { bar } = useParams<"bar">();
const fetchBar = useBar(bar!)

@chaance
Copy link
Collaborator

chaance commented Nov 6, 2021

The rationale here for us, as I see it (and I've talked to Michael about it a bunch and believe he's of the same mind):

  • Current type signature is simpler to type and output of string | undefined for values is technically correct
  • If you don't like it, you have options:
    • Cast the return value: let { slug } = useParams() as { slug: string }
    • Use the non-null assertion operator, as @DragosMocrii points out (simplest IMO, no idea why this seems hacky to some, it's a language feature for a reason!)
    • Declare your own module and use whatever type signature you'd like: https://codesandbox.io/s/modest-lalande-bw2pf?file=/src/App.tsx
    • Use patch-package and override our types in a postinstall hook
    • Turn off strict null checks

@ppbl
Copy link
Contributor

ppbl commented Nov 18, 2021

When I need to declare the type of the value, the current way cannot be achieved

Even if it must be a string, in ts, I can declare 'aa' |'bb' |'cc'

Before i can
image
There will be smart prompts when using params.type in this way

Not now, but this problem can be solved by function overloading

export function useParams<Key extends string = string>(): Readonly<Params<Key>>
export function useParams<
    P extends Record<string, string | undefined> = {
      [key: string]: string | undefined
    }
  >(): Partial<P>

In the code

 export function useParams<
    ParamsOrKey extends string | Record<string, string | undefined> = string
  >(): [ParamsOrKey] extends [string]
    ? Readonly<Params<ParamsOrKey>>
    : Readonly<Partial<ParamsOrKey>>

The best of both worlds! If possible, I can submit a pr

(The returned P is processed by Partial, which is consistent with the current logic)

[ParamsOrKey] extends [string] is to avoid Distributive Conditional Types

@michaeljota
Copy link

Hi @chaance. I wanna talk about your points about the desition of using Partial.

You said: "Current type signature is simpler to type and output of string | undefined for values is technically correct".
Yes, it is technically correct, but technically doesn't mean fully. As this is right now, the only one that knows what is actually correct is the developer, and thus, the only one that should be able to describe this fully is the developer.

After that you mention:

If you don't like it, you have options:

  • The reason why the non-null assertion operator is a language feature is to allow JS to move to TS easily, by taking the risk. We don't have any risk here, if I know that my param needs to be set in order to be able to show the page, I don't need to use the non-null assertion operator, this is an anti-pattern in most cases, and its usage is not recommended unless really needed (again, not the case if I can describe the whole thing properly). eslint rule

  • Enhancing code that should work out the box sounds overkill for this.

  • Turning off strict null check is not an option. That's an architectural choice that should be internal to be taken.

I understand that you think this is the best to really express what this means. However, I think it would be better for everyone to document the cases where using a partial would make sense, and let the user decide what shape they want to use.

@alecglen
Copy link

2nd'ing @michaeljota's points - options 1 and 3 are both steps in the wrong direction as far as TS is concerned, and declaring your own module as a workaround seems very odd when the rationale is "it's simpler to type".

Also like @michaeljota said, in my use case string | undefined is objectively incorrect. There is no logical way to route to ItemDetailsPage with an undefined itemId.

Route: <Route path="/item-details/:itemId" element={<ItemDetailsPage />} />
Hook: const { itemId } = useParams<'itemId'>();.

@timdorr
Copy link
Member

timdorr commented Dec 13, 2021

Also like @michaeljota said, in my use case string | undefined is objectively incorrect. There is no logical way to route to ItemDetailsPage with an undefined itemId.

But you don't know that at compile time. TypeScript isn't aware of how the History API or the internals of React Router work. It can't enforce that for you, even if you have it that way logically. For example, you could be unit testing your ItemDetailsPage component without wrapping it in a Route

The idiomatic way of doing this is to add type guards for params. That's obviously pretty verbose in most cases, so we provide the generic with the safest case of what we know can exist in params at compile time.

@alecglen
Copy link

alecglen commented Dec 13, 2021

But you don't know that at compile time. TypeScript isn't aware of how the History API or the internals of React Router work.

That makes sense for why react-router can't automatically specify it as required. But then why does react-router make the assumption it could be undefined at compile time? There's no right answer for every possible scenario, which is why I believe @DragosMocrii's original interface proposal makes the most sense. #8200 (comment)

If I'm unit testing the component, then I'm just mocking useParams. I'm not changing the type signatures in my production component just to support tests.

@johnnyoshika
Copy link

I didn't want to scatter my code with the non-null assertion operator, so I decided to go with this. Equally bad (or good 😉), but I only need this declaration once inside a component instead of everywhere where the variable is used:

const { id } = useParams() as { id: string };

@ryanflorence
Copy link
Member

ryanflorence commented Jan 27, 2022

This keeps coming up. The thing to remember is that the user in charge of the URL and you can put a useParams anywhere you want in your code.

There is no way for TypeScript, or React Router, to know for sure that a certain param is actually in the URL. It requires a runtime check because your code does not control the value, the URL (at runtime) does.

Best we can do is let you say "I only expect these params" but that doesn't mean they are actually going to be there, because, again, those values come from the URL at runtime, not your code.

Would love to be wrong.

Bring in tiny-invariant and add the runtime check:

let params = useParams<'userId'>();
invariant(params.userId);
// carry on with type safety and a runtime check from invariant

@q-kirill
Copy link

q-kirill commented Feb 10, 2022

I ended up with such solution:

import { Params, useParams } from 'react-router-dom';

interface QueryParamTypes extends Params {
  orgId: string;
  teamId: string;
  teamType: TeamType;
}

const { orgId, teamId, teamType } = useParams() as QueryParamTypes;

This correctly resolves types and avoid using non-null assertion operator

@johnnyoshika
Copy link

@q-kirill Seems very verbose. What's the advantage of declaring an interface over inlining the type?

const { orgId, teamId, teamType } = useParams() as { orgId: string, teamId: string, teamType: TeamType };

@michaeljota
Copy link

I don't think that's solving something, but actually exposing why TS should be about "what this should be at run time" instead of "what it is at code time". Using casting like this is just telling TS "you have to trust me on this", and that's why TS is useful. Having to use casting like this because the package doesn't want to trust the author of the code is overkill.

@johnnyoshika
Copy link

@michaeljota completely agree! Casting like this defeats the purpose of TS, and that's why I added a warning to my suggestion with equally bad. What casting offers us is convenience at the cost of type safety. It's the developer's responsibility to decide whether the compromise is worth it.

@jayarjo
Copy link

jayarjo commented Apr 3, 2022

@chaance What is the point of undefined option in there? If param was undefined that Route wouldn't even load, right? So I don't get the point.

@tombuntus
Copy link

I think the react-router team is doing the right thing with the type definition here, and a runtime check is the right move. It makes your code more explicit, more robust to change, and easier to debug.

For a project I was doing at work, I wrote a useRequiredParams hook which does the runtime check and returns param variables with type string. I wrote it up here in case anyone wants to re-use. I opened a feature request here in case the maintainers see value in adding it (or something like it).

@johnnyoshika
Copy link

@tombuntus Neat solution. I'm not a TypeScript guru, but I think it can be cleaned up a bit using Type Guards.

@chaance
Copy link
Collaborator

chaance commented Apr 17, 2022

@chaance What is the point of undefined option in there? If param was undefined that Route wouldn't even load, right? So I don't get the point.

The point, as Ryan mentioned, is that you can call useParams anywhere in your code, not just the route module itself. Sure it'll always be defined in that module but elsewhere you'll have no such guarantee. It is possible that you may get undefined so the signature is correct, and folks who want the strictest type safety should get it.

You're welcome to keep adding your 👎 to this comment, but it outlines solutions that are totally reasonable and in-line with how TypeScript is designed to work. All of these are TypeScript features for a reason.

I'd also like to ask that you understand that a library cannot make everyone happy all of the time, so what we doing is the technically correct thing. Best we can do for all of the others is offer guidance, which I think we've done here.

@jayarjo
Copy link

jayarjo commented Apr 20, 2022

@chaance I believe this is kind of bug that will bite you for years to come.

It can be undefined elsewhere, but using it elsewhere would be a mistake on the dev side. Params might be string | undefined in general, but when useParams gets a type explicitly, like useParams<SomeType> it should just use that type - that's what everybody expects I guess. As you might have noticed by reactions to the comment that you reference, none of the solutions are fully acceptable, moreover they are actually a hackarounds.

@CPatchane
Copy link

CPatchane commented Apr 29, 2022

Hello here 👋 ,

After reading some discussions and workarounds here, I thought it would be interesting to share a different approach to parameter typing using useParams. Maybe the parameters should be typed according to the path used in the parent Route component. Perhaps useParams could take the path of the route as a parameter so that the params types are inferred from the path itself instead of using a dedicated list. The use of the path can be bound to the component and written in only one place by declaring it as an attribute of the component.

Better code than words, here is how this new hook named useRouteParams looks like:

import React from "react"
import { Route, Routes, useParams } from "react-router-dom"

// The params type doesn't need to be optional anymore since now if a
// param is not detected in a path the related property won't exist at all
type NonNullableKeys<T> = {
  [P in keyof T]-?: NonNullable<T[P]>
}

// Transforms the path to params names as a string literal
type PathParamsLiteral<S extends string> = S extends
  | `:${infer T}/${infer U}`
  | `/:${infer T}/${infer U}`
  ? T | PathParamsLiteral<U>
  : S extends `${infer _T}/${infer U}`
  ? PathParamsLiteral<U>
  : S extends `:${infer T}` | `/:${infer T}`
  ? T
  : never

export const useRouteParams = <RoutePath extends string>(_path: RoutePath) => {
  const params = useParams<PathParamsLiteral<RoutePath>>()
  return params as NonNullableKeys<typeof params>
}

Below are some examples of the result:

// a: NonNullableKeys<Readonly<Params<"foo" | "fob" | "bar" | "oba">>>
const a = useRouteParams(":foo/:fob/:bar/:oba/foo/bar")
// b: NonNullableKeys<Readonly<Params<never>>>
const b = useRouteParams("/foo/bar/foobar")
// c: NonNullableKeys<Readonly<Params<"foo">>>
const c = useRouteParams(":foo")
// d: NonNullableKeys<Readonly<Params<"foo">>>
const d = useRouteParams("/:foo")
// e: NonNullableKeys<Readonly<Params<"foo" | "oba">>>
const e = useRouteParams("/:foo/foobar/:oba")
// f: NonNullableKeys<Readonly<Params<"oba">>>
const f = useRouteParams("/foo/bar/:oba/foobar")
// g: NonNullableKeys<Readonly<Params<"oba">>>
const g = useRouteParams("foo/bar/:oba/foobar")
// h: NonNullableKeys<Readonly<Params<never>>>
const h = useRouteParams("foo")

Then, the component that is bound to a path:

// Route path written only once at one place, living with the component/wrapper using it with useRouteParams
const MY_COMPONENT_PATH = ":foo/:fob/:bar/:oba/foo/bar"

const MyComponent = () => {
  const params = useRouteParams(MY_COMPONENT_PATH)

  return <p>{params.foo}</p>
}

MyComponent.routePath = MY_COMPONENT_PATH

And finally the usage of this component in the router configuration:

import React from "react"
import { Route, Routes } from "react-router-dom"
import { MyComponent } from "./MyComponent"

// To be even stricter, a dedicated Route component
// to enforce the path usage from the component
const AppRoute = <T extends Record<string, unknown>>({
  element: InnerElement,
  elementProps,
}: {
  element: ((props: T) => JSX.Element) & { routePath: string }
  elementProps: T
}) => (
  <Route
    path={InnerElement.routePath}
    element={<InnerElement {...elementProps} />}
  />
)

const App = (props) => (
  <Routes>
    <AppRoute element={MyComponent} elementProps={{ data: props.data }} />
  </Routes>
)

export default App

The fact that the component is linked to a single path could also be considered as a negative point as it makes the component less reusable. But this seems less error prone and a workaround could be to create a simple wrapper and use shared components inside it.

This approach does the job we've been waiting for, but I'd be curious to know what you think 🙂 .
@chaance I guess this could also be one more solution for the list of workarounds you made above 🙃.

I'm also clearly open to making a pull request if that's desired here otherwise we plan to put this in a dedicated NPM package.

@sorenhoyer
Copy link

sorenhoyer commented Jul 1, 2022

This keeps coming up. The thing to remember is that the user in charge of the URL and you can put a useParams anywhere you want in your code.

There is no way for TypeScript, or React Router, to know for sure that a certain param is actually in the URL. It requires a runtime check because your code does not control the value, the URL (at runtime) does.

Best we can do is let you say "I only expect these params" but that doesn't mean they are actually going to be there, because, again, those values come from the URL at runtime, not your code.

Would love to be wrong.

Bring in tiny-invariant and add the runtime check:

let params = useParams<'userId'>();
invariant(params.userId);
// carry on with type safety and a runtime check from invariant

@ryanflorence if you get asked this question a lot, perhaps you should include it in your examples where useParams is used.

@CPatchane
Copy link

For whoever are interested to have the hook useRouteParams like described just above, there is now a dedicated NPM package: @getaround-eu/use-route-params

Still interested by your feedbacks here :)

@Motoxpro
Copy link

Motoxpro commented Jul 23, 2022

This keeps coming up. The thing to remember is that the user in charge of the URL and you can put a useParams anywhere you want in your code.

There is no way for TypeScript, or React Router, to know for sure that a certain param is actually in the URL. It requires a runtime check because your code does not control the value, the URL (at runtime) does.

Best we can do is let you say "I only expect these params" but that doesn't mean they are actually going to be there, because, again, those values come from the URL at runtime, not your code.

Would love to be wrong.

Bring in tiny-invariant and add the runtime check:

let params = useParams<'userId'>();
invariant(params.userId);
// carry on with type safety and a runtime check from invariant

This is very informative. I wrote a hook that uses some ideas here. Thanks for having more foresight than I did.

    /**
   * A hook to typecheck and null check useParams.
   * We do this because react-router can never be sure of where we are calling useParams from.
   * Even if we are sure IN THIS MOMENT, in the future, a route component could be deleted or a param name could change
   * This gives us a test for that case
   */
  function useTypedParams<T extends string>(parameterNames: T[]): Record<T, string> {
    const params = useParams();
    const typedParams: Record<string, string> = {};
    parameterNames.forEach((paramName) => {
      const currentParam = params[paramName];
      invariant(
        currentParam,
        `${paramName} not found in useParams. Check the parent route to make sure nothing changed`,
      );
      typedParams[paramName] = currentParam;
    });
    return typedParams;
  }

  const { organizerSlug } = useTypedParams(['organizerSlug']);

@IanVS
Copy link

IanVS commented Sep 21, 2022

Previously we could use the render prop to access route props, which had knowledge about the params as specified by the path. This was excellent, because I got autocompletion of the params and I didn't have to manually specify them or perform runtime checks. I'm migrating from v5 to v6 and I'm going to miss this.

@patostickar
Copy link

In order to access a param in the URL, you need to give useParams a proper type argument:

const { jobId } = useParams<{ jobId: string}>();

@michaeljota
Copy link

No, you can't. See here for example.

The issue is that we are telling to the router we know what is the shape of the params, and the router is ignoring this information and providing optional typing on top on that anyway. (And that for some reason we can't provide an interface as an argument).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests