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

redirect does not respect as-needed locale prefix #1335

Closed
3 tasks done
jorgemoya opened this issue Sep 12, 2024 · 7 comments · Fixed by #1391 or bigcommerce/catalyst#1491
Closed
3 tasks done

redirect does not respect as-needed locale prefix #1335

jorgemoya opened this issue Sep 12, 2024 · 7 comments · Fixed by #1391 or bigcommerce/catalyst#1491
Labels
bug Something isn't working

Comments

@jorgemoya
Copy link

Description

When using locale prefix as-needed, I would expect redirect to respect this configuration, instead it will append default locale. This is not how using Link behaves, so it is unexpected. For non default locales, I would expect for redirect to append locale prefix.

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://next-intl-redirect-issue-of1rlux37-jorgemoyas-projects.vercel.app/

Reproduction description

Steps to reproduce:

  1. Open reproduction
  2. Click on Click to redirect
  3. Error: Notice how default locale is appended to url. Navigating with header links works as expected.

Expected behaviour

The expected behavior would be to respect the as-needed locale prefix configuration and for navigation within the default locale to ignore appending locale, much like using the Link component. For non default locales it would be fine to append locale prefix.

@amannn
Copy link
Owner

amannn commented Sep 17, 2024

Thanks for the report! I'm currently working on a re-implementation of the navigation APIs in #1316 in order to address some long-standing issues. I think the issue you've reported can be solved too as part of that, I've put this on my list!

This is currently my top priority, I'll keep you updated!

@amannn amannn removed the unconfirmed Needs triage. label Sep 17, 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! 🙏❤️

@DevOsaWebsite
Copy link

DevOsaWebsite commented Oct 5, 2024

@amannn HI!

next: 14.2.4
next-intl: 3.21.0-canary.0

When use the Link component from routing, for links to the default locale you expect an href like

domain.com

but you get

domain.com/defaultLocale

DOM render

<a hreflang="en" href="/en/tools"> but expect href="/tools"

This does not interfere with user navigation (307 redirect works well). But it is a problem for SEO.
Because when crawling a site, non-existent pages will end up in the Google index. Thus eating up the crawling budget. Also disrupting internal linking, eating up link weight...

I suspect the problem is here:

function getLocalePrefix(locale, localePrefix) {
  var _localePrefix$prefixe;
  return localePrefix.mode !== 'never' && ((_localePrefix$prefixe = localePrefix.prefixes) === null || _localePrefix$prefixe === void 0 ? void 0 : _localePrefix$prefixe[locale]) ||
  // We return a prefix even if `mode: 'never'`. It's up to the consumer
  // to decide to use it or not.
  '/' + locale;
}

missing if locale === defaultLocale ...

@amannn
Copy link
Owner

amannn commented Oct 7, 2024

@DevOsaWebsite Thanks for providing feedback here!

Some questions:

  1. Besides updating the version, did you upgrade to createNavigation?
  2. Are you referring to the markup resulting from the server side render, or the final markup after hydration that renders in the browser?
  3. Are you using domains in combination with localePrefix: 'as-needed'?

Please refer to #1316, you should find an answer there.

@jorgemoya
Copy link
Author

@amannn Sorry for not testing this in time, but it works as expected, thank you!

@mfkrause
Copy link

mfkrause commented Oct 21, 2024

@amannn I'm still experiencing this issue on 3.22. I'm using createNavigation and do not use domains. Here's the routing file:

export const routing = defineRouting({
  locales: ['en-US', 'de-DE'],
  defaultLocale: 'de-DE',
  localePrefix: {
    mode: 'as-needed',
    prefixes: {
      'en-US': '/en',
    },
  },
  pathnames: {
    '/': '/',
    // ...
  },
});

export const { Link, redirect, usePathname, useRouter, getPathname } = createNavigation(routing);

When I use the exported Link component like so:

'use client';
import { useParams } from 'next/navigation';
import { Link, usePathname } from '@/i18n/routing';
import { useTranslations, useLocale } from 'next-intl';

const SomeComponent = () => {
  const pathname = usePathname();
  const params = useParams();
  const t = useTranslations();
  const locale = useLocale();

  return (
    <Link
      href={{
        pathname,
        params,
      }}
      locale={locale === 'de-DE' ? 'en-US' : 'de-DE'}
      className="text-sm">
        {t('links.switchLanguage')}
    </Link>
  );
}

The resulting tag will still have the superfluous de-DE prefix. The redirection works.

Any clue?

Edit: Sorry, I just noticed this issue here is about redirect, not Link. If I read the PR for createNavigation correctly, the issue I'm describing should be fixed as well though, no?

@amannn
Copy link
Owner

amannn commented Oct 22, 2024

@mfkrause There's some background on this here: #1316 (comment).

Does that make sense? Maybe I should mention this in the docs somewhere (updated).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants