Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Link component fails to render when parent uses asChild prop #1322

Closed
3 tasks done
MeesEgberts opened this issue Sep 4, 2024 · 6 comments
Closed
3 tasks done

Link component fails to render when parent uses asChild prop #1322

MeesEgberts opened this issue Sep 4, 2024 · 6 comments
Labels
bug Something isn't working unconfirmed Needs triage. upstream-issue This issue is caused by an upstream dependency (e.g. Next.js)

Comments

@MeesEgberts
Copy link

Description

The Link component from next-intl is not rendering when used within a parent component that has the asChild prop set. This issue occurs specifically when trying to use the Link component inside a DropdownMenuItem from shadcn/ui or @radix-ui/react-dropdown-menu.

Verifications

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

Mandatory reproduction URL

https://www.radix-ui.com/primitives/docs/components/dropdown-menu#dropdown-menu

Reproduction description

import ...

export async function DashboardHeaderAccountDropdown() {
  const t = await getTranslations("app.header.account-dropdown");

  return (
    <DropdownMenu>
      <DropdownMenuTrigger asChild>
        <Button variant="secondary" size="icon" className="rounded-full">
          <CircleUser className="h-5 w-5" />
          <span className="sr-only">{t("toggle")}</span>
        </Button>
      </DropdownMenuTrigger>
      <DropdownMenuContent align="end">
        <DropdownMenuLabel>{t("title")}</DropdownMenuLabel>
        <DropdownMenuSeparator />
        <DropdownMenuItem asChild>
          <Link
            className="flex items-center"
            href={{ pathname: "/app/account" }}
          >
            <Settings className="text-muted-foreground mr-2 h-4 w-4" />
            {t("settings")}
          </Link>
        </DropdownMenuItem>
        <DropdownMenuItem asChild>
          <Link href={{ pathname: "/app/account/security" }}>
            <Lock className="text-muted-foreground mr-2 h-4 w-4" />
            {t("security")}
          </Link>
        </DropdownMenuItem>
        <DropdownMenuSeparator />
        {/*<DropdownMenuItem>Logout</DropdownMenuItem>*/}
      </DropdownMenuContent>
    </DropdownMenu>
  );
}

Expected behaviour

I've tried using the next/link component and it renders just fine, but here nothing gets rendered.

@MeesEgberts MeesEgberts added bug Something isn't working unconfirmed Needs triage. labels Sep 4, 2024
@MeesEgberts MeesEgberts reopened this Sep 4, 2024
@amannn
Copy link
Owner

amannn commented Sep 5, 2024

This issue was discussed before in #665 and #1271.

What causes the issue is that Radix uses cloneElement in a component that is marked with 'use client'. The component tries to modify the child, but in your case <Link /> is a Server Component, therefore the props can't be modified anymore in the Client Components render pass (esp. if you consider that they run on the client side).

Link from next-intl is currently either a Server or a Client Component, depending on the environment you're importing it from. The reason for this is that we can avoid shipping some code to the client if you're calling Link in a Server Component.

Due to this, a possible workaround is to call Link in a Client Component.

I'm currently working on an overhaul for the navigation APIs and will have a look at this topic to see if we can do something about this to improve the compatibility with libraries like Radix that use cloneElement.

I'm closing this in favor of #1271.

@amannn amannn closed this as completed Sep 5, 2024
amannn added a commit that referenced this issue Oct 1, 2024
This PR provides a new **`createNavigation`** function that supersedes
the previously available APIs:
1. `createSharedPathnamesNavigation`
2. `createLocalizedPathnamesNavigation`

The new function unifies the API for both use cases and also fixes a few
quirks in the previous APIs.

**Usage**

```tsx
import {createNavigation} from 'next-intl/navigation';
import {defineRouting} from 'next-intl/routing';
 
export const routing = defineRouting(/* ... */);
 
export const {Link, redirect, usePathname, useRouter} =
  createNavigation(routing);
```

