feat: add LocalePrefix never option #388

boris-arkenaar · 2023-07-07T15:53:05Z

fixes #366

vercel · 2023-07-07T15:53:09Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
example-next-13-next-auth	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jul 18, 2023 6:03am
next-intl-docs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jul 18, 2023 6:03am
next-intl-example-next-13	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jul 18, 2023 6:03am

vercel · 2023-07-07T15:53:12Z

@boris-arkenaar is attempting to deploy a commit to the next-intl Team on Vercel.

A member of the Team first needs to authorize it.

boris-arkenaar · 2023-07-10T11:37:39Z

@amannn Could you do a review?

amannn

What a fantastic PR, thank you for the great work! 🙌

I've played a bit with the example-next-13 app in this repo to test the new option and everything looks good to me. It seems like when you continuously switch the locale, at some point Next.js will returned a cached response, which will result in the cookie not being updated. I think there's a chance that the situation here will improve with #149, once we integrate createServerContext. But for the time being this should be fine and I think for a natural user flow (i.e. not randomly switching between languages), this shouldn't be an issue.

I've published a prerelease of this feature:

[email protected]

Would you like to try this out in your app to see if everything works as expected there?

amannn · 2023-07-10T14:43:16Z

packages/next-intl/test/middleware/middleware.test.tsx

