use useHref'ed href for router based navigation

[jprosevear]

Closes #5476 (comment)

This make require a lot of setup for testing, but generally speaking relative links in routers don't work because the useHref'ed value is not used for navigation.

Originally investigated in heroui-inc/heroui

✅ 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:

https://stackblitz.com/edit/wfdcu9f2-5zvxtaag?file=App.jsx has a comparison of the React Router link and RA link behavior for the relative link. Go to /blogs in the preview

It is also worth noting that the http:// link href is wrong as well, perhaps the href return value in linkProps should also be guarded with shouldClientNavigate somehow

🧢 Your Project:

https://www.rhino-project.org/ , https://www.heroui.com/ (I'm not affiliated with them, but they are a user of this lib where this first showed up)

[jprosevear]

routerLinkProps.href is the useHref value, so that relative links are computed correctly. For instance if you are under /blogs in React Router, useHref will compute "/blogs/1" if href="1"

[jprosevear]

An alternative might be to call it with e.currentTarget.href

[jprosevear]

The test failures make sense broadly in that its not expecting to use the full path from useHref for navigation.

expect(navigate).toHaveBeenCalledWith('/base/foo', {foo: 'bar'});

would be the fix

[jprosevear]

@devongovett i think you wrote the related code originally?

[snowystinger]

Thanks for the PR, you'll need to sign the CLA https://react-spectrum.adobe.com/contribute.html#contributor-license-agreement
Also, if you could update the tests so they pass, it'd help with the review.

Thanks again!

[jprosevear]

Thanks for the PR, you'll need to sign the CLA https://react-spectrum.adobe.com/contribute.html#contributor-license-agreement Also, if you could update the tests so they pass, it'd help with the review.

That's not a problem, the key question is if the updated behaviour is correct or is the current behaviour "as designed" or something.

Also how to potentially handle host based links like https://example.com which are quasi-broken even now afaict.

[jprosevear]

Ok, I went back to first creating stories that I believe demonstrate the problem for better discussion.

First, you can see that the href prop on the link is incorrect (useHref constructs ).

react-spectrum/packages/@react-aria/link/src/useLink.ts

Line 70 in a861ca4

linkProps: mergeProps(domProps, routerLinkProps, {

merges the useHref outcome into the props and that becomes the href prop on the link.

Second, the next_nested link has the correct href attribute because routerProps.href is merge, but the onclick handler uses the original href (props.href == "next_nested" at

react-spectrum/packages/@react-aria/link/src/useLink.ts

Line 93 in a861ca4

router.open(e.currentTarget, e, props.href, props.routerOptions);

) instead of the instead of the useHref value ("nested/next_nested")

So the correct behavior should be:

• if the navigation will be done by the router use routerProps.href as the path to router.open AND pass routerProps.href as href from useLink
• if the navigation will not be done by the router, pass props.href back as href from useLink

the trick is how to detect navigation will be done by the router onside the onClick handler since shouldClientNavigate requires the dom element (however, perhaps parsing props.href and seeing if location.origin exists would be enough?)

@snowystinger @devongovett

[jprosevear]

Another alternative would be to add an isExternal flag on the Link component

[devongovett]

This is a breaking change. Shouldn't the router be handling this in the navigate function? That should do the same resolution as useHref.

[jprosevear]

This is a breaking change. Shouldn't the router be handling this in the navigate function? That should do the same resolution as useHref.

Great question:

        <a
          href="blogs/14"
          onClick={(e) => {
            e.preventDefault();
            navigate('blogs/14');
          }}
        >
          direct nav link
        </a>

works fine, I'm not sure why it fails with react-aria.

[devongovett]

I think it might depend on where the RouterProvider is rendered in the tree. In the examples here, useNavigate() is called outside the Route component, so it won't know the nested route context. However, useHref is passed down and is called inside the link component, so it does know the nested route context. Hence the potential mismatch.

You could fix this by calling useNavigate and rendering RouterProvider inside each <Route>, i.e. inside your page component, instead of outside the whole router.