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

Add support for router base path #5395

Closed
wants to merge 11 commits into from
Closed

Conversation

mskelton
Copy link

@mskelton mskelton commented Nov 9, 2023

Closes #5335

✅ Pull Request Checklist:

  • Included link to corresponding React Spectrum GitHub Issue.
  • Added/updated unit tests and storybook for this change (for new code or code which already has tests).
  • Filled out test instructions.
  • Updated documentation (if it already exists for this component).
  • Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

  • Add a basePath prop to the RouterProvider
  • The href should include the base path, but the base path should be removed when calling the navigate function.

🧢 Your Project:

@mskelton mskelton closed this Nov 9, 2023
@mskelton mskelton reopened this Nov 9, 2023
@mskelton mskelton marked this pull request as ready for review November 9, 2023 17:00
reidbarber
reidbarber previously approved these changes Nov 9, 2023
Copy link
Member

@reidbarber reidbarber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@ktabors ktabors left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution! There is one change relating to how we RSP does imports.

packages/@react-aria/link/src/useLink.ts Outdated Show resolved Hide resolved
packages/@react-aria/menu/src/useMenuItem.ts Outdated Show resolved Hide resolved
@devongovett
Copy link
Member

devongovett commented Nov 9, 2023

Is this something that React Aria needs to be aware of? Perhaps it would be better to use React Router's API to solve this problem instead? Their useHref hook should handle this.

import {useHref} from 'react-router-dom';

<MenuItem href={useHref('...')} />

I would prefer this approach because it keeps React Aria closer to native HTML behavior, and agnostic to any particular router.

@mskelton
Copy link
Author

Basename behavior is not unique to react-router. Next.js also supports setting a basename as does TanStack router. The naming of it might be updated to be basePath perhaps which might make more sense.

The useHref example you gave doesn't really work, as it will produce the full href including the basename which is perfect for our anchor tag href, but using that same value to pass to navigate now causes an issue. If you have a router with a basename of /foo and then call navigate('/foo/bar') you end up navigating to /foo/foo/bar. Routing libraries handle these details internally in their own Link components to set the href to the full URL including the prefix, but when doing client side navigation, it doesn't re-apply the basename so that both behave properly.

With react-aria taking over the responsibility of creating the anchor tag, this creates a problem since react-aria doesn't know about the basename, so it's using the same href for both calling navigate as well as applying the href attribute. This results in either the client side navigations failing, or the href being incorrect.

Technically your suggestion does work if you also wrap the navigate function to take the value react-aria called it with and strip the basename off of it. This is what I had to do to make it work without our component library for the time being, but this involves extra manual steps for every implementation, and is very easy to miss until you start opening links in a new tab.

The same actually goes for the href passed to any react-aria components as it would be very easy to forget to add useHref to a component resulting in some working and others not. And again, this is very error prone as even with a patched navigate function that is passed to react-aria, I could leave omit useHref in my component and client side navigation would continue to work while the href attribute would be incorrect.

I also think the behavior would be very confusing as it would vary between react-aria managed links and those that aren't. If you use the Link component directly from your router, you just pass the href without the basename, but if you use a link from your component library which internally is using react-aria, now you need to use useHref or add the basename to the href.

import { Link } from 'react-router-dom'
import { MyLibraryLink } from '@/components/MyLibraryLink'

<Link to="/admin" />
<MyLibraryLink href={useHref("/admin")} />

Sorry, this comment got pretty long, late night brain dumping my thoughts on this 🤣

@mskelton mskelton changed the title Add support for router basename Add support for router base path Nov 14, 2023
@ktabors
Copy link
Member

ktabors commented Nov 14, 2023

It looks like the PR has a failing test. Are you able to fix that?

@mskelton
Copy link
Author

@ktabors Thanks, I updated to basePath instead of basename and forgot to update the test.

ktabors
ktabors previously approved these changes Nov 16, 2023
@devongovett
Copy link
Member

I wonder if you've tried to use the HTML <base> element for this? Since the client side router already expects hrefs relative to the base path, only the native href needs the base path added (e.g. for open in new tab functionality). The <base> element should handle this, since it adds a base path to all native links on the page, and the client side router should already work as is.

Let me know if you try that out. If it works, I'd prefer to add that to the docs rather than adding additional functionality to append/strip the url ourselves.

@mskelton
Copy link
Author

@devongovett I did some experimenting with <base> and unfortunately that introduces more issues than it solves:

In-page anchors (e.g. <a href="#foo" />) are considered relative and thus the <base> prefix is applied. If using in-page anchors (which is by no means uncommon, I do this quite often), you would then be forced to use an absolute path to prevent the base path from being applied, or at the very least apply the full path to the current route excluding the base path and ensuring it's relative so the base path is applied. The same applies with query string links such as <a href="?foo=bar" />

React Router (and I presume other routes as well) will transform links from relative to absolute paths. As such, nothing passed to the <Link> component from react-router will ever use the <base> path since the rendered anchor tags will always have an absolute path.

However, the same would not be true of react-aria. If you have <Link href="/bar"> and <base href="/foo/" /> the resulting url is /bar. However, <Link href="bar"> would result in /foo/bar. This difference is subtle, but quite confusing.

@GustavoSmith
Copy link

Technically your suggestion does work if you also wrap the navigate function to take the value react-aria called it with and strip the basename off of it. This is what I had to do to make it work without our component library for the time being, but this involves extra manual steps for every implementation, and is very easy to miss until you start opening links in a new tab.

The same actually goes for the href passed to any react-aria components as it would be very easy to forget to add useHref to a component resulting in some working and others not. And again, this is very error prone as even with a patched navigate function that is passed to react-aria, I could leave omit useHref in my component and client side navigation would continue to work while the href attribute would be incorrect.

@mskelton Did you make it work with NextJS or with React Router? I've been facing the same issue on NextJS and I couldn't figure out a workaround while I wait for this to merge (and I don't know how much it'll take @reidbarber to review it). I've seen a related one but for RR, so if in your case it's for NextJS and you could provide it, it'll be awesome. Thank you!

@mskelton
Copy link
Author

@GustavoSmith The workaround I used was to manually add logic that would apply the base path to all links I pass to react-aria components before rendering them and then use a function to wrap the router.push or similar method to strip off the base URL. It's pretty nasty, but it works in the meantime while we wait for this PR to be merged.

@ktabors
Copy link
Member

ktabors commented Jan 18, 2024

Thanks for providing details on your workaround. We're considering a solution like what is described in this comment on another issue #5476 (comment). We believe it would address your issue as well.

@mskelton mskelton deleted the basename branch April 3, 2024 02:20
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

Successfully merging this pull request may close these issues.

Links inside RouterProvider with basename have inconsistent href handling
5 participants