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
3 changes: 3 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 @@ -2,6 +2,8 @@
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';
Copy link
Member

Choose a reason for hiding this comment

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

This seems incorrect because:

  • the package should never import from third party runtimes
  • I don't think the implementations are 100% compatible

We need to define an abstract LinkProps type ourselves, and then make sure in the routeradapter to map the concrete implementation to the abstract properties.

import { NotificationsProvider } from '../useNotifications';
import { DialogsProvider } from '../useDialogs';
import {
Expand All @@ -27,6 +29,7 @@ export interface Router {
pathname: string;
searchParams: URLSearchParams;
navigate: Navigate;
Link?: React.JSXElementConstructor<NextLinkProps & ReactRouterLinkProps>;
}

export interface Branding {
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
3 changes: 2 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,6 +1,6 @@
'use client';
import * as React from 'react';
import { useSearchParams, useLocation, useNavigate } from 'react-router';
import { useSearchParams, useLocation, useNavigate, Link } from 'react-router';
import { AppProvider, type AppProviderProps, Navigate, Router } from '../AppProvider/AppProvider';

function ReactRouterAppProvider(props: AppProviderProps) {
Expand All @@ -26,6 +26,7 @@ function ReactRouterAppProvider(props: AppProviderProps) {
pathname,
searchParams,
navigate: navigateImpl,
Link,
}),
[pathname, searchParams, navigateImpl],
);
Expand Down
6 changes: 5 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 @@ -28,7 +28,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} to={href} {...rest} onClick={onClick}>
{children}
</routerContext.Link>
) : (
<a ref={ref} href={href} {...rest} onClick={handleLinkClick}>
{children}
</a>
Expand Down