feat: Add redirects for case mismatches in locale prefixes (e.g. `/EN` → `/en`) #861

fkapsahili · 2024-02-15T07:48:00Z

I noticed the same in one of my projects and thought I could look into it 🙂 . This PR should implement a case-insensitive matching mechanism for the given locales. This means that the package should treat a request with an invalid case locale, such as de-at or DE, as valid and map it correctly to de-AT and de respectively; it should also rewrite the URL to reflect the correct case, for example by rewriting de-at to de-AT.

I have done a little bit research on this topic and I think case-insensitive matching for locales makes sense to enhance the UX and avoid people mixing up uppercase and lowercase when typing in locale codes. Also, it seems to be a common practice on a lot of popular websites. Maybe it would still make sense to be able to control the behavior via a property in the middleware. What do you think about this?

Also, in the Playwright test I added, I noticed that the matcher toHaveURL still expects /DE as the URL. In my browser, however, the redirect works as expected from /DE to /de. Just let me know if I have overlooked something in the implementation, or if I can improve something 🙂.

Prerequisites

Use a meaningful title for the pull request
Test the changes you've made by adding or editing tests

Content

Added tests to the middleware that verify the pathLocale is converted correctly in a case-insensitive manner
Changed getNormalizedPathname to handle the locale in a case-insensitive manner
Changed getLocaleFromPathname to get a validated version of the locale with case-insensitive matching

Tests

Added tests to verify the redirect for invalid cased locales works as intended
Added a playwright test to the example-app-router-playground to make sure the redirect works

Closes #775

vercel · 2024-02-15T07:48:04Z

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

Name	Status	Preview	Comments	Updated (UTC)
next-intl-docs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Feb 20, 2024 8:49am
next-intl-example-app-router	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Feb 20, 2024 8:49am

vercel · 2024-02-15T07:48:05Z

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

A member of the Team first needs to authorize it.

amannn

Another great PR, thank you so much @fkapsahili! 🙌

Let me know if my inline comment is helpful, it seems to me like there might have been a slight misunderstanding.

Maybe it would still make sense to be able to control the behavior via a property in the middleware. What do you think about this?

Let's make this non-configurable for the time being, I can't think of a good use case where you wouldn't want this and composing the middleware generally gives users more options for edge cases than what the next-intl middleware assumes as defaults.

amannn · 2024-02-16T20:21:53Z

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

