[core] Use router specific Link components - #4661
Conversation
bharatkashyap
added
the
internal
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.
| */ | ||
|
|
||
| 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.
| 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
| 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
| }, [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.
| */ | ||
|
|
||
| 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?
| pathname, | ||
| searchParams, | ||
| navigate, | ||
| Link, |
There was a problem hiding this comment.
I believe this needs to handle the history prop?
Janpot
left a comment
Janpot
left a comment
There was a problem hiding this comment.
Looks great now. Maybe some tests?
I think we can add effective tests for this only if we set up and run an actual app (avoid mocking), so I think we can take in a follow-up to this? I'll merge this and create an item for myself to do that |
3 participants
Uh oh!
There was an error while loading. Please reload this page.