diff --git a/.changeset/olive-donkeys-shave.md b/.changeset/olive-donkeys-shave.md new file mode 100644 index 0000000000..158e99b7d3 --- /dev/null +++ b/.changeset/olive-donkeys-shave.md @@ -0,0 +1,5 @@ +--- +'@tanstack/react-router': patch +--- + +bail out of `Link` re-renders when the resolved href and active state are unchanged diff --git a/packages/react-router/src/link.tsx b/packages/react-router/src/link.tsx index 5347a52680..0ce9b25c94 100644 --- a/packages/react-router/src/link.tsx +++ b/packages/react-router/src/link.tsx @@ -19,9 +19,11 @@ import { useForwardedRef, useIntersectionObserver } from './utils' import { useHydrated } from './ClientOnly' import type { + ActiveOptions, AnyRouter, Constrain, LinkOptions, + ParsedLocation, RegisteredRouter, RoutePaths, } from '@tanstack/router-core' @@ -31,6 +33,114 @@ import type { ValidateLinkOptionsArray, } from './typePrimitives' +type LinkState = [ + href: string | undefined, + externalLink: string | undefined, + isActive: boolean, +] + +// Keep a referentially stable value while the contents are equal. Links +// routinely pass inline `params` / `search` object literals, which would +// otherwise change `_options` identity on every parent render, rebuild the +// store selector, and discard its memoized selection. +// +// `ignoreUndefined: false` is required: an explicit `undefined` clears an +// inherited param or search key, so `{}` and `{ category: undefined }` build +// different locations and must not be treated as equal here. +function useValueStable(value: T): T { + const ref = React.useRef(value) + // `deepEqual` short-circuits on reference equality, so this covers both cases. + if (!deepEqual(ref.current, value, { ignoreUndefined: false })) { + ref.current = value + } + return ref.current +} + +function compareLinkState(a: LinkState, b: LinkState) { + return a[0] === b[0] && a[1] === b[1] && a[2] === b[2] +} + +function resolveExternalLink( + hrefOption: { href: string; external?: boolean } | undefined, + to: unknown, + protocolAllowlist: AnyRouter['protocolAllowlist'], +): string | undefined { + if (hrefOption?.external) { + // Block dangerous protocols for external links + if (isDangerousProtocol(hrefOption.href, protocolAllowlist)) { + if (process.env.NODE_ENV !== 'production') { + console.warn(`Blocked Link with dangerous protocol: ${hrefOption.href}`) + } + return undefined + } + return hrefOption.href + } + if (isSafeInternal(to)) { + return undefined + } + if (typeof to !== 'string' || to.indexOf(':') === -1) { + return undefined + } + try { + new URL(to) + // Block dangerous protocols like javascript:, blob:, data: + if (isDangerousProtocol(to, protocolAllowlist)) { + if (process.env.NODE_ENV !== 'production') { + console.warn(`Blocked Link with dangerous protocol: ${to}`) + } + return undefined + } + return to + } catch {} + return undefined +} + +function resolveIsActive( + location: ParsedLocation, + next: ParsedLocation, + activeOptions: ActiveOptions | undefined, + basepath: string, + isHydrated: boolean, + isExternal: boolean, +): boolean { + if (isExternal) { + return false + } + if (activeOptions?.exact) { + const testExact = exactPathTest(location.pathname, next.pathname, basepath) + if (!testExact) { + return false + } + } else { + const currentPathSplit = removeTrailingSlash(location.pathname, basepath) + const nextPathSplit = removeTrailingSlash(next.pathname, basepath) + + const pathIsFuzzyEqual = + currentPathSplit.startsWith(nextPathSplit) && + (currentPathSplit.length === nextPathSplit.length || + currentPathSplit[nextPathSplit.length] === '/') + + if (!pathIsFuzzyEqual) { + return false + } + } + + if (activeOptions?.includeSearch ?? true) { + const searchTest = deepEqual(location.search, next.search, { + partial: !activeOptions?.exact, + ignoreUndefined: !activeOptions?.explicitUndefined, + }) + if (!searchTest) { + return false + } + } + + if (activeOptions?.includeHash) { + return isHydrated && location.hash === next.hash + } + return true +} + /** * Build anchor-like props for declarative navigation and preloading. * @@ -381,6 +491,12 @@ export function useLinkProps< // eslint-disable-next-line react-hooks/rules-of-hooks const isHydrated = useHydrated() + // eslint-disable-next-line react-hooks/rules-of-hooks + const stableSearch = useValueStable(options.search) + // eslint-disable-next-line react-hooks/rules-of-hooks + const stableParams = useValueStable(options.params) + // eslint-disable-next-line react-hooks/rules-of-hooks + const stableActiveOptions = useValueStable(activeOptions) // eslint-disable-next-line react-hooks/rules-of-hooks const _options = React.useMemo( () => options, @@ -391,136 +507,64 @@ export function useLinkProps< options._fromLocation, options.hash, options.to, - options.search, - options.params, + stableSearch, + stableParams, options.state, options.mask, options.unsafeRelative, ], ) + // Derive inside the selector so `compareLinkState` can bail out. Deriving after + // the subscription instead re-renders every link on every navigation, because + // the comparator only sees the location, not whether this link's output moved. // eslint-disable-next-line react-hooks/rules-of-hooks - const currentLocation = useStore( - router.stores.location, - (l) => l, - (prev, next) => prev.href === next.href, - ) - - // eslint-disable-next-line react-hooks/rules-of-hooks - const next = React.useMemo(() => { - const opts = { _fromLocation: currentLocation, ..._options } - return router.buildLocation(opts as any) - }, [router, currentLocation, _options]) - - // Use publicHref - it contains the correct href for display - // When a rewrite changes the origin, publicHref is the full URL - // Otherwise it's the origin-stripped path - // This avoids constructing URL objects in the hot path - const hrefOptionPublicHref = next.maskedLocation - ? next.maskedLocation.publicHref - : next.publicHref - const hrefOptionExternal = next.maskedLocation - ? next.maskedLocation.external - : next.external - // eslint-disable-next-line react-hooks/rules-of-hooks - const hrefOption = React.useMemo( - () => - getHrefOption( - hrefOptionPublicHref, - hrefOptionExternal, + const selectLinkState = React.useCallback( + (location: ParsedLocation): LinkState => { + const next = router.buildLocation({ + _fromLocation: location, + ..._options, + } as any) + + // Use publicHref - it contains the correct href for display + // When a rewrite changes the origin, publicHref is the full URL + // Otherwise it's the origin-stripped path + // This avoids constructing URL objects in the hot path + const hrefOption = getHrefOption( + next.maskedLocation ? next.maskedLocation.publicHref : next.publicHref, + next.maskedLocation ? next.maskedLocation.external : next.external, router.history, disabled, - ), - [disabled, hrefOptionExternal, hrefOptionPublicHref, router.history], - ) - - // eslint-disable-next-line react-hooks/rules-of-hooks - const externalLink = React.useMemo(() => { - if (hrefOption?.external) { - // Block dangerous protocols for external links - if (isDangerousProtocol(hrefOption.href, router.protocolAllowlist)) { - if (process.env.NODE_ENV !== 'production') { - console.warn( - `Blocked Link with dangerous protocol: ${hrefOption.href}`, - ) - } - return undefined - } - return hrefOption.href - } - const safeInternal = isSafeInternal(to) - if (safeInternal) return undefined - if (typeof to !== 'string' || to.indexOf(':') === -1) return undefined - try { - new URL(to as any) - // Block dangerous protocols like javascript:, blob:, data: - if (isDangerousProtocol(to, router.protocolAllowlist)) { - if (process.env.NODE_ENV !== 'production') { - console.warn(`Blocked Link with dangerous protocol: ${to}`) - } - return undefined - } - return to - } catch {} - return undefined - }, [to, hrefOption, router.protocolAllowlist]) - - // eslint-disable-next-line react-hooks/rules-of-hooks - const isActive = React.useMemo(() => { - if (externalLink) return false - if (activeOptions?.exact) { - const testExact = exactPathTest( - currentLocation.pathname, - next.pathname, - router.basepath, ) - if (!testExact) { - return false - } - } else { - const currentPathSplit = removeTrailingSlash( - currentLocation.pathname, - router.basepath, - ) - const nextPathSplit = removeTrailingSlash(next.pathname, router.basepath) - - const pathIsFuzzyEqual = - currentPathSplit.startsWith(nextPathSplit) && - (currentPathSplit.length === nextPathSplit.length || - currentPathSplit[nextPathSplit.length] === '/') - if (!pathIsFuzzyEqual) { - return false - } - } + const externalLink = resolveExternalLink( + hrefOption, + to, + router.protocolAllowlist, + ) - if (activeOptions?.includeSearch ?? true) { - const searchTest = deepEqual(currentLocation.search, next.search, { - partial: !activeOptions?.exact, - ignoreUndefined: !activeOptions?.explicitUndefined, - }) - if (!searchTest) { - return false - } - } + return [ + hrefOption?.href, + externalLink, + resolveIsActive( + location, + next, + stableActiveOptions, + router.basepath, + isHydrated, + externalLink !== undefined, + ), + ] + }, + [stableActiveOptions, disabled, isHydrated, _options, router, to], + ) - if (activeOptions?.includeHash) { - return isHydrated && currentLocation.hash === next.hash - } - return true - }, [ - activeOptions?.exact, - activeOptions?.explicitUndefined, - activeOptions?.includeHash, - activeOptions?.includeSearch, - currentLocation, - externalLink, - isHydrated, - next.hash, - next.pathname, - next.search, - router.basepath, - ]) + // eslint-disable-next-line react-hooks/rules-of-hooks + const [href, externalLink, isActive] = useStore( + router.stores.location, + selectLinkState, + compareLinkState, + ) // Get the active props const resolvedActiveProps: React.HTMLAttributes = isActive @@ -563,13 +607,13 @@ export function useLinkProps< // eslint-disable-next-line react-hooks/rules-of-hooks const doPreload = React.useCallback(() => { - router - .preloadRoute({ ..._options, _builtLocation: next } as any) - .catch((err) => { - console.warn(err) - console.warn(preloadWarning) - }) - }, [router, _options, next]) + // `preloadRoute` builds the location itself; it is no longer held in render + // state. It only reads the options, so `_options` can go through as-is. + router.preloadRoute(_options as any).catch((err) => { + console.warn(err) + console.warn(preloadWarning) + }) + }, [router, _options]) // eslint-disable-next-line react-hooks/rules-of-hooks const preloadViewportIoCallback = React.useCallback( @@ -699,7 +743,7 @@ export function useLinkProps< ...propsSafeToSpread, ...resolvedActiveProps, ...resolvedInactiveProps, - href: hrefOption?.href, + href, ref: innerRef as React.ComponentPropsWithRef<'a'>['ref'], onClick: composeHandlers([onClick, handleClick]), onBlur: composeHandlers([onBlur, handleLeave]), diff --git a/packages/react-router/tests/link.test.tsx b/packages/react-router/tests/link.test.tsx index f06a3a8dec..b1c82b5368 100644 --- a/packages/react-router/tests/link.test.tsx +++ b/packages/react-router/tests/link.test.tsx @@ -31,6 +31,7 @@ import { redirect, retainSearchParams, stripSearchParams, + useLinkProps, useLoaderData, useMatchRoute, useParams, @@ -7540,3 +7541,165 @@ describe('protocolAllowlist', () => { ) }) }) + +describe('link re-render bail-out', () => { + // `useLinkProps` subscribes to the location store. Counting renders of a + // component that calls it therefore measures exactly what the subscription + // publishes: a link whose resolved href and active state are unaffected by a + // navigation should not re-render at all. + // + // The components are memoized so a re-render of the route component that owns + // them cannot be mistaken for the subscription firing, and the link options are + // module-stable for the same reason. + const stableOptions = { + unaffected: { to: '/elsewhere' } as const, + becomesActive: { to: '/posts' } as const, + } + + function setup() { + const renderCounts = { unaffected: 0, becomesActive: 0 } + + const CountingLink = React.memo(function CountingLink({ + name, + }: { + name: keyof typeof renderCounts + }) { + renderCounts[name]++ + const linkProps = useLinkProps(stableOptions[name]) + return + }) + + const rootRoute = createRootRoute({ + component: () => ( + <> + + + + Go + + + + ), + }) + + const indexRoute = createRoute({ + getParentRoute: () => rootRoute, + path: '/', + component: () =>

