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.

Sign up for GitHub

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

Support basePath for relative href prop #248

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Support basePath for relative href prop #248

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
da89d17
Support basePath for href prop
upreeti Dec 16, 2024
739eeec
no basePath for external links
upreeti Dec 16, 2024
158922c
rename external to absolute
upreeti Dec 16, 2024
e89e61a
match links with no protocol
upreeti Dec 16, 2024
2ac3ec2
linting
upreeti Dec 16, 2024
4c10b28
add changeset
upreeti Dec 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/happy-fireants-visit.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"react-resource-router": patch
---

Support basePath for relative href prop
4 changes: 2 additions & 2 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 5 additions & 1 deletion src/ui/link/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,8 +72,12 @@ const Link = forwardRef<HTMLButtonElement | HTMLAnchorElement, LinkProps>(
)) ||
''
: to;
const IS_ABSOLUTE_LINK_REGEX = /^((?:(http|https):\/\/)|\/\/)/;
Copy link
Collaborator

@liamqma liamqma Dec 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The code change itself looks good. However, I wonder if href should be prefixed.
For to, it makes sense because to must be one of routes.
But for href, it's basically any URL. I know that you have the check whether href is a relative URL. If yes, then prefix. But should we prefix it at the product code?

const { getBasePath } = useRouterActions();
const basePath = getBasePath();

const href = `${getBasePath}/something`;

<Link href={href} />

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it too many places we need to prefix if it's done at the product level?

Copy link
Author

@upreeti upreeti Dec 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

yes, that's why we wanted to have it built into the code here since it would be more scalable and if we did have it at the product level there would be too many places to prefix. We are also using the MenuLinkItem component (which uses Link downstream), and it doesn't take a to prop (only href)

we have this change a patch fix right now, but I thought it might be useful to upstream

const staticBasePath =
href == null && typeof to === 'string' ? routeAttributes.basePath : '';
(href != null && !IS_ABSOLUTE_LINK_REGEX.test(href)) ||
typeof to === 'string'
? routeAttributes.basePath
: '';

const triggerPrefetch = useCallback(() => {
// ignore if async route not ready yet
Expand Down
42 changes: 42 additions & 0 deletions src/ui/link/test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,48 @@ describe('<Link />', () => {
);
});

it('should push history with correct link for relative href when given basePath', () => {
renderInRouter(
'my link',
{
href: '/route/1?foo=bar',
},
'/base'
);
expect(screen.getByRole('link', { name: 'my link' })).toHaveAttribute(
'href',
'/base/route/1?foo=bar'
);
});

it('should not add basePath for absolute links', () => {
renderInRouter(
'my link',
{
href: 'https://www.atlassian.com/',
},
'/base'
);
expect(screen.getByRole('link', { name: 'my link' })).toHaveAttribute(
'href',
'https://www.atlassian.com/'
);
});

it('should not add basePath for absolute links with no protocol specified', () => {
renderInRouter(
'my link',
{
href: '//www.atlassian.com/',
},
'/base'
);
expect(screen.getByRole('link', { name: 'my link' })).toHaveAttribute(
'href',
'//www.atlassian.com/'
);
});

it('should pass props to the child element', () => {
renderInRouter('my link', {
...defaultProps,
Expand Down
Loading