+ });
+
+ it('rewrites requests for other locales at a nested path', () => {
+ middleware(createMockRequest('/list', 'de'));


Can you also include some tests where the accept-language header and the cookie value don't match while no locale is in the pathname? The cookie value should have precedence in this case.

amannn · 2023-07-10T14:47:26Z

packages/next-intl/src/middleware/middleware.tsx

+ configWithDefaults.localePrefix === 'never' ||
+ (hasMatchedDefaultLocale &&
+ (configWithDefaults.localePrefix === 'as-needed' ||
+ configWithDefaults.domains))


I'm wondering what we should do with the alternate links in case of localePrefix: 'never' (see L198 below).

SEO is not really an issue here, but maybe some assistive technology will still read the header.

Here's an example for /about when I have NEXT_LOCALE=de:

Link: <http://localhost:3000/about>; rel="alternate"; hreflang="en", <http://localhost:3000/de/about>; rel="alternate"; hreflang="de", <http://localhost:3000/about>; rel="alternate"; hreflang="x-default"

This seems somewhat off, probably we should adjust the links to match this:

Link: <http://localhost:3000/about>; rel="alternate"; hreflang="de", <http://localhost:3000/en/about>; rel="alternate"; hreflang="en", <http://localhost:3000/about>; rel="alternate"; hreflang="x-default"

Would you mind looking into this?

Alternate links don't really make sense in this case, since the URL won't set or change the current locale, right?

Or you could actually. It's some 'hidden' functionality at the moment I believe. When you go to a URL prefixed with a locale it will set that locale in the cookie and redirect to the URL without the locale prefix.

Maybe more explicit then, like this?

Link: <http://localhost:3000/de/about>; rel="alternate"; hreflang="de", <http://localhost:3000/en/about>; rel="alternate"; hreflang="en", <http://localhost:3000/about>; rel="alternate"; hreflang="x-default"

Cannot find anything about screen readers and the use of alternate link headers though.

Maybe leave it out for locale prefix never?

Sure, that's the other option. Fine by me, but we should mention it in the docs then!

Disabling the alternate links and a brief mention in the docs would be the last part missing here, afterwards we can release this!

amannn · 2023-07-10T14:48:40Z

docs/pages/docs/routing/middleware.mdx

+```
+
+In this case all requests for all locales will be rewritten internally.
+


Well written 👌

boris-arkenaar · 2023-07-10T17:11:12Z

Would you like to try this out in your app to see if everything works as expected there?

Thanks for the pre-release. Tested with my app. Seems to be working well here as well!

boris-arkenaar · 2023-07-10T17:14:40Z

Oh, wait. Still running my custom middleware. Let me set it up properly now.

boris-arkenaar · 2023-07-10T18:04:29Z

Got it. Seems to work great :)

amannn · 2023-07-17T08:24:45Z

@boris-arkenaar Do you have the time to complete this or can I help with something? (see the last open point)

boris-arkenaar · 2023-07-17T09:04:08Z

@amannn You mean about the alternate links? Missed that comment. I can update that soon.

amannn · 2023-07-17T09:24:20Z

@boris-arkenaar Yep, correct! Thank you so much! 🙌

boris-arkenaar · 2023-07-17T17:00:20Z

@amannn Updated code and docs. Wanted to add some tests as well, but couldn't figure out how to mock/spy on getAlternateLinksHeaderValue.

- A little bit less usage of callout in the docs (we already have so many) - Minor wording fixes in middleware docs - Fix mock behavior in middleware - Add tests for alternate links in middleware

amannn · 2023-07-18T08:09:20Z

@amannn Updated code and docs. Wanted to add some tests as well, but couldn't figure out how to mock/spy on getAlternateLinksHeaderValue.

Thank you so much! I've added a commit with minor fixes, including a fix for the mock behavior in the middleware and tests for the alternate links.

Will merge and release this once CI is green, thank you so much for the great contribution! 🙌

vercel bot deployed to Preview – example-next-13-next-auth July 7, 2023 15:54 View deployment

vercel bot deployed to Preview – next-intl-example-next-13 July 7, 2023 15:54 View deployment

feat: Add LocalePrefix never option

2204613

boris-arkenaar force-pushed the feat/locale-prefix-never branch from 30f885a to 2204613 Compare July 7, 2023 15:55

boris-arkenaar changed the title ~~Add LocalePrefix never option~~ feat: add LocalePrefix never option Jul 7, 2023

vercel bot deployed to Preview – example-next-13-next-auth July 7, 2023 15:56 View deployment

vercel bot deployed to Preview – next-intl-example-next-13 July 7, 2023 15:56 View deployment

Update documentation

e094552

vercel bot deployed to Preview – example-next-13-next-auth July 8, 2023 11:19 View deployment

vercel bot deployed to Preview – next-intl-example-next-13 July 8, 2023 11:19 View deployment

vercel bot deployed to Preview – next-intl-docs July 10, 2023 12:24 View deployment

amannn reviewed Jul 10, 2023

View reviewed changes

Add tests with cookie

b8ccc29

vercel bot deployed to Preview – example-next-13-next-auth July 10, 2023 16:21 View deployment

vercel bot deployed to Preview – next-intl-example-next-13 July 10, 2023 16:21 View deployment

Add tests for locale in url

00f5457

vercel bot deployed to Preview – next-intl-example-next-13 July 10, 2023 16:51 View deployment

vercel bot deployed to Preview – example-next-13-next-auth July 10, 2023 16:51 View deployment

vercel bot deployed to Preview – next-intl-docs July 11, 2023 07:57 View deployment

Skip alternate links on localePrefix never

1bc6fe1

vercel bot deployed to Preview – next-intl-example-next-13 July 17, 2023 17:00 View deployment

vercel bot deployed to Preview – example-next-13-next-auth July 17, 2023 17:00 View deployment

vercel bot deployed to Preview – next-intl-docs July 18, 2023 06:03 View deployment

nits:

d90b1dc

- A little bit less usage of callout in the docs (we already have so many) - Minor wording fixes in middleware docs - Fix mock behavior in middleware - Add tests for alternate links in middleware

vercel bot deployed to Preview – next-intl-docs July 18, 2023 08:08 View deployment

amannn merged commit 92ec33a into amannn:main Jul 18, 2023
3 checks passed

boris-arkenaar deleted the feat/locale-prefix-never branch July 18, 2023 11:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add LocalePrefix never option #388

feat: add LocalePrefix never option #388

boris-arkenaar commented Jul 7, 2023

vercel bot commented Jul 7, 2023 •

edited

Loading

vercel bot commented Jul 7, 2023

boris-arkenaar commented Jul 10, 2023

amannn left a comment

amannn Jul 10, 2023

boris-arkenaar Jul 10, 2023

amannn Jul 10, 2023

boris-arkenaar Jul 10, 2023

boris-arkenaar Jul 10, 2023

boris-arkenaar Jul 10, 2023

boris-arkenaar Jul 10, 2023

boris-arkenaar Jul 10, 2023

amannn Jul 10, 2023

amannn Jul 11, 2023

amannn Jul 10, 2023

boris-arkenaar commented Jul 10, 2023

boris-arkenaar commented Jul 10, 2023

boris-arkenaar commented Jul 10, 2023

amannn commented Jul 17, 2023

boris-arkenaar commented Jul 17, 2023

amannn commented Jul 17, 2023

boris-arkenaar commented Jul 17, 2023

amannn commented Jul 18, 2023

		```

		In this case all requests for all locales will be rewritten internally.

feat: add LocalePrefix never option #388

feat: add LocalePrefix never option #388

Conversation

boris-arkenaar commented Jul 7, 2023

vercel bot commented Jul 7, 2023 • edited Loading

vercel bot commented Jul 7, 2023

boris-arkenaar commented Jul 10, 2023

amannn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boris-arkenaar commented Jul 10, 2023

boris-arkenaar commented Jul 10, 2023

boris-arkenaar commented Jul 10, 2023

amannn commented Jul 17, 2023

boris-arkenaar commented Jul 17, 2023

amannn commented Jul 17, 2023

boris-arkenaar commented Jul 17, 2023

amannn commented Jul 18, 2023

vercel bot commented Jul 7, 2023 •

edited

Loading