Index

, + }) + + const postsRoute = createRoute({ + getParentRoute: () => rootRoute, + path: '/posts', + component: () =>

Posts

, + }) + + const elsewhereRoute = createRoute({ + getParentRoute: () => rootRoute, + path: '/elsewhere', + component: () =>

Elsewhere

, + }) + + const router = createRouter({ + routeTree: rootRoute.addChildren([ + indexRoute, + postsRoute, + elsewhereRoute, + ]), + history: createMemoryHistory({ initialEntries: ['/'] }), + }) + + return { router, renderCounts } + } + + test('does not re-render a link a navigation cannot affect', async () => { + const { router, renderCounts } = setup() + render() + + await screen.findByTestId('unaffected') + const before = { ...renderCounts } + expect(screen.getByTestId('becomesActive')).not.toHaveAttribute( + 'data-status', + ) + + fireEvent.click(await screen.findByTestId('go')) + expect(await screen.findByText('Posts')).toBeInTheDocument() + + // `/posts` gains its active state, so it has to re-render. + expect(renderCounts.becomesActive).toBeGreaterThan(before.becomesActive) + expect(screen.getByTestId('becomesActive')).toHaveAttribute( + 'data-status', + 'active', + ) + + // `/elsewhere` is neither the origin nor the destination: its href and + // active state are identical before and after, so the subscription must + // bail out rather than publish an equal value. Asserting the published + // values too, so a selector that returned a constant would still fail. + expect(renderCounts.unaffected).toBe(before.unaffected) + expect(screen.getByTestId('unaffected')).toHaveAttribute( + 'href', + '/elsewhere', + ) + expect(screen.getByTestId('unaffected')).not.toHaveAttribute('data-status') + }) +}) + +describe('explicit-undefined params are not collapsed into an empty object', () => { + // `params: { category: undefined }` clears an inherited optional param while + // `params: {}` inherits it, so the two build different locations. The link + // options are stabilised by value, and that comparison must not treat them as + // equal or the link keeps publishing the stale href. + it('updates href when params goes from {} to { category: undefined }', async () => { + function CategoryLink({ + params, + }: { + params: Record + }) { + return ( + + link + + ) + } + + const rootRoute = createRootRoute({ component: () => }) + const postsRoute = createRoute({ + getParentRoute: () => rootRoute, + path: '/posts/{-$category}', + component: function Posts() { + const [params, setParams] = React.useState< + Record + >({}) + return ( + <> + + + + ) + }, + }) + + const router = createRouter({ + routeTree: rootRoute.addChildren([postsRoute]), + history: createMemoryHistory({ initialEntries: ['/posts/tech'] }), + }) + + render() + + await waitFor(() => + expect(screen.getByTestId('lnk')).toHaveAttribute('href', '/posts/tech'), + ) + + fireEvent.click(screen.getByTestId('clear')) + + await waitFor(() => + expect(screen.getByTestId('lnk')).toHaveAttribute('href', '/posts'), + ) + }) +})