fix: relative navigation from index routes

[JaffParker]

This fixes the following issue: #8350

Quick example, copied from packages/react-router/__tests__/navigate-test.tsx. With a router like this:

<MemoryRouter initialEntries={["/home"]}>
  <Routes>
    <Route path="home" element={<Navigate to="../about" />} />
    <Route path="about" element={<h1>About</h1>} />
  </Routes>
</MemoryRouter>

Relative navigation works as expected. However, with introduction of index routes such as this:

<MemoryRouter initialEntries={["/home"]}>
  <Routes>
    <Route path="home">
      <Route index element={<Navigate to="../about" />} />
    </Route>
    <Route path="about" element={<h1>About</h1>} />
  </Routes>
</MemoryRouter>

It breaks since <Route path="home"> becomes the index route's parent. This PR addresses that by making relative '..' hrefs from index routes go an extra level up.

I could've probably done it better by rearranging some arguments in the touched functions, but I wanted to minimize my footprint.

Possible breaking change, not a minor release. Even myself I adjusted my components that were affected by this and will need to know when this releases to remove workarounds.

[remix-cla-bot]

Hi @JaffParker,

Welcome, and thank you for contributing to React Router!

Before we consider your pull request, we ask that you sign our Contributor License Agreement (CLA). We require this only once.

You may review the CLA and sign it by adding your name to contributors.yml.

Once the CLA is signed, the CLA Signed label will be added to the pull request.

If you have already signed the CLA and received this response in error, or if you have any questions, please contact us at [email protected].

Thanks!

- The Remix team

[remix-cla-bot]

Thank you for signing the Contributor License Agreement. Let's get this merged! 🥳

[JaffParker]

Possibly needed to consider: how do layout routes affect relative nav. Is it each parent or only parent routes with a match that count? I didn't check that

[brophdawg11]

Thanks for the PR @JaffParker! I agree this feels like a breaking change so will need @ryanflorence and/or @mjackson to weigh in on how to best incorporate this behavior.

I wanted to give you a heads up we just merged some large changes for 6.4.0 into dev which caused some conflicts in here, would you mind updating when you have a moment? The useNavigate hook moved down into packages/react-router/lib/hooks.tsx and resolveTo now lives in packages/router/utils.ts

[JaffParker]

Hey, @brophdawg11 will do!

[ryanflorence]

I'm thumbs up on fixing this without an "opt-in". I don't consider it a breaking change because the current behavior is meaningless "go up to my parent route and then right back to me" is not a use case.

Let's also make sure pathless routes are skipped as well.

<Route path="/one" element={<One />}>
  <Route element={<Two />}>
    <Route path="three" element={<Navigate to=".." />} />
  </Route>
</Route>

Should go to /one since Two doesn't have a path.

In other words, all routes that don't have paths (index or pathless) should be skipped in all relativity (links and navigations).

[JaffParker]

I fixed the conflict, and also expanded the test to include layout routes as well! So it's confirmed - navigating to .. skips pathless routes in general.

There's also a second commit, some tests were crashing until I fixed those paths.

[Janpot]

I don't consider it a breaking change because the current behavior is meaningless "go up to my parent route and then right back to me" is not a use case.

I don't care for an opt-in, just want to mention that it's breaking for people that worked around the issue using:

<Route index element={<Navigate to="../.." />} />

[brophdawg11]

We don't want to update these paths - they need to point to the local copy of ./react-router-dom which is put there during the build - the reasoning is explained in the comment above these exports

[brophdawg11]

Actually - I see your note about the failing tests now. That may just be because you hadn't run yarn build locally which copies these files into place. I think you can just git revert 71e8b7f to undo the pathing changes here, then yarn build && yarn test and see if that fixes it 👍

[brophdawg11]

Same here - need to use the local copy of react-router-dom

[brophdawg11]

Let's remove KutnerUri which is already in here above - likely a merge conflict issue

[ryanflorence]

@Janpot thanks for the input, we'll definitely call it out in the release notes. Every bugfix is a breaking change for somebody 🙃

[brophdawg11]

In other words, all routes that don't have paths (index or pathless) should be skipped in all relativity (links and navigations).

Just to clarify - I believe this also means (as unlikely of a use case as this might be) that the following would be valid as well. This fix as implemented only handles a single level of pathless depth. I'm going to dig into what would be required to support N levels of pathless depth.

<Routes>
  <Route path="/home">
    <Route element={<Outlet />}>
      <Route element={<Outlet />}>
        <Route element={<Outlet />}>
          <Route index element={<Navigate to="../about" />} />
        </Route>
      </Route>
    </Route>
  </Route>
  <Route path="about" element={<h1>About</h1>} />
</Routes>

[JaffParker]

@brophdawg11 good point, it didn't look like that would be possible without too big of a footprint, but I might've missed something. I'll check it out too

[brophdawg11]

It might just involve switching from a boolean (essentially being used as depth of 0/1) to a number to represent the depth. A very quick test looks promising, but I'll throw some more tests at it tomorrow:

// useNavigate()
let pathlessDepth = Math.max(
  [...matches]
    .reverse()
    .findIndex((m) => !m.route.index && m.route.path?.length),
  0
);

// resolveTo()
let routePathnameIndex = routePathnames.length - 1 - pathlessDepth;

[brophdawg11]

Going to merge this into a branch I created and expand on the multi-level aspect above before merging into dev