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]: Relative navigation issue while navigating from "index" route: ".." doesn't works, I must use "../.." to go back #8350

Closed
Gryndii opened this issue Nov 17, 2021 · 21 comments · Fixed by #8954, #8697 or #8985
Labels

Comments

@Gryndii
Copy link

Gryndii commented Nov 17, 2021

What version of React Router are you using?

6.0.2

Steps to Reproduce

<Some other routes>
....
<Route path='/pages'>
    <Route index element={<Pages />} />

    <Route path=':pageId' element={<Item />} />              
  </Route>
  

Navigation via '..' from Pages with "index" doesn't works but it is ok for Item
I should use '../..' for the same result as '..'

Expected Behavior

Should navigate level up via '..' from index route
And it would be nice to have option with nested routes that is used only to create url hierarchy for relative navigation usage, but without children and parrents ui rendering

Actual Behavior

doesnt navigate from index page via '..'

@Gryndii Gryndii added the bug label Nov 17, 2021
@Gryndii Gryndii changed the title [Bug]: Relative navigation issue while navigating from "index" route: ".." doesn't works, I must us "../.." to go back [Bug]: Relative navigation issue while navigating from "index" route: ".." doesn't works, I must use "../.." to go back Nov 18, 2021
@maxandriani
Copy link

I'm struggling w/ the same issue, but I think the root cause is because the "base" url should contains a trailing slash, otherwise the history dependency will compute the relative path wrong.

The problems appears to be in the resolve-pathname.js -> resolve-pathname -> history -> react-router dependency.

In the line 30, the algorithm just consider everything in the same level as a filename string w/o check if it is really a filename string (file.ext).

image

@mrlika
Copy link

mrlika commented Dec 1, 2021

I think it is because .. means navigate one <Route> up in the routes tree and not one path segment (directory) up. Consider this example:

function NavigateUp() {
  return <Navigate to='..' />;
}

<Route path='/test'>
  <Route path=':a/:b/:c' element={<NavigateUp />} />
  <Route path=':a' element={<NavigateUp />} />
</Route>

Both /test/a/b/c and /test/a are redirected to /test because one route up in the tree is <Route path='/test'> that is really weird. I can't find this in documentation.

@gowthamvbhat
Copy link
Contributor

Both /test/a/b/c and /test/a are redirected to /test that is really weird. I can't find this in documentation.

@mrlika This is discussed here - upgrading to v6.

@mrlika
Copy link

mrlika commented Dec 1, 2021

@gowthamvbhat, thanks for pointing out that. As for me, it is better to write unlike <a href> that goes up by path segments.

@mrlika
Copy link

mrlika commented Dec 1, 2021

The workaround is to use resolvePath that has no relation to <Route> tree:

const navigate = useNavigate();
navigate(resolvePath('..', window.location.pathname));

// or

<Link to={resolvePath('..', window.location.pathname)} />

// or

<Navigate to={resolvePath('..', window.location.pathname)} />

@Huulivoide
Copy link

Seems that a route match gets generated for both the parent route and the nested index route.
Matches array of RouteContext:

(5) [{…}, {…}, {…}, {…}, {…}]
0: {params: {…}, pathname: '/', pathnameBase: '/', route: {…}}
1: {params: {…}, pathname: '/person', pathnameBase: '/person', route: {…}}
2: {params: {…}, pathname: '/person/203876', pathnameBase: '/person/203876', route: {…}}
3:
    params: {personId: '203876'}
    pathname: "/person/203876/items"
    pathnameBase: "/person/203876/items"
    route: {caseSensitive: undefined, element: {…}, index: undefined, path: 'items', children: Array(4)}
4:
    params: {personId: '203876'}
    pathname: "/person/203876/items/"
    pathnameBase: "/person/203876/items/"
    route: {caseSensitive: undefined, element: {…}, index: true, path: undefined, children: undefined}

