[core] Use router specific Link components #4661
Conversation
bharatkashyap
commented
Feb 5, 2025
•
bharatkashyap
commented
- Fixes Basename Not Included in href for Links in Left Navigation #4556
- Fixes Navigation links with locale #4579
Netlify deploy preview |
| import { LinkProps as ReactRouterLinkProps } from 'react-router'; | ||
| import { LinkProps as NextLinkProps } from 'next/link'; |
There was a problem hiding this comment.
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.
| @@ -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'> { | |||
There was a problem hiding this comment.
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.
| } | ||
|
|
||
| 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; |
There was a problem hiding this comment.
| 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.
…ui-toolpad into fix/router-specific-link
| /** | ||
| * Abstract router used by Toolpad components. | ||
| */ | ||
| export interface Router { | ||
| pathname: string; | ||
| searchParams: URLSearchParams; | ||
| navigate: Navigate; | ||
| Link?: React.JSXElementConstructor<LinkProps>; |
There was a problem hiding this comment.
| Link?: React.JSXElementConstructor<LinkProps>; | |
| Link?: React.ComponentType<LinkProps>; |
I don't think we want to allow for say 'div' or something
| @@ -28,7 +29,11 @@ export const Link = React.forwardRef(function Link( | |||
| }; | |||
| }, [routerContext, onClick, history]); | |||
|
|
|||
| return ( | |||
| return routerContext?.Link && href ? ( | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
| @@ -5,12 +5,13 @@ import { RouterContext } from './context'; | |||
| * @ignore - internal component. | |||
| */ | |||
|
|
|||
| export interface LinkProps extends React.AnchorHTMLAttributes<HTMLAnchorElement> { | |||
| export interface DefaultLinkProps extends React.AnchorHTMLAttributes<HTMLAnchorElement> { | |||
There was a problem hiding this comment.
Can't this just be the LinkProps that we use both for DefaultLink and Link?
| export interface DefaultLinkProps extends React.AnchorHTMLAttributes<HTMLAnchorElement> { | |
| export interface LinkProps extends React.AnchorHTMLAttributes<HTMLAnchorElement> { |
| import { LinkProps } from '../shared/Link'; | ||
|
|
||
| const Link = React.forwardRef<HTMLAnchorElement, LinkProps>((props, ref) => { | ||
| const { href, ...rest } = props; |
There was a problem hiding this comment.
I believe this should also handle the history prop?
| @@ -41,6 +42,7 @@ export function NextAppProviderPages(props: AppProviderProps) { | |||
| pathname, | |||
| searchParams, | |||
| navigate, | |||
| Link, | |||
There was a problem hiding this comment.
I believe this needs to handle the history prop?
