remix-run / react-router

Declarative routing for React
https://reactrouter.com
MIT License
53.36k stars 10.34k forks source link

[v6] Link with absolute path doesn't respect basename #7216

Closed morhekil closed 3 years ago

morhekil commented 4 years ago

Version

6.0.0-alpha.2

Test Case

https://codesandbox.io/s/react-router-v6-basepath-1dcnm

Steps to reproduce

Set up <Routes basename="/something">, and inside its component tree use a link with absolute path <Link to="/path">path</Link>

Expected Behavior

Link respects basename specified in its parent Routes, and generates URL <a href="/something/path">

Actual Behavior

Link ignores basename, and generates absolute URL from website root <a href="/path">

Comment

This is the expected behaviour of basename as described in v5 docs - e.g. here, unless it was an intentional change in v6 - was it?

Looking at the code, I suspect the problem is these two lines of resolveLocation: https://github.com/ReactTraining/react-router/blob/dev/packages/react-router/index.js#L766-L767

If toPathname starts with a slash (like in a codesandbox example above) - resolveLocation ignores fromPathname, and as the result - loses basename value that it may have contained.

morhekil commented 4 years ago

If this is a bug indeed, then as a possible solution idea - should basename be added to the RouteContext value, and passed to resolveLocation as the base path to use with absolute URLs?

jeffersonlicet commented 4 years ago

Please let me know if I can help on this. I'll be happy to fix it.

eip1599 commented 4 years ago

Yes, this also happens in React Router 5. I am using nested MemoryRouter with BrowserRouter, though I am not sure if it happens only in MemoryRouter.

It renders to wrong href, though it behaves correctly when clicked.

jeffersonlicet commented 4 years ago

MemoryRouter does not support basename.

On the other hand, maybe we can strip the leading slash from basename? Ok, I think I’m sending a PR fixing this

levymetal commented 4 years ago

Would anyone be able to confirm whether this is an intentional change or a bug? I'm proactively preparing a very large project for v6 and I'm a bit stuck on this - unsure whether I should expect basename to work as it previously did, or if I'm going to need to manually prepend the basename to every Link & navigate etc.

jeffersonlicet commented 4 years ago

Would anyone be able to confirm whether this is an intentional change or a bug? I'm proactively preparing a very large project for v6 and I'm a bit stuck on this - unsure whether I should expect basename to work as it previously did, or if I'm going to need to manually prepend the basename to every Link & navigate etc.

Ìn order to make it work in v6 you should omit the leading slash from basename

morhekil commented 4 years ago

@jeffersonlicet it doesn't work if we're routing inside nested components - it appends basepath to the current path, which results in incorrect URLs

levymetal commented 4 years ago

Yeah it isn't working for me either. Here's the original sandbox with the leading slash removed from basename: https://codesandbox.io/s/react-router-v6-basepath-co3h9. <Link to="/path"> still generates <a href="/path"> instead of <a href="/base/path">

malyzeli commented 4 years ago

Seems like an intentional change to me, did you read latest docs about migrating, especially section about relative routes and trailing slash?

levymetal commented 4 years ago

@malyzeli Yeah I've been through the migration docs. They don't mention anything about the behaviour of basename being different to previous versions. Regarding the trailing slash, the docs specifically state the behaviour is the same regardless of whether the current url has a trailing slash or not so I'm unclear what this has to do with the described issue. Would you be kind enough to elaborate?

Just to clarify with the most concise example I can conjure up:

<Routes basename="/foo/bar">
  <Route path="/baz" element={<Link to="/baz">/baz</Link>} />
</Routes>

The Route with path /baz matches /foo/bar/baz because of the basename on Routes. The Link with the exact same path as the Route ignores the basename and generates an anchor to /baz. This is different to previous versions of React Router where both the Route and the Link would be prepended with the basename.

If this is intentional then perhaps basename should be removed altogether to remove confusion.

morhekil commented 4 years ago

I would add to @levymetal example above that from my understanding, the intent behind basename is to support the same application deployed under different base URLs. Taking that /for/bar example above, I may have the same app deployed as https://example.com/foo/bar (no basepath), or https://example2.com/home/foo/bar (basepath /home), or https://example3.com/deep/route/foo/bar (basepath /deep/route).

In v5 one could define appropriate basepath value for these deployment (e.g. pass it down as configuration parameter), and have the app working correctly completely oblivious to the fact that it's deployed at the root URL.

In v6 the only workable approach so far seems to be passing basepath down to all components in the app, and prefixing all relevant routes/link like <Link to={${basepath}/baz} /> - which works, but it's very noisy and looks like a step back from v5.

I'm happy to PR a fix, but I was hoping to get some confirmation from react-router team here that this is indeed a bug, and/or an thumbs up on the approach to the fix outlined in my comment above

malyzeli commented 4 years ago

@levymetal my fault - somehow I missed @morhekil is talking about basename, thought it's about path of nested Routes!

