[Bug]: useNavigate's NavigateFunction is mixing async and non-async return types

[kylsgl]

What version of React Router are you using?

7.0.1

Steps to Reproduce

Initialize navigate

const navigate = useNavigate();

Expected Behavior

The navigate function return type should be either:

void

or

Promise<void>

This would make it clear whether we need to handle a promise or not, making the behavior of navigate more predictable.

Actual Behavior

navigate return type is

void | Promise<void>

[dorei44]

use this:

Promise.resolve(navigate('/')).catch(console.log)

[DimaAmega]

@kylsgl Check changelog: https://github.com/remix-run/react-router/blob/main/CHANGELOG.md#exposed-router-promises

[jgarplind]

Hey @timdorr! Would you mind sharing the rationale behind closing? Not saying it's wrong in any way, just interested to know what you consider to be the solution. Is it one of the suggestions mentioned above? Or was there a code update?

[MarcusCaspeco]

use this:

Promise.resolve(navigate('/')).catch(console.log)

Is this actually the way we're supposed to invoke navigate now? @timdorr

It seems more ideomatic to make this return a Promise regardless if it ends up being a synchronous operation or not (just Promise.resolve()), that way we can at least handle invoking this.

[dorei44]

@timdorr can you reopen this issue? React.use is not compatible with navigate

[MarcusCaspeco]

@brookslybrand Is the team aware of this type issue? This issue was closed but I don't think a good solution was presented

[brophdawg11]

The tricky part about useNavigate is that it can return both void or Promise<void> so the type is accurate. It forks logic internally based on whether or not you're using a data router so that we can provide a stable navigate function reference when using a data router.

The data router version returns the promise from router.navigate while the non-data-router version is a synchronous function because there's no async data loading step.

Changing the return type to Promise<void> would mean making the non-data-router version unnecessarily async and would also likely constitute a breaking change.

I reopened this so we can figure out the best recommendation for usage with use now that 19 is out. For now, I would wrap it in app code and cast the return type if you're using a data router. Something like:

function useNavigatePromise(...args) {
  return useNavigate(...args) as (args: Parameters<NavigateFunction>) => Promise<void>
}

[Philipp91]

use(navigate()); isn't the only case where this break TS checks on existing code. I also have a couple situations like:

const myHelper = (): void => navigate(...);  // tsc error: can't cast Promise<void> to void

navigate(...);  // ESLint error: Don't ignore the floating promise.

---

It forks logic internally

FTR that happens here. Why are they called "stable" and "unstable"?

One solution could be to expose both of those functions separately, for users who know what they're getting. More concretely, have useNavigatePromise() and useNavigateVoid() available in the library, but it starts with asserting isDataRoute has the right value and then calls the right internal function variant. And perhaps this requires the NavigateFunction type to be forked too, and useNavigate() would return a union type.

Changing the return type to Promise would mean making the non-data-router version unnecessarily async

Is it thinkable that this becomes a Promise in the future too, when the non-data-router version also supports data loading or for other reasons? (I have no idea whether this question even makes sense to ask.)

Would doing an extra Promise.resolve() in the library hurt (performance or so)?

and would also likely constitute a breaking change.

I guess it would be the same amount of breaking as the change to void | Promise<void> was?

[timdorr]

FTR that happens here. Why are they called "stable" and "unstable"?

Because the architecture of BrowserRouter (et. al) necessitates that when the location changes, the result of useNavigate will change. In the data router form, we can produce a "stable" version of useNavigate that won't change when the location changes and won't trigger lots of renders for components that don't actually depend on the location.

[Philipp91]

I see, thanks for the explanation!

So the stable/unstable situation is conceptually unrelated to the void/promise situation, just in today's world it happens to be fully correlated. That naming is fine for those two internal functions, but perhaps not worth adopting when exposing two different functions (I guess?), if that's even a reasonable solution in the first place (if so, I proposed two other names above).

[IanVS]

What is the current recommendation from the react-router team? I guess we need a matrix of: React 18/19 and data router / non-data-router. The documentation at https://reactrouter.com/start/library/navigating#usenavigate seems inadequate now that it's a bit more complex of a situation in v7.

[Corrob]

Do we really expect everyone calling navigate to use Promise.resolve(navigate('/')).catch(console.error) everywhere? Seems to be a very poor API... Can we expose a useNavigateNoPromise() that does that on every call automatically?

[harshithmohan]

Do we really expect everyone calling navigate to use Promise.resolve(navigate('/')).catch(console.error) everywhere? Seems to be a very poor API... Can we expose a useNavigateNoPromise() that does that on every call automatically?

I'm just using a custom hook for now but yeah, this should be handled by the library itself.

import { useNavigate } from 'react-router';
import type { NavigateOptions, To } from 'react-router';

type NavigateFunctionVoid = {
  (to: To, options?: NavigateOptions): void;
  (delta: number): void;
};

const useNavigateVoid = () => useNavigate() as NavigateFunctionVoid;

export default useNavigateVoid;

[gromaco]

To avoid floating-promises error, for now, we are using void when calling navigate:

const navigate = useNavigate()

const takeMeToTheNextPage = () {
    void navigate('nextPage');
}

But would be awesome to see the recommended way from the react-router team.

[sdavids]

navigate('/');

triggers @typescript-eslint/no-floating-promises.

The proposed workaround

void navigate('/');

triggers no-void.