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

[v6] [Bug]: React Router is too greedy. /routetypo matches /route instead of the * fallback #7972

Closed
eakl opened this issue Aug 23, 2021 · 5 comments

Comments

@eakl
Copy link

eakl commented Aug 23, 2021

What version of React Router are you using?

6.0.0-beta.2

Steps to Reproduce

https://codesandbox.io/s/react-router-routing-j72xo?file=/src/App.js

Expected Behavior

Route path parameter is confusing

<Routes>
  <Route path="/" element={<p>Root</p>} />
  <Route path="home">
    <Route path="/" element={<p>Home</p>} />
    <Route path="*" element={<Navigate to={"."} />} />
  </Route>
  <Route path="*" element={<p>Not Found.</p>} />
</Routes>

/ should match Root
/home should look to its children and match Home
/hometypo should match Not Found
/home/wrongParam should match <Navigate ... />

Actual Behavior

/ matches Root (OK)
/home looks to its children and matches Home (OK)
/hometypo matched Home (NOT OK, RR is too greedy)
/home/wrongParam redirects to <Navigate /> (OK but the redirect path is inconsistent with <Route path />)

Referring to #7335, I agree that <Route path /> should implement a . for the root of the subcomponent instead of the forward slash.

<Route path="home">
  <Route path="/" element={<p>Home</p>} />
  <Route path="*" element={<Navigate to={"."} />} />
</Route>

When having <Route path /> and <Navigate to /> side by side, it becomes confusing to match the root of the parent route with a forward slash instead of a .

@openscript
Copy link

I'm not 100% sure but I think I run into a similar problem. I recently updated to 6.0.0-beta.2 and my <Link>s pointed to the wrong path.

<Routes>
  <ProtectedRoute {...defaultProtectedRouteProps} path="employee/" element={<EmployeeLayout />}>
  <Route path="system/synchronization/*" element={<SystemSynchronization />} />
  </ProtectedRoute>
</Routes>

<SystemSynchronization />:

<Routes>
  <Route path="" element={<DefaultSystemSynchronization />} />
  <Route path=":limit/:page" element={<DefaultSystemSynchronization />} />
</Routes>

<DefaultSystemSynchronization /> and 6.0.0-beta.2 at localhost/employee/system/synchronization:

<Link to={'/10/0'}> -> localhost/10/0
<Link to={'10/0'}> -> localhost/employee/system/synchronization/10/0

Subsequently clicking on <Link to={'10/0'}> stacks up to localhost/employee/system/synchronization/10/0/10/0.....


<DefaultSystemSynchronization /> and 6.0.0-beta.1 at localhost/employee/system/synchronization:

<Link to={'/10/0'}> -> localhost/employee/system/synchronization/10/0 without stacking on subsequent clicks
<Link to={'10/0'}> -> localhost/employee/system/synchronization/10/0 with stacking on subsequent clicks


I reverted to 6.0.0-beta.1 as I couldn't find a way to achieve the routing I wanted with beta.2.

@chaance chaance changed the title [Bug][v6]: React Router is too greedy. /routetypo matches /route instead of the * fallback [v6] [Bug]: React Router is too greedy. /routetypo matches /route instead of the * fallback Sep 4, 2021
@mjackson
Copy link
Member

Thanks for bringing this to our attention, @eakl. You're right, * is too greedy. I have a fix incoming for that one shortly.

I agree that should implement a . for the root of the subcomponent instead of the forward slash.

In beta.4 we stopped using path="/" for index routes and use the <Route index> prop instead. We decided not to go with <Route path="."> because dots are not valid in route paths. For example, <Route path=".."> doesn't make any sense. Also, <Route index> communicates more clearly that it's an index route.

As for the <Navigate to=".">, that's a slightly different concept. The to value is a destination, not a route path. But I agree with you that the . should refer to the route's path and not the path of its parent route. A fix for that will be incoming shortly as well.

@openscript I don't think your issue is related. In v6, all relative <Link to> values build on the current URL. This is why you see the "stacking" you mentioned when you use relative links. If you're still confused, please open a separate issue with an example we can reproduce.

mjackson added a commit that referenced this issue Sep 17, 2021
This prevents `*` from matching without a preceding `/` in the URL
pathname. So e.g. `/users/*` matches `/users/mj` but not `/userstypo`.

This also fixes `to="."` in splat routes so they point to the route
path *including* the portion of the URL that was matched by the `*`,
which also makes `*` more consistent with `:param` since it's treated
just the same as any other param.

There is however a subtle breaking change if you are using the low-level
`match` API, e.g. the `match` object you get back from `matchRoutes()`
or `useMatch()`. `match.pathname` in a splat route now includes the full
URL pathname that was matched instead of only the portion before the
`*`. There is a new variable, `match.pathnameStart` that you can use if
you needed this for doing your own route matching.

Fixes #7972
@eakl
Copy link
Author

eakl commented Sep 18, 2021

Thanks @mjackson for the fixes

mjackson added a commit that referenced this issue Sep 20, 2021
This prevents `*` from matching without a preceding `/` in the URL
pathname. So e.g. `/users/*` matches `/users/mj` but not `/userstypo`.

This also fixes `to="."` in splat routes so they point to the route
path *including* the portion of the URL that was matched by the `*`,
which also makes `*` more consistent with `:param` since it's treated
just the same as any other param.

There is however a subtle breaking change if you are using the low-level
`match` API, e.g. the `match` object you get back from `matchRoutes()`
or `useMatch()`. `match.pathname` in a splat route now includes the full
URL pathname that was matched instead of only the portion before the
`*`. There is a new variable, `match.pathnameStart` that you can use if
you needed this for doing your own route matching.

Fixes #7972
mjackson added a commit that referenced this issue Sep 20, 2021
This prevents `*` from matching without a preceding `/` in the URL
pathname. So e.g. `/users/*` matches `/users/mj` but not `/userstypo`.

This also fixes `to="."` in splat routes so they point to the route
path *including* the portion of the URL that was matched by the `*`,
which also makes `*` more consistent with `:param` since it's treated
just the same as any other param.

There is however a subtle breaking change if you are using the low-level
`match` API, e.g. the `match` object you get back from `matchRoutes()`
or `useMatch()`. `match.pathname` in a splat route now includes the full
URL pathname that was matched instead of only the portion before the
`*`. There is a new variable, `match.pathnameStart` that you can use if
you needed this for doing your own route matching.

Fixes #7972
@mjackson
Copy link
Member

@julioflima
Copy link

This version breaks the nested layout.

Steps to Reproduce:

  1. Access: https://codesandbox.io/s/react-router-routing-forked-7n9ch?file=/src/App.js
  2. Test it, see that it's working.
  3. Change the version in package.json to "react-router-dom": "6.0.0-beta.5".
  4. Test it again, to see that the nested navigation isn't working anymore.

Follow in below the main structure of code.

    <Router>
        <Link to="/home">Home</Link>
        <Link to="/home/detail">Home - Detail</Link>
        <Link to="/home/wrongParam">Wrong Id</Link>

        <Routes>
          <Route
            path="/*"
            element={
              <div>
                <h1>Root</h1>
                <Routes>
                  <Route path="/" element={<h2>Welcome!</h2>} />
                  <Route path="home/">
                    <Route path="" element={<h2>Home</h2>} />
                    <Route path="detail" element={<p>Home / Detail</p>} />
                  </Route>
                  <Route path="*" element={<h2>Not Found.</h2>} />
                </Routes>
              </div>
            }
          />
        </Routes>
      </Router>

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

No branches or pull requests

4 participants