Configured basename only on a Router component, didn't know it was moved to Routes - is that mentioned anywhere in docs?

morhekil commented 4 years ago

@malyzeli

Configured basename only on a Router component, didn't know it was moved to Routes - is that mentioned anywhere in docs?

it's been mentioned in another issue here

omasback commented 4 years ago

Bumping this since there's been two releases since and it seems to still not be addressed. Old basename would automatically prepend it to Link paths. Preferable that would stay the same IMO.

kentcdodds commented 4 years ago

Just confirming that this issue is still valid in v6.0.0-alpha.5: https://codesandbox.io/s/react-router-v6-basepath-1dcnm

mmrath commented 4 years ago

This is still a problem in the recent beta

benneq commented 4 years ago

The same issue is present with the Navigate component (using 6.0.0-beta.0):

<Routes basename={process.env.PUBLIC_URL}>
        <Route path="/" element={<Navigate to="/home" replace />} />        
        <Route path="/home/*" element={<Home />} />
</Routes>

When directly opening http://localhost:3000/my-public-url-path/home it works fine. But when opening http://localhost:3000/my-public-url-path/ I get redirected to http://localhost:3000/home.

bogdansoare commented 4 years ago

This issue still exists

owen2345 commented 4 years ago

Having the same issue. Please review this issue, it is an important feature of the library.

stale[bot] commented 4 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. You can add the fresh label to prevent me from taking any action.

morhekil commented 4 years ago

a bump to keep the stale bot away

DavidWells commented 4 years ago

Ran into this issue as well. Any thoughts on merging fix https://github.com/ReactTraining/react-router/pull/7462?

caseyjhol commented 4 years ago

It would certainly be great to hear the devs' thoughts regarding this issue. basename functionality has changed from v5:

Some additional examples using the latest versions:

malyzeli commented 4 years ago

I'm trying understand what is the reason for providing basename on Routes component while having no basename support on (Browser)Router. Definitely not saying it's wrong, just that I'm unable to see the use case it's probably intended for.

I would like to know what is the recommended solution/workaround when we need to deploy React app under non-root path on the server?

Explicitly prefixing all Link/Navigate paths in the codebase seems wrong.

renatobenks commented 4 years ago

what you mean with non-root paths @malyzeli? like navigating to external domains? 🤔

levymetal commented 4 years ago

@renatobenks They mean in a subdirectory, eg foo.com/bar instead of foo.com. In previous versions of RR it's easy to run RR out of a subdirectory; all you need to do is set the basename in one location. No other parts of the codebase need to be aware of the url structure. However, in v6, basename only works for routes but not links, requiring them to be prepended with the basename. This means every link in the application needs knowledge of the basename which results in more complexity, verbosity, and/or tighter coupling to the base.

It seems the overwhelming majority prefer the behaviour of previous versions because they're much easier to set up with subdirectories, which a lot of people are doing. Also, it doesn't make sense how basename works for routes but not links. Either it should be a configuration that works for both or it should be completely removed.

I'm only repeating what has already been discussed so apologies to all who have notifications turned on.

malyzeli commented 3 years ago

Seems to me that for now the best workaround would be cloning the source, manually applying fix from PR here, then keep using this custom patched version until it's either resolved in official package or until we get some explanation from maintainers - possibly suggesting alternative solutions if basename is not meant to be included in v6 (though I'm convinced that wouldn't happen without prior notice of deprecation).

ivanjonas commented 3 years ago

Currently, the migration guide says that we merely need to move the basename prop from <Router> to its child <Route>s. It even makes it sound easy: "This is a simple change of moving the prop."

Is this true? If true, this is minimally disruptive. But from all the upheaval in this thread/issue, the migration guide might be glossing over some nasty details.

jflarosa commented 3 years ago

I confirm the problem. In a micro front architecture it is very annoying

caub commented 3 years ago

I don't know yet if I like this change, basically if I've a route with a link to another route

with react-router@5 you'd just:

<BrowserRouter basename="/app">
  <Switch>
    <Route path="/foo" exact render={() => <>foo! <Link to="/bar"></Link></>}>
    <Route path="/bar" exact render={()=>'bar!'} />
  </Switch>
</BrowserRouter>

and the link to "/bar" would translate to "/app/bar" as expected

now with react-router@6 it seems you need to do:

<BrowserRouter>
  <Route path="/app">
    <Route path="/foo" element={<>foo! <Link to="../bar"></Link></>}>
    <Route path="/bar" element={'bar!'} />
  </Route>
</BrowserRouter>

where you need to play with relative paths all the way

This is super annoying if a component is reused and displayed from different path levels, and simply impractical overall, I prefer dealing with absolute paths, they're more explicit
The idea of course if to avoid repeating "/app" prefix over and over (in my case it would be dynamically set)

I could try to make my own Link and useNavigate components that will prefix "/app" to a link like "/bar", but why doesn't react-router@6 provides this? It seems so important

