Support `cloneElement` in RSC-rendered `<Link />` (better compatibility with Radix Primitives & friends)

mschinis commented 3 months ago

Description

I'm trying to use shadcn NavigationMenu together with the next-intl link, using legacyBehavior+passHref but I'm getting a bunch of the following warnings:

"onClick" was passed to <Link> with `href` of `/about` but "legacyBehavior" was set. The legacy behavior requires onClick be set on the child of next/link

Usage

// Import next-intl Link component returned by createSharedPathnamesNavigation
import { Link } from "@/i18n/navigation"; 

function NavigationMenuLocalizedLink({ item }: { item: INavigationMenuItem }) {
  return (
    <Link href={item.href} legacyBehavior passHref>
      <NavigationMenuLink>{item.title}</NavigationMenuLink>
    </Link>
  );
}

Tracing back the issue, it looks like nextjs doesn't like setting the onClick property, when legacyBehavior is set. https://github.com/amannn/next-intl/blob/ed4681fa70b8f67a97e1364deaabf58df302d80c/packages/next-intl/src/navigation/shared/BaseLink.tsx#L76

Verifications

[X] I've verified that the problem I'm experiencing isn't covered in the docs.
[X] I've searched for similar, existing issues on GitHub and Stack Overflow.
[X] I've compared my app to a working example to look for differences.

Mandatory reproduction URL

https://github.com/mschinis/next-intl-bug-repro-app-router

Reproduction description

Steps to reproduce:

Open reproduction
Look at console

Expected behaviour

Expected behavior is to not have any warnings.

mschinis commented 3 months ago

The following workaround seems to work for now, but the behavior of the issue still stands:

'use client';

function NavigationMenuLocalizedLink({ item }: { item: INavigationMenuItem }) {
  return (
    <NavigationMenuLink asChild>
      <Link href={item.href}>{item.title}</Link>
    </NavigationMenuLink>
  );
}

amannn commented 3 months ago

Thanks for the careful report and reproduction!

This could probably be addressed by adding cloneElement in BaseLink in case legacyBehavior is detected. I'm not sure though if it's worth it though, given that—as the name suggests—this is legacy behavior and there are modern alternatives (like the one you suggested).

This was in fact the first time this was reported, so maybe it's a good idea to keep this open to see if more people are affected and to give visibility to the alternative you've posted.

rogerogers commented 3 months ago

Yes, I'm also facing the same issue. Directly importing from next/link does not have this problem.

amannn commented 2 months ago

This topic came up again in https://github.com/amannn/next-intl/issues/1322 and I've added some details in https://github.com/amannn/next-intl/issues/1322#issuecomment-2329841708.

Will have a look if we can improve something here.

Btw. possibly a non-obtrusive workaround might be this:

// components/Link.tsx
'use client';

// Re-export from the file that creates the `<Link />` component
export {Link as default} from '@/routing';

// Import the re-exported `Link`
import Link from "components/Link"; 

function NavigationMenuLocalizedLink({ item }: { item: INavigationMenuItem }) {
  return (
    <Link href={item.href} legacyBehavior passHref>
      <NavigationMenuLink>{item.title}</NavigationMenuLink>
    </Link>
  );
}

I think this should work as expected with Radix Primitives & friends.

EDIT: Asked the Radix team here about an alternative implementation that would work with RSC: https://github.com/radix-ui/primitives/issues/2537#issuecomment-2381324518.

amannn / next-intl