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

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

[core] Use router specific Link components #4661

Show file tree
Hide file tree
Changes from 1 commit
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
5 changes: 2 additions & 3 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 @@ -2,8 +2,6 @@
import * as React from 'react';
import PropTypes from 'prop-types';
import { createTheme as createMuiTheme, Theme } from '@mui/material/styles';
import { LinkProps as ReactRouterLinkProps } from 'react-router';
import { LinkProps as NextLinkProps } from 'next/link';
import { NotificationsProvider } from '../useNotifications';
import { DialogsProvider } from '../useDialogs';
import {
Expand All @@ -13,6 +11,7 @@ import {
WindowContext,
} from '../shared/context';
import { AppThemeProvider } from './AppThemeProvider';
import { LinkProps } from '../shared/Link';

export interface NavigateOptions {
history?: 'auto' | 'push' | 'replace';
Expand All @@ -29,7 +28,7 @@ export interface Router {
pathname: string;
searchParams: URLSearchParams;
navigate: Navigate;
Link?: React.JSXElementConstructor<NextLinkProps & ReactRouterLinkProps>;
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
8 changes: 5 additions & 3 deletions packages/toolpad-core/src/shared/Link.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,17 @@ import { RouterContext } from './context';
* @ignore - internal component.
*/

export interface LinkProps extends React.AnchorHTMLAttributes<HTMLAnchorElement> {
export interface LinkProps extends Omit<React.AnchorHTMLAttributes<HTMLAnchorElement>, '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.

No need to change this type. The router adapters are responsible for translating this to their concrete implementation. The goal is for us internally have only one standard way to create links, and the router adapter to translate this to a specific implementation.

history?: 'auto' | 'push' | 'replace';
href: string;
to: string;
}

export const Link = React.forwardRef(function Link(
props: LinkProps,
ref: React.ForwardedRef<HTMLAnchorElement>,
) {
const { children, href, onClick, history, ...rest } = props;
const { children, href, onClick, history, to, ...rest } = props;
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
const { children, href, onClick, history, to, ...rest } = props;
const { children, href, onClick, history, ...rest } = props;

We're just passing href internally. Remap correctly to to in the react-router adapter instead.

const routerContext = React.useContext(RouterContext);

const handleLinkClick = React.useMemo(() => {
Expand All @@ -29,7 +31,7 @@ export const Link = React.forwardRef(function Link(
}, [routerContext, onClick, history]);

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} to={href} {...rest} onClick={onClick}>
<routerContext.Link href={href} to={to} {...rest} onClick={onClick}>
{children}
</routerContext.Link>
) : (
Expand Down