Reproducible demo https://stackblitz.com/edit/github-1iyzlr?file=src%2FApp.tsx

Thus navigating to .. in my case, would pop the index page out of the match stack, and then take the user to that address. But that ends up being the link to the nesting "parent" page

@Huulivoide
Copy link

It seems that this is as designed
They have tests in place ensuring that to=".." navigates back to the same page in index route.

I guess that technically this is "correct" if the documentation says that .. traverses up by one "Route".

Tough I feel like the index routes should maybe be a special case to the rule.

@shulcsm
Copy link

shulcsm commented Jan 24, 2022

This is REALLY annoying with index routes and used to work in beta.

Imagine scenario

<Route path=":id/">
  <Route index element={<Link to="../../sibling" />} />
  ....
</Route>

@Romcol
Copy link

Romcol commented Jan 28, 2022

I have also experienced the bug. This is also a breaking change coming from reach router. It should be fixed or specified in the doc :)

@JaffParker
Copy link
Contributor

JaffParker commented Mar 2, 2022

I experience this problem and it makes kind of no sense how these work. I actually expect it to go up a route in the tree as opposed to just removing one segment (although in this particular instance it's the same). Here are my routes:

<Route path="/settings">
  <Route index element={<SettingsRoute />} />
  <Route
    path="account"
    element={<SettingsRoute page="account" />}
  />
  <Route path="team">
    <Route index element={<SettingsRoute page="team" />} />
    <Route
      path="create"
      element={<CreateUserRoute />}
    />
    <Route path=":id">
      <Route index element={<EditUserRoute />} />
      <Route
        path="history"
        element={<UserActivityHistoryRoute />}
      />
    </Route>
  </Route>
</Route>

Here's the list of routes I have with the desired to=".." behavior:

/settings -> nothing, it's the root
/settings/account -> go back to /settings, works
/settings/team -> go back to /settings, does not work since the index' parent route is path="team", which is silly
/settings/team/create -> go back to /settings/team, works
/settings/team/:id -> go back to /settings/team, does not work since the index' parent route is path=":id", again silly
/settings/team/:id/history -> go back to /settings/team/:id, works

So the silly behavior comes from how the index routes are handled - they're treated as children of the parent route instead of being treated as the parent route itself.

Basically what I'm saying is, I think to=".." inside index routes should be treated as to="../.." for proper predictability. Otherwise, there's no workaround for this right now since I can't just render the element in the parent route, since that'll render it for all child routes; and I can't move everything a level up since that'll break navigation for the rest of the child routes.

Upd: I realize that's what everyone is saying, kinda got lost in the point while trying to make it. But the conclusion stands - index routes should ignore one parent in the tree when going back.

@ryanflorence
Copy link
Member

Whew ... this is a tough one but I get it.

The question is:

  1. do we remain consistent that .. means "up the route hierarchy"
  2. or do we add special cases for index and pathless layout routes?

I think everybody is right that (2) is probably better. So the explanation would be

".." means up the route hierarchy, ignoring routes without paths

Seems easy enough to explain and both index and pathless layout routes don't have any paths.

So the silly behavior...

What's the point of life if we can't be silly sometimes?

@ryanflorence
Copy link
Member

Trouble is it's a breaking change technically :\ Gonna see if there's a way to make it opt-in.

@kiliman
Copy link
Contributor

kiliman commented May 27, 2022

I think .. should look at the URL, not necessarily the route structure. As that matches how the browser would treat relative links. I believe index routes should always end with /, otherwise the browser treats it as a file instead of a folder which breaks relative links.

@JaffParker
Copy link
Contributor

@kiliman that would definitely be a breaking change. I actually based my app's entire navigation on the fact that .. goes back according to the route structure lol

@ryanflorence
Copy link
Member

ryanflorence commented Jun 7, 2022

@kiliman I wish I had a link or something to send to you, but we labored over that behavior for months (literally) with dozens of scenarios and are confident traversing the route hierarchy is preferred.

The only question here is do we skip routes that don't add any path segments.

@kiliman
Copy link
Contributor

kiliman commented Jun 7, 2022

EDIT: fixed typo and added more examples.

Ha no problem. I would probably just create a helper function then to navigate relative paths. Trailing backslash means you're in a folder. Non-trailing means current route is a "file".

. always refers to current folder and .. refers to parent folder

/a/b/c/  . = /a/b/c/   .. = /a/b/
/a/b/c   . = /a/b/     .. = /a/
/a/b/c   ./d = /a/b/d  ../d = /a/d

It even handles search params properly. If you provide them, it will overwrite existing search for current location.

function getRelativePath(location: Location, relativePath: string) {
  const base = new URL(`http://dummy${location.pathname}${location.search}`)
  const relative = new URL(relativePath, base)
  return `${relative.pathname}${relative.search}`
}

// path = /parent/child/?a=1
const location = useLocation()
const relative = getRelativePath(location, '..?b=2') 
// relative = /parent/?b=2

@brophdawg11
Copy link
Contributor

brophdawg11 commented Jun 8, 2022

This has been fixed in #8697 / #8954. Routes that do not provide any path should not contribute to relative pathing logic - should be released in 6.4.0

Update: One more case handled in #8985

@brophdawg11 brophdawg11 self-assigned this Jun 8, 2022
@brophdawg11 brophdawg11 linked a pull request Jun 16, 2022 that will close this issue
@brophdawg11
Copy link
Contributor

This is fixed in the above mentioned PRs - should be included in the next 6.4.0 prerelease 👍

@brophdawg11 brophdawg11 removed their assignment Jun 22, 2022
@kirkobyte
Copy link

kirkobyte commented Oct 17, 2022

Is there any way/workaround to receive the preivous behaviour? I've got a use-case where the previous behaviour was really beneficial around optional props.

In V5 we had a route with an optional parameter (options/:option?), which we now render like this:

<Route path="options">
  <Route index element={<ListPage />}/>
  <Route path=":option" element={<ListPage/>}/>
 </Route>

We were hoping that in ListPage we could define our routes with a simple <Link to="../<option id>">Option</Link>. But since index routes are ignored, the routing ends up leaving options.

@brophdawg11
Copy link
Contributor

In your case I would probably just use the presence of the option param to drive the to:

let params = useParams();
let to = params.option ? ".." : ".";
...
<Link to={to}>Option</Link>

We have been chatting a bit about optional params recently and I think they'll be on the roadmap once we finish getting Remix on top of React Router 6.4, so keep an eye on #8381 as we'll post any updates over there if/when we move forward on that front.

@andi0b
Copy link

andi0b commented Nov 28, 2022

EDIT: I think people who were confused with needing to use "../.." with index routes, were expected to use path based routing. So maybe it's better to revert to the old behavior, but switch to path based routing by default.

@kirkobyte, @brophdawg11: I stumbled into exactly the same issue. I have a module and all my components link relative from there. But once I use an index route, all links break. Which means index routes can't be really used anymore in a meaningful way, because they break relative links without any way of fixing it (there isn't even an opt-in switch on the Route). This new "fixed" behavior is highly illogical for me.

This "new feature" already cost me hours of my productivity, because it's really hard to figure out what "parent route" really means. Route should have a flag like transparent, or canBeParent which specifies if it should participate in relative routing.

<Route path="module">
  <Route index element={<FeatureA/>}/> {/* which breaks all the links, no way to solve it, except handling it inside feature a */}
  <Route path="feature-a" element={<FeatureA/>}/>
  <Route path="feature-b" element={<FeatureB/>}/>
  {...}
 </Route>

ugly workaround:

<Route path="module">
  <Route index element={<Naviage to='./feature-a' />}/>
  <Route path="feature-a" element={<FeatureA/>}/>
  <Route path="feature-b" element={<FeatureB/>}/>
  {...}
 </Route>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment