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

[Bug]: useParams should not return undefined #8498

Closed
tzachov opened this issue Dec 17, 2021 · 12 comments
Closed

[Bug]: useParams should not return undefined #8498

tzachov opened this issue Dec 17, 2021 · 12 comments
Labels

Comments

@tzachov
Copy link

tzachov commented Dec 17, 2021

What version of React Router are you using?

6.1.1

Steps to Reproduce

Define a route with param (e.g. user/:id)
Use useParams:

const UserPage = () => {
  const { id } = useParams<'id'>()
  return <Profile userId={id} />
}

const Profile: React.FC<{ userId: string }> = ({ userId }) => {...}

This won't compile because id is possibly undefined and Profile expects string

Expected Behavior

id should not be string | undefined because /user should match a different route than /user/123

Actual Behavior

id is string | undefined unless you use cast:

interface UserParams {
  id: string
}

const UserPage = () => {
  const { id } = useParams<keyof UserParams>() as UserParams
  ...
}
@tzachov tzachov added the bug label Dec 17, 2021
@timdorr
Copy link
Member

timdorr commented Dec 17, 2021

You cannot know at compile time the component will be contained within any Route. Hence, the params must be partial. We have specifically chosen this type because anything else would encourage unsafe behavior and produce types that essentially lie to you.

@timdorr timdorr closed this as completed Dec 17, 2021
@tzachov
Copy link
Author

tzachov commented Dec 18, 2021

It's a route param - not query string. If the parameter doesn't exist it means the route is different.
Let's say you have this configuration: /group/:groupId/post/:postId
If groupId is undefined the route will be /group/post/<postId> and if postId is undefined the route will be /group/<groupId>/post
These two examples should match different routes than /group/:groupId/post/:postId

A common convention is that path/route params are manditory while query params are optional.

@EnesKilicaslan
Copy link

I totally agree with @tzachov , this issue should be resolved instead of being closed.

@sagiMorPlanck
Copy link

sagiMorPlanck commented Dec 29, 2021

I agree with @tzachov.
The convention is that path/route params are mandatory while query params are optional

@talks2much
Copy link

talks2much commented Jan 16, 2022

@timdorr I think that decision was completely wrong, #8498 (comment) already explained why. By default params must be mandatory. Let developer choose when to use optional params:

const Post: React.FC = () => {
    const params = useParams<{ groupId: string, postId?: string }>()
    // expected: groupId is required and postId not
    // actual: both are optional ...
    /* ... */
}

You make useParams generic useless and in 100% cases its much easier to cast the type:

const Post: React.FC = () => {
    const params = useParams() as { groupId: string, postId?: string }
    /* ... */
}

Why don't make useParams accept two args in generic:

  • First one for mandatory params
  • Second one for optional

And in case of passing an object force devs that want old behavior to write ?.

UPD: didn't see #8200 (comment) and #8200 (comment) earlier, but that's exactly what I mean

zardoy added a commit to zardoy/vscode-better-snippets that referenced this issue Jan 16, 2022
    - make `type` of customSnippets typed. Previously it was a string, now it a string enum. Number support is preserved.
    - add `executeCommand` to allow running arbitrary commands on snippet accept
