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

Improve initial render of Link for localePrefix: 'as-necessary' and make getPathname more useful #444

Closed
amannn opened this issue Aug 7, 2023 · 16 comments · Fixed by #1391
Labels
area: routing enhancement New feature or request

Comments

@amannn
Copy link
Owner

amannn commented Aug 7, 2023

Is your feature request related to a problem? Please describe.

The localePrefix: 'as-necessary' strategy currently has these drawbacks:

  1. The initial render of Link in RSC always includes a prefix, which we potentially remove during hydration if we detect that no locale prefix should be used.
  2. getPathname is somewhat hard to use, because users have to specify their own logic to check if a prefix should be added.

Describe the solution you'd like

  1. The navigation APIs should accept localePrefix, defaultLocale and domains, all being expected to be provided in case you use them. We should move all routing config to a shared type that can be shared with the middleware. Update: Done in feat: Add defineRouting for easier i18n routing setup #1299
  2. Link should consider the new options for the initial render (see Improve initial render of Link for localePrefix: 'as-necessary' and make getPathname more useful #444 (comment)). If Link receives locales, we should be able to use usePathname to see if the current pathname is prefixed and use this information to get the initial SSR render right.
  3. getPathname should consider the options too and return a final pathname (with/without a locale prefix, with/without domain). Furthermore, getPathname should be supported for shared pathnames too.
  4. The remaining navigation APIs should also be checked for which improvements we could make based on the new configuration.
  5. Navigation APIs could accept a domain if a user wants to link/redirect to another domain.

This will require a major version to ensure users supply the new options (related to #779).

In regard to (2) there's an edge case for localePrefix: 'as-necessary' and domains. The domain is only available at request time, therefore we have to either a) support SSG and potentially patch the href of Link on the client side or b) require dynamic rendering and return a final URL. This is TBD. Note that getPathname only has "one shot" to return a final URL, which would certainly require reading from headers. An idea could be to require dynamic rendering in case domains are configured, but to point users towards the alternative of building a separate app per domain if SSG is important.

Make sure to remove the note in the localePrefix: 'as-needed' docs once this is implemented.

Describe alternatives you've considered

The problems outlined above are mostly related to localePrefix: 'as-necessary'.

You can avoid these by either:

  1. Using localePrefix: 'always'
  2. Using localePrefix: 'never' and supplying this to the navigation factory function (already supported)
@amannn amannn added enhancement New feature or request unconfirmed Needs triage. labels Aug 7, 2023
@amannn
Copy link
Owner Author

amannn commented Aug 7, 2023

@jlalmes Can you provide a screenshot from the Google Search Console issue that shows the issue?

Maybe I was a bit too naive to think that this isn't an issue for Google, since we a) always have a working URL with the redirect and b) patch it up as necessary on the client side.

@jlalmes
Copy link
Contributor

jlalmes commented Aug 7, 2023

Here's a screenshot from Google Search Console for a new-ish site I'm working on. It's a non-critical issue because the correct pages are indexed, but this error is also shown for the prefixed paths.

Screenshot 2023-08-07 at 06 49 19

@amannn
Copy link
Owner Author

amannn commented Aug 8, 2023

Hmm, I see—thanks! Is maybe localePrefix: 'always' a viable workaround for you for the time being? I'll look into getting this fixed for localePrefix: 'as-necessary' though, but I can't promise an ETA yet!

@amannn
Copy link
Owner Author

amannn commented Aug 22, 2023

Small update:

The factory functions for navigation APIs that are being worked on in #426 could help to mitigate this for localePrefix: 'as-necessary'. However, as soon as the user introduces domain configuration, we're again in a position where we don't know if a given pathname should be prefixed or not, since domains can have different default locales.

That means that if the default locale can only be determined in the middleware, we can't statically render different link targets on the server side. If the navigation APIs would also accept domain config, we could check if the user doesn't use domain config to mitigate this, but this gets quite complex and still doesn't fix all cases (i.e. when the user uses a domain config). Also, we have to accept domain config only to detect if the user doesn't use one—seems a bit backward.

I'm wondering if it would be a good idea to flip the default for localePrefix to always, as it doesn't have the issue, and educate users about this side effect when using localePrefix: 'as-necessary'.

Update: With the release candidate, the default for localePrefix was changed to always, which seems to be a better default.

@amannn amannn removed the unconfirmed Needs triage. label Aug 23, 2023
@amannn amannn changed the title Consider defaultLocale and localePrefix for initial render of Link when localePrefix: 'as-necessary' Consider defaultLocale and localePrefix for initial render of Link when localePrefix: 'as-necessary' or localePrefix: 'never' Nov 24, 2023
@amannn amannn changed the title Consider defaultLocale and localePrefix for initial render of Link when localePrefix: 'as-necessary' or localePrefix: 'never' Consider defaultLocale and localePrefix for initial render of Link when localePrefix is as-necessary or never Nov 24, 2023
@zipme
Copy link

zipme commented Nov 28, 2023

@amannn Could this be fixed more easily for when the localePrefix is set to never?

@amannn
Copy link
Owner Author

amannn commented Nov 28, 2023

That's a good point actually. Based on your use case, in 18157bc I've added a note that localeDetection: 'never' is also useful if you have locales per domain.

With that, we could consider:

  1. Adding an optional localePrefix to createSharedPathnamesNavigation as well as createLocalizedPathnamesNavigation that if set to never will not add a prefix to links. With this, only as-necessary needs the current patching behavior.
  2. Update the docs accordingly:
    1. Mention in the middleware docs for localePrefix that if a value other than always is chosen, this should be set for the navigation factory function too.
    2. Add a note to the navigation docs about the new property.

Would you be interested in working on this?

@zipme
Copy link

zipme commented Nov 28, 2023

@amannn sounds like a plan! For now I have added a wrapper component that seems to do the trick:

"use client";

import {
  type I18nLink,
  type LocaleCode,
  getPathname,
  type pathnames,
} from "@/lib/navigation";
import { useLocale } from "next-intl";
import NextLink from "next/link";
import { type ComponentProps } from "react";

export function Link<Pathname extends keyof typeof pathnames>({
  href,
  ...rest
}: ComponentProps<typeof I18nLink<Pathname>>) {
  const locale = useLocale() as LocaleCode;
  // @ts-expect-error
  const localizedHref = getPathname({ href, locale });
  return <NextLink href={localizedHref} {...rest} />;
}

@amannn
Copy link
Owner Author

amannn commented Nov 28, 2023

Sounds good! I've started working on an implementation.

amannn added a commit that referenced this issue Nov 29, 2023
…render of `Link` when using `localePrefix: never`. Also fix edge case in middleware when using localized pathnames for redirects that remove a locale prefix (fixes an infinite loop). (#678)

By accepting an optional `localePrefix` for the navigation APIs, we can
get the initial render of the `href` of `Link` right if `localePrefix: 'never'` is set. This can be helpful if domain-based routing is used and
you have a single locale per domain.

Note that this change is backward-compatible. It's now recommended to
set the `localePrefix` for the navigation APIs to get improved behavior
for `Link` in case `localePrefix: 'never'` is used, but otherwise your
app will keep working with the previous behavior.





Ref #444
@amannn
Copy link
Owner Author

amannn commented Nov 29, 2023

@zipme I've addressed your use case in #678, this should help!

In regard to this issue: As a second step, if the navigation APIs would accept domains (like the middleware), then we could fix the initial render of Link for localePrefix: 'as-needed' too (if no domains are provided). There could arguably be some additional benefit by accepting a typed domain argument for Link (e.g. <Link href={{pathname: '/', domain: 'ca.example.com'}} />) in case users want to link to other domains. I think that would move us to a good-enough situation, where patching of the href is really only required in case you use domain-based routing and localePrefix: 'as-necessary'.

This would require a major version though to require domains for the navigation APIs in case this feature is being used. I'll leave this here for reference for now, it's TBD if this will be added.

@amannn amannn changed the title Consider defaultLocale and localePrefix for initial render of Link when localePrefix is as-necessary or never Consider defaultLocale and localePrefix for initial render of Link when localePrefix is as-necessary Nov 29, 2023
@amannn amannn changed the title Consider defaultLocale and localePrefix for initial render of Link when localePrefix is as-necessary Incorporate localePrefix, defaultLocale and domains for navigation APIs Jan 19, 2024
@amannn amannn changed the title Incorporate localePrefix, defaultLocale and domains for navigation APIs Incorporate localePrefix, defaultLocale and domains for navigation APIs to improve handling of localePrefix: 'as-necessary' Jan 19, 2024
@amannn amannn changed the title Incorporate localePrefix, defaultLocale and domains for navigation APIs to improve handling of localePrefix: 'as-necessary' Incorporate localePrefix, defaultLocale and domains for navigation APIs Jan 19, 2024
@markomitranic
Copy link

I was fiddling around with the navigation APIs these days, and realised that it seems impossible (at least for me) to make the domains feature in a way that supports SSG, since the domain can't be controlled as static params and is only resolved through middleware (or by manually specifying it). Did you have any ideas on how to solve that issue @amannn ?

@amannn
Copy link
Owner Author

amannn commented Jan 23, 2024

Replied here: #653 (comment)

@amannn amannn changed the title Incorporate localePrefix, defaultLocale and domains for navigation APIs Improve initial render of Link for localePrefix: 'as-necessary' and make getPathname more useful Mar 28, 2024
@zipme
Copy link

zipme commented Apr 4, 2024

When using localePrefix = "as-needed" and no domains, using createLocalizedPathnamesNavigation like

export const {
  redirect,
  Link
} = createLocalizedPathnamesNavigation({ locales, pathnames, localePrefix });

And calling redirect("/login") would still redirect to /en/login even if en is the default locale. (The same goes for Link #808)

I think it's possible and not too difficult if we also pass in the defaultLocale here:

export const {
  redirect,
  Link
} = createLocalizedPathnamesNavigation({ locales, pathnames, localePrefix, defaultLocale });

To let it redirect correctly right? But indeed, for domains configuration this would still be quite complicated I suppose

@zipme
Copy link

zipme commented Apr 4, 2024

I am doing this now as a workaround:

import { type RedirectType, redirect as nextRedirect } from "next/navigation";
import { locales, pathnames, localePrefix, defaultLocale } from './config'
import NextLink from "next/link";

export const {
  redirect,
  getPathname: getPathname_
} = createLocalizedPathnamesNavigation({ locales, pathnames, localePrefix });

export const getPathname: typeof getPathname_ = ({ href, locale }) => {
  const pathname = getPathname_({ href, locale });
  let prefix = "";
  if (locale !== defaultLocale) {
    prefix = `/${locale}`;
  }
  return `${prefix}${pathname}`;
};

export const redirect: typeof redirect_ = (href, type) => {
  const locale = useLocale();
  const path = getPathname({ href, locale });

  nextRedirect(path, type);
};

export function Link<Pathname extends keyof typeof pathnames>(
  { href, ...rest }: ComponentProps<typeof I18nLink<Pathname>>,
  ref: ForwardedRef<ElementRef<typeof NextLink>>,
) {
  const locale = useLocale();
  // @ts-expect-error this is okay
  const localizedHref = getPathname({ href, locale });
  return <NextLink href={localizedHref} {...rest} ref={ref} />;
}

@davx1992
Copy link

davx1992 commented May 2, 2024

Thanks for workarounds. Hope, that fix will be rolled out soon. As there is no point to initially render locale in tag, as it is causing SEO issues.

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 Author

amannn commented Oct 1, 2024

A quick note here: 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! 🙏❤️

@aigc-in-all
Copy link

I also encountered this problem. I temporarily created a custom Link and the test was ok.

// components/LocaleLink.tsx

import React from 'react';
import Link from 'next/link';
import { useLocale } from 'next-intl';

interface Props extends React.AnchorHTMLAttributes<HTMLAnchorElement> {
    href: string;
    locale?: string;
}

const LocaleLink: React.FC<Props> = ({ href, locale, ...props }) => {
    const defaultLocale = useLocale();
    const currentLocale = locale || defaultLocale;
    const isDefaultLocale = currentLocale === 'en'; // Assuming English is the default language

    let finalHref = href;

    if (isDefaultLocale) {
        // For the default language (English), remove the '/en' prefix if present
        finalHref = href.startsWith('/en/') ? href.slice(3) : href;
    } else if (!href.startsWith(`/${currentLocale}`)) {
        // For non-default languages, ensure the href includes the correct language prefix
        finalHref = `/${currentLocale}${href.startsWith('/') ? href : `/${href}`}`;
    }

    return <Link href={finalHref} {...props} />;
};

export default LocaleLink;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: routing enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants