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

[core] Use router specific Link components #4661

Merged
merged 11 commits into from
Feb 24, 2025
Merged

[core] Use router specific Link components #4661

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
6b85584
fix: Use router specific Link components
bharatkashyap Feb 5, 2025
31f2bdf
fix: Jan review, possibly CI
bharatkashyap Feb 5, 2025
dd024a8
fix: Jan review 2
bharatkashyap Feb 5, 2025
4f57c42
Merge branch 'master' into fix/router-specific-link
bharatkashyap Feb 5, 2025
c41f976
fix: CI
bharatkashyap Feb 5, 2025
5d4ca97
Merge branch 'fix/router-specific-link' of github.com:bharatkashyap/m…
bharatkashyap Feb 5, 2025
2b2bd2b
fix: Jan review 3
bharatkashyap Feb 5, 2025
c9a92e1
fix: CI
bharatkashyap Feb 5, 2025
c6fee40
fix: CI
bharatkashyap Feb 6, 2025
57d3d22
Merge branch 'master' of github.com:mui/mui-toolpad into fix/router-s…
bharatkashyap Feb 10, 2025
ecbf7e3
fix: Jan review 4
bharatkashyap Feb 10, 2025
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
1 change: 1 addition & 0 deletions docs/data/toolpad/core/components/page-container/CustomPageContainer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ function Content({ router }) {

Content.propTypes = {
router: PropTypes.shape({
Link: PropTypes.elementType,
navigate: PropTypes.func.isRequired,
pathname: PropTypes.string.isRequired,
searchParams: PropTypes.instanceOf(URLSearchParams).isRequired,
Expand Down
7 changes: 7 additions & 0 deletions packages/toolpad-core/src/AppProvider/AppProvider.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,13 +20,20 @@ export interface Navigate {
(url: string | URL, options?: NavigateOptions): void;
}

interface LinkProps {
href: string;
children: React.ReactNode;
onClick?: React.MouseEventHandler<HTMLAnchorElement>;
}

/**
* Abstract router used by Toolpad components.
*/
export interface Router {
pathname: string;
searchParams: URLSearchParams;
navigate: Navigate;
Link?: React.JSXElementConstructor<LinkProps>;
Copy link
Member

@Janpot Janpot Feb 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
Link?: React.JSXElementConstructor<LinkProps>;
Link?: React.ComponentType<LinkProps>;

I don't think we want to allow for say 'div' or something

}

export interface Branding {
Expand Down
2 changes: 2 additions & 0 deletions packages/toolpad-core/src/nextjs/NextAppProvider.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@ vi.mock('next/router', () => ({ useRouter: () => null }));

vi.mock('next/compat/router', () => ({ useRouter: () => null }));

vi.mock('next/link', () => ({ default: () => null }));

interface RouterTestProps {
children: React.ReactNode;
}
Expand Down
2 changes: 2 additions & 0 deletions packages/toolpad-core/src/nextjs/NextAppProviderApp.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import * as React from 'react';
import Link from 'next/link';
import { usePathname, useSearchParams, useRouter } from 'next/navigation';
import { AppProvider } from '../AppProvider';
import type { AppProviderProps, Navigate, Router } from '../AppProvider';
Expand Down Expand Up @@ -29,6 +30,7 @@ export function NextAppProviderApp(props: AppProviderProps) {
pathname,
searchParams,
navigate,
Link,
}),
[pathname, navigate, searchParams],
);
Expand Down
2 changes: 2 additions & 0 deletions packages/toolpad-core/src/nextjs/NextAppProviderPages.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import * as React from 'react';
import Link from 'next/link';
import { asArray } from '@toolpad/utils/collections';
import { useRouter } from 'next/router';
import { AppProvider } from '../AppProvider';
Expand Down Expand Up @@ -41,6 +42,7 @@ export function NextAppProviderPages(props: AppProviderProps) {
pathname,
searchParams,
navigate,
Link,
Copy link
Member

Choose a reason for hiding this comment

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

I believe this needs to handle the history prop?

}),
[navigate, pathname, searchParams],
);
Expand Down
9 changes: 8 additions & 1 deletion packages/toolpad-core/src/react-router/ReactRouterAppProvider.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,13 @@
'use client';
import * as React from 'react';
import { useSearchParams, useLocation, useNavigate } from 'react-router';
import { useSearchParams, useLocation, useNavigate, Link as ReactRouterLink } from 'react-router';
import { AppProvider, type AppProviderProps, Navigate, Router } from '../AppProvider/AppProvider';
import { LinkProps } from '../shared/Link';

const Link = React.forwardRef<HTMLAnchorElement, LinkProps>((props, ref) => {
const { href, ...rest } = props;
Copy link
Member

Choose a reason for hiding this comment

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

I believe this should also handle the history prop?

return <ReactRouterLink ref={ref} to={href} {...rest} />;
});

function ReactRouterAppProvider(props: AppProviderProps) {
const { pathname } = useLocation();
Expand All @@ -26,6 +32,7 @@ function ReactRouterAppProvider(props: AppProviderProps) {
pathname,
searchParams,
navigate: navigateImpl,
Link,
}),
[pathname, searchParams, navigateImpl],
);
Expand Down
7 changes: 6 additions & 1 deletion packages/toolpad-core/src/shared/Link.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import { RouterContext } from './context';

export interface LinkProps extends React.AnchorHTMLAttributes<HTMLAnchorElement> {
history?: 'auto' | 'push' | 'replace';
href: string;
}

export const Link = React.forwardRef(function Link(
Expand All @@ -28,7 +29,11 @@ export const Link = React.forwardRef(function Link(
};
}, [routerContext, onClick, history]);

return (
return routerContext?.Link && href ? (
Copy link
Member

@Janpot Janpot Feb 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

When there is a Link component it should always be used. Maybe it needs to be able to accept an optional href? Or you need to adjust the types of the Link component?

Copy link
Member

@Janpot Janpot Feb 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

also, the Link component that is exported from this module should have the same signature as the one in the routerContext. Ideally the current implementation is renamed to DefaultLink and we export a new version that wraps it:

export const Link = React.forwardRef(function Link(
  props: LinkProps,
  ref: React.ForwardedRef<HTMLAnchorElement>,
) {
  const routerContext = React.useContext(RouterContext);
  const LinkComponent = routerContext?.Link ?? DefaultLink
  return <LinkComponent ref={ref} {...props} />
})

this to avoid unnecessary callback creation and other render logic running in the default link.

<routerContext.Link href={href} onClick={onClick} {...rest}>
{children}
</routerContext.Link>
) : (
<a ref={ref} href={href} {...rest} onClick={handleLinkClick}>
{children}
</a>
Expand Down