So :+1: for this issue description

taranvohra commented 3 years ago

7462 has a fix for this but is awaiting merge approval.

rediska1114 commented 3 years ago

While we await PR merge, I suggest we consider using custom Link and Navigate components where we'll add basename

example:

import React from 'react'
import { To } from 'history'
import { Link as RLink, LinkProps, Navigate as RNavigate } from 'react-router-dom'
import { NavigateProps } from 'react-router'

const basename = process.env.PUBLIC_URL

function removeLeadingSlash(path: string) {
    return path.replace(/^\//, '')
}

function getAbsolutePath(path: string) {
    // skip relative paths
    if (!path.startsWith('/')) return path

    return ['/', removeLeadingSlash(basename), '/', removeLeadingSlash(path)].join('')
}

function convertTo(to: To): To {
    if (typeof to === 'string') return getAbsolutePath(to)

    return { ...to, pathname: to.pathname ? getAbsolutePath(to.pathname) : to.pathname }
}

export const Link: React.FC<LinkProps & React.RefAttributes<HTMLAnchorElement>> = props => {
    const to = convertTo(props.to)
    return <RLink {...props} to={to} />
}

export const Navigate: React.FC<NavigateProps> = props => {
    const to = convertTo(props.to)
    return <RNavigate {...props} to={to} />
}
mortargrind commented 3 years ago

BTW You can use the patch-package to patch the react-router module locally after npm install as an alternative to forking this repo, patching & maintaining that fork, until the https://github.com/ReactTraining/react-router/pull/7462 is approved and merged.

chaance commented 3 years ago

Fixed in https://github.com/ReactTraining/react-router/pull/7462. New beta release coming this week!

kentcdodds commented 3 years ago

Heck yes!

jeffersonlicet commented 3 years ago

❤️

labriola commented 3 years ago

Unless I am misunderstanding it, I think this fix causes a different issue. In an app with multiple deeply nested routes that are built via the useRoutes hooks,

We see something like this: basename = basename ? joinPaths([parentPathname, basename]) : parentPathname;

Nested useRoutes set the basename based on the parent's path name. And then when you navigate, it now uses that base name, meaning that, if I am not mistaken, a call to navigate() has absolutely no ability to 'break out' of the most nested route in which it is found, as it will always be relative to the context's basename

I do agree that, in general, navigation being relative to the basename (for the purposes discussed above) is a good thing but how would you approach this scenario as it seems like a trap?

apolakipso commented 3 years ago

a call to navigate() has absolutely no ability to 'break out' of the most nested route in which it is found, as it will always be relative to the context's basename

I've already experienced this issue before the beta.1 I believe, the only way to break out of the nested route when using <Link/> or navigate() is to use ../ to move up the path. I wrote a hook useAbsoluteTo that rewrites links starting with // to be "actually absolute" (replacing // with multiple ../ to move up to the basename root of the app. This looks and feels stupid, but at least I can wrap the <RouterLink> in my own <Link> and create links that work for the whole app.

I haven't seen any reasoning in the documentation so far that would explain why we shouldn't have "really" absolute routes in nested routes (or if so, a proper way to then deal with the trap you described). This seems silly. It's nice to have "absolute" links in nested routes, but not being able to navigate out of the nested route is something that doesn't make any sense to me at all. I don't understand how anyone is currently using nested routes in v6 with this behavior...

Note: I used a hook because I accessed the basename from my AppContext, so this isn't strictly necessary

labriola commented 3 years ago

I wrote a hook useAbsoluteTo that rewrites links starting with // to be "actually absolute" (replacing // with multiple ../ to move up to the basename root of the app.

I took a similar approach that I can use unless I am missing a more proper solution. I have a usePathNavigate() which makes / absolute to the top level basename and decided on a unix-y convention of using ~/ to mean 'local to my basename'. At least to me, when I see a navigate('/foo') in the app, I like knowing its from the app root regardless of the location from which it is invoked.

apolakipso commented 3 years ago

a unix-y convention of using ~/ to mean 'local to my basename'.

Hmm, I like using the ~/ convention, but to me it'd make more sense the other way round – treating ~/ as the root of the nested route I'm in (as if the nested route was a Unix user with their own user folder).

That would result in all my links differing from v6's approach (absolute links starting with / while v6 requires ../../../, and relative ones starting with ~/ while v6 uses /, but I need a readable solution that makes sense to me, and theirs currently doesn't).

I might go with ~/ as root for nested routes and / for absolute ones, from all the versions I've seen and tried, that reads best to me. Thanks for sharing, @labriola !

quoctienkt commented 1 year ago

Issue still occurs in react router dom latest version! 6.18.0

Ella-Ksenofontova commented 5 months ago

I confirm the words of user above. When I was trying to refactor my code by deleting explicit paths, an error occured. BTW, I use the latest version - 6.24.0