+        expect(MockedNextResponse.rewrite).toHaveBeenCalled();
+        expect(MockedNextResponse.next).not.toHaveBeenCalled();
+        expect(MockedNextResponse.redirect).not.toHaveBeenCalled();
+        expect(MockedNextResponse.rewrite.mock.calls[0][0].toString()).toBe(
+          'http://localhost:3000/de-AT'


I'm not sure if we're on the same page, but this test doesn't seem to assert for what the description says.

Let's clear up the terminology:

Redirect: A status code in the 3xx range is returned that points the user to another location.

Rewrite: The incoming request is mapped to another internal path.

In the context of next-intl, we e.g. have a redirect for the root / to the default locale (e.g. /en). On the other hand, we use rewrites e.g. for localized pathnames to handle them via a single internal pathname.

The way I'd imagine this feature and how I've observed it e.g. on MDN is that a redirect is returned when there's a case mismatch.

Due to this, the test should read like this:

Suggested change

expect(MockedNextResponse.rewrite).toHaveBeenCalled();

expect(MockedNextResponse.next).not.toHaveBeenCalled();

expect(MockedNextResponse.redirect).not.toHaveBeenCalled();

expect(MockedNextResponse.rewrite.mock.calls[0][0].toString()).toBe(

'http://localhost:3000/de-AT'

expect(MockedNextResponse.rewrite).not.toHaveBeenCalled();

expect(MockedNextResponse.next).not.toHaveBeenCalled();

expect(MockedNextResponse.redirect).toHaveBeenCalled();

expect(MockedNextResponse.redirect.mock.calls[0][0].toString()).toBe(

'http://localhost:3000/de-AT'

Ah yes, that makes perfect sense. I also noticed during my research that a redirect is returned if there is a mismatch in the locale - I got something mixed up when writing the tests 😄. I have now adjusted this according to your suggestions and the tests failed as expected, but after I undid the changes in resolveLocale, everything works. And as a result, only very few changes are necessary in the middleware 🙏🏼.

amannn · 2024-02-16T20:23:06Z

examples/example-app-router-playground/tests/main.spec.ts

+  const page = await context.newPage();
+
+  await page.goto('/DE');
+  await expect(page).toHaveURL('/DE');


Also, in the Playwright test I added, I noticed that the matcher toHaveURL still expects /DE as the URL.

See the comment below, that should help to fix this test too.

amannn · 2024-02-16T20:28:03Z

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

+        middlewareWithPathnames(createMockRequest('/de-at', 'de-AT'));
+        expect(MockedNextResponse.rewrite).toHaveBeenCalled();
+        expect(MockedNextResponse.next).not.toHaveBeenCalled();
+        expect(MockedNextResponse.redirect).not.toHaveBeenCalled();
+        expect(MockedNextResponse.rewrite.mock.calls[0][0].toString()).toBe(
+          'http://localhost:3000/de-AT'
+        );


Suggested change

middlewareWithPathnames(createMockRequest('/de-at', 'de-AT'));

expect(MockedNextResponse.rewrite).toHaveBeenCalled();

expect(MockedNextResponse.next).not.toHaveBeenCalled();

expect(MockedNextResponse.redirect).not.toHaveBeenCalled();

expect(MockedNextResponse.rewrite.mock.calls[0][0].toString()).toBe(

'http://localhost:3000/de-AT'

);

middlewareWithPathnames(createMockRequest('/de-at', 'de-AT'));

expect(MockedNextResponse.rewrite).not.toHaveBeenCalled();

expect(MockedNextResponse.next).not.toHaveBeenCalled();

expect(MockedNextResponse.redirect).toHaveBeenCalled();

expect(MockedNextResponse.redirect.mock.calls[0][0].toString()).toBe(

'http://localhost:3000/de-AT'

);

fkapsahili · 2024-02-17T09:58:25Z

Let me know if there is anything to improve or if I have overlooked something! 🙂

amannn

Thanks for the update, this already looks pretty good!

only very few changes are necessary in the middleware

These are the best PRs, a few new tests and a minimal change!

Would you mind adding these tests too:

Redirects for nested paths (e.g. /about) for the default and secondary locales
Redirects for localePrefix: 'as-necessary' for a secondary locale
Redirects for nested paths when using non-localized pathnames (default and secondary locales)

Btw. my apologies if the middleware code is a bit hard to follow, I'd really like to clean it up into more grouped code paths at some point. I think for now it's important to have a comprehensive test suite to rely on, this would enable a potential refactor at some point.

I undid the changes in resolveLocale

I really appreciate how careful you are with changes in PRs 🙏

fkapsahili · 2024-02-19T17:45:39Z

Thanks for the review! Also thanks for your tip, I noticed that nested paths were still rewritten for the default locale - I have now fixed this with a small change in getKnownLocaleFromPathname so that the redirect works in all cases.
I also added a few more Playwright tests to make sure that all redirects work correctly in the browser. The test cases you mentioned should also all be covered now. I assumed that by as-necessary you meant the as-needed strategy in the middleware 🙂.

…cept-language`

amannn · 2024-02-20T08:39:20Z

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

-  const pathLocale = locales.includes(pathLocaleCandidate)
+  const pathLocale = locales.find(
+    (locale) => locale.toLowerCase() === pathLocaleCandidate.toLowerCase()
+  )


The value returned from this function is assigned to a variable here:

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

Line 137 in 5cc264d

const pathLocale = getKnownLocaleFromPathname(

… and since it doesn't equal the detected locale at

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

Line 219 in 5cc264d

} else if (pathLocale === locale) {

… a redirect is invoked:

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

Line 246 in 5cc264d

response = redirect(`/${locale}${normalizedPathnameWithSearch}`);

That is correct.

However, the locale that is returned from resolveLocale here:

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

Line 47 in 5cc264d

const {domain, locale} = resolveLocale(

… doesn't pick up the prefix since it used to be case senstive.

Since we redirect based on locale (and not pathLocale), I noticed that that request like createMockRequest('/EN', 'de') in the test would redirect to /de.

I've added a small fix in f05395b that uses the locale from the pathname instead in this case—hope this is ok for you!

amannn

Thanks for adding more tests, this looks really solid to me now! 💯

I'll just wait for the tests to pass …

I assumed that by as-necessary you meant the as-needed strategy in the middleware 🙂.

Yeah, definitely—I keep confusing these two 🤦‍♂️. In retrospect, maybe it should have been called non-default 🙂

Many thanks again for this super clean PR! 👏👏

vercel bot deployed to Preview – next-intl-example-app-router February 15, 2024 07:49 View deployment

vercel bot deployed to Preview – next-intl-example-app-router February 15, 2024 13:34 View deployment

amannn reviewed Feb 16, 2024

View reviewed changes

vercel bot deployed to Preview – next-intl-example-app-router February 17, 2024 09:51 View deployment

vercel bot deployed to Preview – next-intl-docs February 17, 2024 10:22 View deployment

amannn reviewed Feb 17, 2024

View reviewed changes

fkapsahili force-pushed the fix/case-insensitive-locales branch from 4680973 to 02d7dbe Compare February 19, 2024 17:14

vercel bot deployed to Preview – next-intl-example-app-router February 19, 2024 17:16 View deployment

fkapsahili added 7 commits February 19, 2024 18:36

Make locale handling case insensitive

a278f7a

Add playwright test for invalid cased locale handling

1668aea

Refactor locale matching

3b1a859

Increase middleware size limit

d736b3b

Make sure a redirect is returned for case mismatches in the locale

1cb9304

Handle invalid cased default locales and add more middleware tests

709327c

Add playwright tests for redirecting invalid cased locales

9542deb

fkapsahili force-pushed the fix/case-insensitive-locales branch from c707400 to 9542deb Compare February 19, 2024 17:36

vercel bot deployed to Preview – next-intl-example-app-router February 19, 2024 17:38 View deployment

vercel bot deployed to Preview – next-intl-docs February 20, 2024 07:07 View deployment

Use locale from pathname for redirects instead of a fallback from `ac…

f05395b

…cept-language`

vercel bot deployed to Preview – next-intl-example-app-router February 20, 2024 08:37 View deployment

amannn reviewed Feb 20, 2024

View reviewed changes

vercel bot deployed to Preview – next-intl-docs February 20, 2024 08:41 View deployment

amannn approved these changes Feb 20, 2024

View reviewed changes

Bump middleware size

1a337a4

vercel bot deployed to Preview – next-intl-example-app-router February 20, 2024 08:46 View deployment

amannn changed the title ~~feat: Make locales case-insensitive and allow automatic resolving to the URL with the casing provided in the locales property~~ feat: Add redirects for case mismatches in locale prefixes (e.g. /en-us → /en-US) Feb 20, 2024

amannn changed the title ~~feat: Add redirects for case mismatches in locale prefixes (e.g. /en-us → /en-US)~~ feat: Add redirects for case mismatches in locale prefixes (e.g. /EN → /en) Feb 20, 2024

vercel bot deployed to Preview – next-intl-docs February 20, 2024 08:49 View deployment

amannn merged commit 3b2b446 into amannn:main Feb 20, 2024
7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add redirects for case mismatches in locale prefixes (e.g. `/EN` → `/en`) #861

feat: Add redirects for case mismatches in locale prefixes (e.g. `/EN` → `/en`) #861

fkapsahili commented Feb 15, 2024 •

edited

vercel bot commented Feb 15, 2024 •

edited

vercel bot commented Feb 15, 2024

amannn left a comment

amannn Feb 16, 2024

fkapsahili Feb 17, 2024

amannn Feb 16, 2024

amannn Feb 16, 2024

fkapsahili commented Feb 17, 2024

amannn left a comment

fkapsahili commented Feb 19, 2024

amannn Feb 20, 2024

amannn left a comment

feat: Add redirects for case mismatches in locale prefixes (e.g. /EN → /en) #861

feat: Add redirects for case mismatches in locale prefixes (e.g. /EN → /en) #861

Conversation

fkapsahili commented Feb 15, 2024 • edited

Prerequisites

Content

Tests

vercel bot commented Feb 15, 2024 • edited

vercel bot commented Feb 15, 2024

amannn left a comment

Choose a reason for hiding this comment

amannn Feb 16, 2024

Choose a reason for hiding this comment

fkapsahili Feb 17, 2024

Choose a reason for hiding this comment

amannn Feb 16, 2024

Choose a reason for hiding this comment

amannn Feb 16, 2024

Choose a reason for hiding this comment

fkapsahili commented Feb 17, 2024

amannn left a comment

Choose a reason for hiding this comment

fkapsahili commented Feb 19, 2024

amannn Feb 20, 2024

Choose a reason for hiding this comment

amannn left a comment

Choose a reason for hiding this comment

feat: Add redirects for case mismatches in locale prefixes (e.g. `/EN` → `/en`) #861

feat: Add redirects for case mismatches in locale prefixes (e.g. `/EN` → `/en`) #861

fkapsahili commented Feb 15, 2024 •

edited

vercel bot commented Feb 15, 2024 •

edited