diff --git a/packages/react-router/src/link.tsx b/packages/react-router/src/link.tsx index eca94ab56f..0116b1893c 100644 --- a/packages/react-router/src/link.tsx +++ b/packages/react-router/src/link.tsx @@ -13,8 +13,6 @@ import { import { isServer } from '@tanstack/router-core/isServer' import { useRouter } from './useRouter' -import { useForwardedRef } from './utils' - import { useHydrated } from './ClientOnly' import type { ActiveOptions, @@ -68,8 +66,8 @@ function preloadLink(router: AnyRouter, options: unknown) { }) } -function compareLinkState(a: LinkState, b: LinkState) { - return a[0] === b[0] && a[1] === b[1] +const LINK_SELECTOR_OPTIONS = { + compare: (a: LinkState, b: LinkState) => a[0] === b[0] && a[1] === b[1], } function resolveExternalLink( @@ -201,12 +199,31 @@ function useLinkPropsFor< // 3. In client bundles, `isServer` is `false`, so the early return never executes // ========================================================================== + // The link's own ref: the element for the viewport observer and the key + // of a pending intent timer. A forwarded ref is filled alongside it by one + // callback, memoized on the forwarded ref so React re-attaches it (and + // notifies the consumer) only when their ref changes, not on every render. + // A cleanup returned by a consumer callback is passed through to React. + // eslint-disable-next-line react-hooks/rules-of-hooks + const innerRef = React.useRef(null) // eslint-disable-next-line react-hooks/rules-of-hooks - const innerRef = useForwardedRef(forwardedRef) + const mergedRef = React.useCallback( + (element: Element | null) => { + innerRef.current = element + if (typeof forwardedRef === 'function') { + return forwardedRef(element) + } + if (forwardedRef) { + forwardedRef.current = element + } + return undefined + }, + [forwardedRef], + ) const { activeOptions, - to: toOption, + to, preload: userPreload, preloadDelay: userPreloadDelay, hashScrollIntoView, @@ -223,8 +240,7 @@ function useLinkPropsFor< onMouseEnter, onMouseLeave, onTouchStart, - } = options - const to = toOption as string | undefined + } = options as typeof options & { to?: string } // eslint-disable-next-line react-hooks/rules-of-hooks const isHydrated = useHydrated() @@ -300,7 +316,7 @@ function useLinkPropsFor< const [href, isActive] = useSelector( router.stores.location, selectLinkState, - { compare: compareLinkState }, + LINK_SELECTOR_OPTIONS, ) const externalLink = isActive === undefined ? href : undefined const linkDisabled = disabled || href === undefined @@ -320,18 +336,10 @@ function useLinkPropsFor< // eslint-disable-next-line react-hooks/rules-of-hooks const enqueuePreload = React.useCallback( (e?: React.MouseEvent | React.FocusEvent | IntersectionObserverEntry) => { - if (!e) { - cancelPreload(innerRef) - return - } - - if ( - !( - (e as IntersectionObserverEntry).isIntersecting ?? - preload === 'intent' - ) - ) { - if ((e as IntersectionObserverEntry).isIntersecting === false) { + const isIntersecting = (e as IntersectionObserverEntry | undefined) + ?.isIntersecting + if (!(isIntersecting ?? preload === 'intent')) { + if (isIntersecting === false) { cancelPreload(innerRef) } return @@ -379,12 +387,12 @@ function useLinkPropsFor< } return () => { observer?.disconnect() - enqueuePreload() + cancelPreload(innerRef) } }, [router, _options, preload, enqueuePreload, innerRef]) const props = collectElementProps(options, host) - props.ref = innerRef + props.ref = forwardedRef ? mergedRef : innerRef // External links get no router behavior: element props pass through as given. if (externalLink) { props.href = externalLink @@ -393,11 +401,12 @@ function useLinkPropsFor< // The click handler const handleClick = (e: React.MouseEvent) => { - // Check actual element's target attribute as fallback - const elementTarget = ( - e.currentTarget as HTMLAnchorElement | SVGAElement - ).getAttribute('target') - const effectiveTarget = target !== undefined ? target : elementTarget + // The element's own target attribute is the fallback. + const effectiveTarget = + target ?? + (e.currentTarget as HTMLAnchorElement | SVGAElement).getAttribute( + 'target', + ) if ( !linkDisabled && @@ -522,15 +531,15 @@ function applyLinkState( props.disabled = linkDisabled } props.target = target - // Merge class and style with the state's. Assign only when one side gave a - // value, so links without them do not carry `undefined` keys. + // Merge class and style with the state's. Links without either keep their + // props as given and carry no `undefined` keys. const stateStyle = stateProps.style - if (style !== undefined || stateStyle !== undefined) { + if (style || stateStyle) { props.style = style && stateStyle ? { ...style, ...stateStyle } : style || stateStyle } const stateClassName = stateProps.className - if (className !== undefined || stateClassName !== undefined) { + if (className || stateClassName) { props.className = className ? stateClassName ? `${className} ${stateClassName}` @@ -808,8 +817,8 @@ export function createLink( * @returns An anchor-like element that navigates without full page reloads. * @link https://tanstack.com/router/latest/docs/framework/react/api/router/linkComponent */ -export const Link: LinkComponent<'a'> = React.forwardRef( - (props, ref) => { +export const Link: LinkComponent<'a'> = React.memo( + React.forwardRef((props, ref) => { const host = props._asChild || 'a' const linkProps = useLinkPropsFor(props as any, ref, host) @@ -821,9 +830,39 @@ export const Link: LinkComponent<'a'> = React.forwardRef( : props.children return React.createElement(host, linkProps, children) - }, + }), + areLinkPropsEqual, ) as any +// A Link's output depends only on its props, the router context and the +// location store, which React tracks for memoized components, so a parent +// re-render with equal props can skip it. Router options are compared by +// value: destinations are usually inline object literals. Element props +// (`children`, handlers, `style`, ...) are compared by reference only, since +// they may hold arbitrary (even cyclic) data. +function areLinkPropsEqual( + prev: Record, + next: Record, +): boolean { + let extraKeys = 0 + for (const key in next) { + extraKeys++ + if (prev[key] === next[key]) { + continue + } + if ( + !ROUTER_OPTION_KEYS.has(key) || + !deepEqual(prev[key], next[key], { ignoreUndefined: false }) + ) { + return false + } + } + for (const _key in prev) { + extraKeys-- + } + return extraKeys === 0 +} + export type LinkOptionsFnOptions< TOptions, TComp, diff --git a/packages/react-router/src/utils.ts b/packages/react-router/src/utils.ts index f3a0351d6e..7887761993 100644 --- a/packages/react-router/src/utils.ts +++ b/packages/react-router/src/utils.ts @@ -57,22 +57,3 @@ export function usePrevious(value: T): T | null { // return the previous value only return ref.current.prev } - -/** - * React hook to take a `React.ForwardedRef` and returns a `ref` that can be used on a DOM element. - * - * @param ref - The forwarded ref - * @returns The inner ref returned by `useRef` - * @example - * ```tsx - * const MyComponent = React.forwardRef((props, ref) => { - * const innerRef = useForwardedRef(ref) - * return
- * }) - * ``` - */ -export function useForwardedRef(ref?: React.ForwardedRef) { - const innerRef = React.useRef(null) - React.useImperativeHandle(ref, () => innerRef.current!, []) - return innerRef -} diff --git a/packages/react-router/tests/link-memo.test.tsx b/packages/react-router/tests/link-memo.test.tsx new file mode 100644 index 0000000000..873bbf9e7a --- /dev/null +++ b/packages/react-router/tests/link-memo.test.tsx @@ -0,0 +1,175 @@ +import React from 'react' +import { act, cleanup, render } from '@testing-library/react' +import { afterEach, expect, test, vi } from 'vitest' +import { + Link, + RouterContextProvider, + createLink, + createMemoryHistory, + createRootRoute, + createRoute, + createRouter, +} from '../src' + +afterEach(cleanup) + +async function createPostsRouter() { + const root = createRootRoute() + const router = createRouter({ + routeTree: root.addChildren([ + createRoute({ getParentRoute: () => root, path: '/posts/$postId' }), + ]), + history: createMemoryHistory({ initialEntries: ['/posts/1'] }), + }) + await router.load() + return router +} + +test('a parent re-render with equal props does not re-render the Link', async () => { + let hostRenders = 0 + const Host = React.forwardRef>( + (props, ref) => { + hostRenders++ + return + }, + ) + const CustomLink = createLink(Host) + const router = await createPostsRouter() + + let rerender = () => {} + let setPostId: React.Dispatch> = () => {} + function Parent() { + const [, setTick] = React.useState(0) + const [postId, set] = React.useState(1) + rerender = () => setTick((tick) => tick + 1) + setPostId = set + return ( + + + Post + + + ) + } + const view = render() + const anchor = view.getByText('Post') + expect(anchor).toHaveAttribute('href', '/posts/1') + expect(anchor).toHaveClass('current') + const rendersAfterMount = hostRenders + + // Same props, new inline objects: the memoized Link is skipped. + act(() => rerender()) + expect(hostRenders).toBe(rendersAfterMount) + + // A changed destination renders again. + act(() => setPostId(2)) + expect(hostRenders).toBe(rendersAfterMount + 1) + expect(anchor).toHaveAttribute('href', '/posts/2') + expect(anchor).not.toHaveClass('current') +}) + +test('element props are compared by reference: new children re-render, a cyclic prop does not throw', async () => { + let hostRenders = 0 + const Host = React.forwardRef< + HTMLAnchorElement, + React.ComponentProps<'a'> & { payload?: object } + >(({ payload: _payload, ...props }, ref) => { + hostRenders++ + return + }) + const CustomLink = createLink(Host) + const router = await createPostsRouter() + + let rerender = () => {} + function Parent() { + const [, setTick] = React.useState(0) + rerender = () => setTick((tick) => tick + 1) + // A new cyclic object per render: deep-comparing it would never return. + const cyclic: Record = {} + cyclic.self = cyclic + return ( + + + Post + + + ) + } + render() + expect(hostRenders).toBe(1) + + // Equal router options, but `children` is a new element and `payload` a + // new object, so the Link renders again instead of comparing them deeply. + act(() => rerender()) + expect(hostRenders).toBe(2) +}) + +test('a forwarded callback ref is notified once per element, not per Link render', async () => { + const router = await createPostsRouter() + const ref = vi.fn<(element: HTMLAnchorElement | null) => void>() + + const view = render( + + + Post + + , + ) + const anchor = view.getByText('Post') + expect(ref.mock.calls).toEqual([[anchor]]) + expect(anchor).toHaveAttribute('data-status', 'active') + + // A location change re-renders the Link (it is no longer active) without + // detaching and re-attaching the consumer's ref. + await act(() => + router.navigate({ to: '/posts/$postId', params: { postId: '2' } }), + ) + expect(anchor).not.toHaveAttribute('data-status') + expect(ref).toHaveBeenCalledTimes(1) + + // A different ref is attached to the same element; the old one is released. + const next = vi.fn<(element: HTMLAnchorElement | null) => void>() + view.rerender( + + + Post + + , + ) + expect(ref.mock.calls).toEqual([[anchor], [null]]) + expect(next.mock.calls).toEqual([[anchor]]) + + view.unmount() + expect(next.mock.calls).toEqual([[anchor], [null]]) +}) + +test('a cleanup returned by a forwarded callback ref runs on unmount', async () => { + const router = await createPostsRouter() + const cleanup = vi.fn() + const ref = vi.fn((_element: HTMLAnchorElement | null) => cleanup) + + const view = render( + + + Post + + , + ) + const anchor = view.getByText('Post') + expect(ref.mock.calls).toEqual([[anchor]]) + expect(cleanup).not.toHaveBeenCalled() + + view.unmount() + // React runs the cleanup instead of calling the ref with `null`. + expect(cleanup).toHaveBeenCalledTimes(1) + expect(ref).toHaveBeenCalledTimes(1) +})