- ### improve builtin `useParam` snippet:
  1. Now it will appear on top with Event icon, similar to other builtin js snippets. Most importantly it now appear higher score than `useParams`
  2. Now it will expand to `const params = useParams() as { groupId: string }` instead of `const params = useParams<'groupId'>()`. You can look at the [react-router issue](remix-run/react-router#8498) for the motivation of this change. You can use `useParamMode` setting to switch to old behavior.
----
- allow to use captured group names from regexs of `when` in `body` of snippet
- ### Introduce New Type of Snippets!
- ### Typing Snippets
- More in readme
feat: ### New builtin snippets
  - `ed` -> `export default` (for js langs)
  - New snippets for markdown:
  - `ts` -> `` ```ts\n$1\n``` ``
  - `ts` -> `` ```ts\n$1\n``` ``
  - `codeblock` -> `` ```$1\n$2\n``` `` (fenced codeblock)
  - `dropdown` (fenced codeblock)
fix: Fix a rare case when the same snippet with multiple locations where included multiple times
fix: Allow to disable sortText applied by default snippet config
@vadimshvetsov
Copy link

vadimshvetsov commented Jun 26, 2022

I've found the most handy way for myself in using invariant here from tiny-invariant library.

import invariant from 'tiny-invariant'

interface UserParams {
  id: string
}

const UserPage = () => {
  const { id } = useParams<UserParams>()
  invariant(id, 'User id isn\'t set within the route')
  // After `invariant` evaluation `id` considering as truthy
  return <Profile userId={id} />
}

@spiropoulos94
Copy link

spiropoulos94 commented Sep 21, 2022

What version of React Router are you using?

6.1.1

Steps to Reproduce

Define a route with param (e.g. user/:id) Use useParams:

const UserPage = () => {
  const { id } = useParams<'id'>()
  return <Profile userId={id} />
}

const Profile: React.FC<{ userId: string }> = ({ userId }) => {...}

This won't compile because id is possibly undefined and Profile expects string

Expected Behavior

id should not be string | undefined because /user should match a different route than /user/123

Actual Behavior

id is string | undefined unless you use cast:

interface UserParams {
  id: string
}

const UserPage = () => {
  const { id } = useParams<keyof UserParams>() as UserParams
  ...
}

Hello, I was wondering about why do we have to explicitly set <keyof UserParams> as a type to pass on useParams. Nothing changes if we omit it and just call useParams() as UserParams instead.

I think that just casting it to our custom UserParams interface makes our code work propertly and also forces the return value to have the properties of the UserParams . Could you please explain? Maybe I am missing something.

Thank you for your time, this is really important for me, I have been searching an answer for ages.

@alex-ironside
Copy link

alex-ironside commented Oct 12, 2022

My solution is just a custom hook. It's not perfect, but it works

function useRequiredParams<T extends Record<string, any>>() {
  const params = useParams<T>();
  return params as T;
}

Usage:

export default function App() {
  const { id } = useRequiredParams<{ id: string }>();
  return <>test</>;
}

@ilbrando
Copy link

ilbrando commented Jan 7, 2023

My solution is just a custom hook. It's not perfect, but it works

function useRequiredParams<T extends Record<string, any>>() {
  const params = useParams<T>();
  return params as T;
}

Usage:

export default function App() {
  const { id } = useRequiredParams<{ id: string }>();
  return <>test</>;
}

I agree, useParams shouldn't force us to make casts everywhere. Your work around solves the problem. Just for your information it can be written a bit shorter and you can avoid any by using unknown.

export const useRequiredParams = <T extends Record<string, unknown>>() => useParams() as T;

@ArturKustyaev
Copy link

My solution is just a custom hook. It's not perfect, but it works

function useRequiredParams<T extends Record<string, any>>() {
  const params = useParams<T>();
  return params as T;
}

Usage:

export default function App() {
  const { id } = useRequiredParams<{ id: string }>();
  return <>test</>;
}

I agree, useParams shouldn't force us to make casts everywhere. Your work around solves the problem. Just for your information it can be written a bit shorter and you can avoid any by using unknown.

export const useRequiredParams = <T extends Record<string, unknown>>() => useParams() as T;

how to pass an interface to a generic, an error pops up?

@ilbrando
Copy link

ilbrando commented Sep 8, 2023

My solution is just a custom hook. It's not perfect, but it works

function useRequiredParams<T extends Record<string, any>>() {
  const params = useParams<T>();
  return params as T;
}

Usage:

export default function App() {
  const { id } = useRequiredParams<{ id: string }>();
  return <>test</>;
}

I agree, useParams shouldn't force us to make casts everywhere. Your work around solves the problem. Just for your information it can be written a bit shorter and you can avoid any by using unknown.

export const useRequiredParams = <T extends Record<string, unknown>>() => useParams() as T;

how to pass an interface to a generic, an error pops up?

I think this has to do with interfaces and Record, see this

By the way, I have stopped using interfaces and now only uses types, but of course that is not always an option.

@609harsh
Copy link

Which version release has this problem solved? i am using 6.27.0 and issue is still there

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

No branches or pull requests