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]: Typescript 4.9.x error when using #10505

Closed
gianluca932 opened this issue May 18, 2023 · 8 comments
Closed

[Bug]: Typescript 4.9.x error when using #10505

gianluca932 opened this issue May 18, 2023 · 8 comments
Labels

Comments

@gianluca932
Copy link

What version of React Router are you using?

6.11

Steps to Reproduce

Starting from <RouterProvider>.
Import useNavigate from "react-router-dom".
Paste this three functions on line 22 under App component.

const navigate = useNavigate();
function promptNavHandler(path: string | number): void {
    navigate(path);
}
const goBack = (num: number) => {
    navigate(num);
};
const goAt = (path: string) => {
    navigate(path);
};

Open a new terminal and run npm run build to run tsc

Expected Behavior

react-router ts files compile with no error

Actual Behavior

 Overload 1 of 2, '(to: To, options?: NavigateOptions | undefined): void', gave the following error.
    Argument of type 'string | number' is not assignable to parameter of type 'To'.
      Type 'number' is not assignable to type 'To'.
  Overload 2 of 2, '(delta: number): void', gave the following error.
    Argument of type 'string | number' is not assignable to parameter of type 'number'.
      Type 'string' is not assignable to type 'number'.

According to the documentation, navigate accepts both numbers or string, in order of going next/back in the history navigate(-1) or to reach the targeted route navigate('/pages').
When used alone navigate works pretty well, but not when it's wrapped in an other function.

@timdorr
Copy link
Member

timdorr commented May 19, 2023

The To type is defined as string | Partial<Path>. I don't see anywhere the docs reference a string | number type.

@timdorr timdorr closed this as completed May 19, 2023
@gianluca932
Copy link
Author

In packages/react-router/hooks.tsx, at line 983 inside the function useNavigateStable(), the default useNavigate function expects the to parameters to be To or number.

 let navigate: NavigateFunction = React.useCallback(
    (to: To | number, options: NavigateOptions = {}) => {
      warning(activeRef.current, navigateEffectWarning);

      // Short circuit here since if this happens on first render the navigate
      // is useless because we haven't wired up our router subscriber yet
      if (!activeRef.current) return;

      if (typeof to === "number") {
        router.navigate(to);
      } else {
        router.navigate(to, { fromRouteId: id, ...options });
      }
    },
    [router, id]
  );

As you stated the To type is imported from packages/router/history.ts and is:
export type To = string | Partial<Path>;
In my example above, Typescript doesn't match (string|number) types with their current overloads.
Furthermore, if I do something like that it works, because Typescript itself match or string or number.

function promptNavHandler(path: string | number): void {
    if(typeof path === "number"){
      navigate(path);
    } else {
      navigate(path);
    }
  }

So for me it's missing an overload here, on line 150 of react-router/hook.ts:

export interface NavigateFunction {
  (to: To, options?: NavigateOptions): void;
  (delta: number): void;
}

And it should become:

export interface NavigateFunction {
  (to: To, options?: NavigateOptions): void;
  (delta: number): void;
  (arg: To | number): void;
}

@gerardparareda
Copy link

@timdorr to date navigate(-1) is still in the documentation https://reactrouter.com/en/main/start/tutorial#cancel-button but impossible with the To typing string | Partial<Path>. Can you please check this issue again?

@timdorr
Copy link
Member

timdorr commented Jul 18, 2024

There is the overload of (delta: number) that supports that argument:

export interface NavigateFunction {
(to: To, options?: NavigateOptions): void;
(delta: number): void;
}

@timdorr
Copy link
Member

timdorr commented Jul 18, 2024

And just confirming that this works in TS 4.9 (as well as other versions):

https://www.typescriptlang.org/play/?ts=4.9.5&q=243#code/JYWwDg9gTgLgBAbzgVwM4FMByBDAbsAc2xnTgF84AzKCEOAcinWwGMYBaG5EqegKD4sIAO1TxheQsVIBeFBhz4iJABQBKARKXSV9APRhiAC3oatU1ewCMaoA

@JCown
Copy link

JCown commented Jul 18, 2024

Let's assume that we are using useNavigate hook in a component which accepts a prop that is used as an argument passed to the navigate function. Example type for such prop could be string | number (since navigate works with both of these types).

Without workarounds like mentioned above:

function promptNavHandler(path: string | number): void {
    if(typeof path === "number"){
      navigate(path);
    } else {
      navigate(path);
    }
  }

I think its not possible to satisfy TS cause basically we end up with something like this navigate('/path' as string | number)

@gerardparareda
Copy link

gerardparareda commented Jul 19, 2024

Let's assume that we are using useNavigate hook in a component which accepts a prop that is used as an argument passed to the navigate function. Example type for such prop could be string | number (since navigate works with both of these types).

Without workarounds like mentioned above:

function promptNavHandler(path: string | number): void {
    if(typeof path === "number"){
      navigate(path);
    } else {
      navigate(path);
    }
  }

I think its not possible to satisfy TS cause basically we end up with something like this navigate('/path' as string | number)

Albeit ugly, it does the job.

I wonder if this overload design was used because this way development was easier. From the user of the library perspective, it seems as if having a type that accepts To | number would be easier. Exactly like @gianluca932 suggested. Or perhaps not overloading this function.

@timdorr
Copy link
Member

timdorr commented Jul 19, 2024

The overload doesn't accept the options argument, which is intentional. We don't want to give the impression you can "go back" but also alter the location. That is contradictory and usually not even allowed by the browser (so you can't read cross-domain information). A type union is not appropriate for the first argument.

dgdavid added a commit to agama-project/agama that referenced this issue Sep 11, 2024
Mainly to avoid the tricky workaround for the ReactRouterDom#navigate
overloading. See remix-run/react-router#10505 (comment)
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

4 participants