Skip to content

Basename is not used in createHref #3839

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

Closed
jochenberger opened this issue Sep 14, 2016 · 18 comments
Closed

Basename is not used in createHref #3839

jochenberger opened this issue Sep 14, 2016 · 18 comments
Labels
Milestone

Comments

@jochenberger
Copy link
Contributor

I use a basename in my router setup. In one of my components I have a Link to="/". The href attribute points to http://${hostname}/ instead of http://${hostname}/${contextPath}. The click handler works fine though.

@timdorr
Copy link
Member

timdorr commented Sep 14, 2016

Pass it in as a prop to BrowserRouter or HashRouter.

@timdorr timdorr closed this as completed Sep 14, 2016
@jochenberger
Copy link
Contributor Author

I'm doing that, but it seems to be ignored for the href.

@timdorr
Copy link
Member

timdorr commented Sep 14, 2016

I'll double-check when I'm at a computer, but it's got tests at least: http://github.com/ReactTraining/react-history/commit/b61ae2c362a34fe6545b24f5f06c77d23ab050b7

@jochenberger
Copy link
Contributor Author

jochenberger commented Sep 14, 2016

I do about this:

<Router basename='/foo'>
  <Match exactly pattern="/" render={()=><Link to="/">Index</Link>} />
</Router>

The link renders as <a href="/"> but I expect it to render as as <a href="/foo">. The click handler works fine though, so it's more of a cosmetic issue.

@chentsulin
Copy link
Contributor

chentsulin commented Sep 14, 2016

My comments in #3843: #3843 (comment)

Those tests didn't cover Link's href.

@timdorr
Copy link
Member

timdorr commented Sep 14, 2016

@chentsulin

It seems that history is no longer responsible for creating hrefs. See remix-run/history#365 (comment)

And Link component just use router.createHref to create hrefs, so miss the basename: https://github.com/ReactTraining/react-router/blob/v4/modules/Link.js#L91

Ah, shazbot. Well, that's a little confusing as to who should be responsible then? Let me see what the thought process is on that and we can either re-open that PR or integrate the ideas in a more clear API. I feel like history should be aware of it, but we'll see if that stops at react-history or not.

Either way, thanks for this issue!

@timdorr timdorr reopened this Sep 14, 2016
@timdorr timdorr changed the title [v4] basename missing from Link href Basename is not used in createHref Sep 14, 2016
@timdorr timdorr added this to the v4.0.0 milestone Sep 14, 2016
@mjackson
Copy link
Member

mjackson commented Sep 14, 2016

Just to clarify where responsibilities are: history has no knowledge of queries, so router is responsible for creating hrefs. It does know about basenames, but all locations you get in the history.listen callback already have properly-truncated pathnames.

EDIT: Likewise, all paths that you pass to functions like history.push should not have basenames in them.

@timdorr
Copy link
Member

timdorr commented Sep 15, 2016

Started a fix over in #3858. Need to do the latter part of what @mjackson said.

@jochenberger
Copy link
Contributor Author

jochenberger commented Sep 15, 2016

Now I'm confused. Consider the following:

<Router basename='/foo'>
  ...
  <Link to={"bar"} /> // renders `href='/foo/bar'` but transitions to `/foobar` [sic]
  <Link to={"/bar"} /> // renders `href='/bar'` but transitions to `/foo/bar`
  ...
</Router>

I'd say they should both behave the same, i.e. render href='/foo/bar' and transition to /foo/bar.

@timdorr
Copy link
Member

timdorr commented Sep 15, 2016

@jochenberger That is not how it should end up behaving. This area of the code should be considered "in progress".

@jochenberger
Copy link
Contributor Author

Yes, I thought so, that's why I let you know. ;-)

@timdorr timdorr added the bug label Sep 16, 2016
@mjackson
Copy link
Member

I'm looking into this today.

@nibblesnbits
Copy link

Is there any progress on this? I've noticed that the behavior for this in the 4.0.0-alpha.4 package is incorrect. The docs reflect this as well.

basename

The base URL for all locations. If your app is served from a sub-directory on your server, you’ll want to set this to the sub-directory.

<HashRouter basename="/calendar" />

// now links like this:
<Link to="/today"/>
// will generate links with an href to "#/calendar/today"

That last comment is the issue. If my application exists at http://example.com/calendar/, then what should be generated is a link to /calendar/#/today, not #/calendar/today. As it stands now, the actual behavior is as documented.

@pshrmn
Copy link
Contributor

pshrmn commented Oct 23, 2016

@nibblesnbits If your app exists at http://example.com/calendar/, then a link within your app to #/today is a link to http://example.com/calendar/#/today, no basename is necessary.

Basenames are used to change the root url (location of /). Everything before the root doesn't matter to RR. For the <HashRouter>, #/ is the root url, so http://example.com/calendar/ doesn't matter.

@patotoma
Copy link

Hey @pshrmn, basename is required for that /calendar thing because it is a subdirectory and not the actual root of the webserver.

@ryanflorence
Copy link
Member

07725d7 fixes this? let me know if it doesn't and we'll reopen.

@patotoma
Copy link

Hello @ryanflorence, sorry for a stupid question but how can I try this thing out? Is it already merged and published on npm? I found that this was merged a month ago but I experienced these issues a few days ago so in that case this commit wasnt helpful. Thanks in advance.

@nibblesnbits
Copy link

@patotoma, the easiest way I know of is to npm link from the repo.

  1. Download the repo, and npm install its dependencies.
  2. npm link the package so that you can use it from other projects on that machine.
  3. npm install the version of react-router that you linked in you project and test as needed.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

8 participants