(see the [updated navigation
docs](https://next-intl-docs-git-feat-create-navigation-next-intl.vercel.app/docs/routing/navigation))

**Improvements**
1. A single API can be used both for shared as well as localized
pathnames. This reduces the API surface and simplifies the corresponding
docs.
2. `Link` can now be composed seamlessly into another component with its
`href` prop without having to add a generic type argument.
3. `getPathname` is now available for both shared as well as localized
pathnames (fixes #785)
4. `router.push` and `redirect` now accept search params consistently
via the object form (e.g. `router.push({pathname: '/users', query:
{sortBy: 'name'})`)—regardless of if you're using shared or localized
pathnames.
5. When using `localePrefix: 'as-necessary'`, the initial render of
`Link` now uses the correct pathname immediately during SSR (fixes
[#444](#444)). Previously, a
prefix for the default locale was added during SSR and removed during
hydration. Also `redirect` now gets the final pathname right without
having to add a superfluous prefix (fixes
[#1335](#1335)). The only
exception is when you use `localePrefix: 'as-necessary'` in combination
with `domains` (see [Special case: Using `domains` with `localePrefix:
'as-needed'`](https://next-intl-docs-git-feat-create-navigation-next-intl.vercel.app/docs/routing#domains-localeprefix-asneeded))
6. `Link` is now compatible with the `asChild` prop of Radix Primitives
when rendered in RSC (see
[#1322](#1322))

**Migrating to `createNavigation`**

`createNavigation` is generally considered a drop-in replacement, but a
few changes might be necessary:
1. `createNavigation` is expected to receive your complete routing
configuration. Ideally, you define this via the
[`defineRouting`](https://next-intl-docs.vercel.app/docs/routing#define-routing)
function and pass the result to `createNavigation`.
2. If you've used `createLocalizedPathnamesNavigation` and have
[composed the `Link` with its `href`
prop](https://next-intl-docs.vercel.app/docs/routing/navigation#link-composition),
you should no longer provide the generic `Pathname` type argument (see
[updated
docs](https://next-intl-docs-git-feat-create-navigation-next-intl.vercel.app/docs/routing/navigation#link-composition)).
```diff
- ComponentProps<typeof Link<Pathname>>
+ ComponentProps<typeof Link>
```
3. If you've used
[`redirect`](https://next-intl-docs.vercel.app/docs/routing/navigation#redirect),
you now have to provide an explicit locale (even if it's just [the
current
locale](https://next-intl-docs.vercel.app/docs/usage/configuration#locale)).
This change was necessary for an upcoming change in Next.js 15 where
`headers()` turns into a promise (see
[#1375](#1375) for details).
```diff
- redirect('/about')
+ redirect({pathname: '/about', locale: 'en'})
```
4. If you've used
[`getPathname`](https://next-intl-docs.vercel.app/docs/routing/navigation#getpathname)
and have previously manually prepended a locale prefix, you should no
longer do so—`getPathname` now takes care of this depending on your
routing strategy.
```diff
- '/'+ locale + getPathname(/* ... */)
+ getPathname(/* ... */);
```
5. If you're using a combination of `localePrefix: 'as-necessary'` and
`domains` and you're using `getPathname`, you now need to provide a
`domain` argument (see [Special case: Using `domains` with
`localePrefix:
'as-needed'`](https://next-intl-docs-git-feat-create-navigation-next-intl.vercel.app/docs/routing#domains-localeprefix-asneeded))
@amannn
Copy link
Owner

amannn commented Oct 1, 2024

The feedback raised in this issue has been addressed as part of an upcoming createNavigation function (currently available as a prerelease here: #1391). In case you decide to give it a spin, let me know if everything works as expected for you!

Also, thanks again for the great feedback here! 🙏❤️

@amannn
Copy link
Owner

amannn commented Oct 2, 2024

Sorry, upon further investigation, I found that Radix still doesn't seem to handle all cases. I got in touch with a maintainer of Raidx to hopefully resolve this upstream: radix-ui/primitives#2537 (comment)

@amannn amannn added the upstream-issue This issue is caused by an upstream dependency (e.g. Next.js) label Oct 2, 2024
@wottpal
Copy link

wottpal commented Nov 20, 2024

@amannn Is there any update on this as the upstream issue is closed but the behavior still persists?

@amannn
Copy link
Owner

amannn commented Nov 20, 2024

A maintainer asked me to open a new issue instead: radix-ui/primitives#3165

@amannn
Copy link
Owner

amannn commented Nov 20, 2024

I've also shared a workaround here btw.: #1271 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working unconfirmed Needs triage. upstream-issue This issue is caused by an upstream dependency (e.g. Next.js)
Projects
None yet
Development

No branches or pull requests

3 participants