feat: When using localized pathnames, allow access to internal pathnames only if they match an entry from a particular locale—otherwise redirect

[RomainGueffier]

Fixes #904

Example:

createIntlMiddleware({
  locales: ['lv', 'ru'],
  defaultLocale: 'lv',
  pathnames: {
    '/': '/',
    '/about': {
      lv: '/par-mums',
      ru: '/o-nas',
    }
  }
});

In this case, the only available app routes are /lv, /ru, /lv/par-mums and /ru/o-nas.

If a request for /lv/about or /ru/about is initiated, a 302 redirect to the localized alternative will now be returned.

If en: '/about' is added to pathnames, then the internal pathname becomes available—but only for this particular locale.

This PR furthermore includes a minor fix for the case where multiple localized pathnames share the same entry, but there are still variations:

'/about': {
  'en-UK': '/about',
  'en-US': '/about',
  'lv': '/par-mums'
}

Previously, /en-US/about could incorrectly redirect to /en-UK/about. Now, this is fixed.

[vercel]

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	Mar 25, 2024 9:25am
next-intl-example-app-router	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Mar 25, 2024 9:25am

[vercel]

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

A member of the Team first needs to authorize it.

[amannn]

Hey @RomainGueffier,

thank you so much for this PR! 🙏

I currently have quite a full schedule and need to prioritize a little bit. Just wanted to let you know that I have this PR on my list, I'll provide a review here as soon I find time for it!

Thanks again and have a good weekend! 🙌

[amannn]

Hey @RomainGueffier,

I've finally found time to look into this, thank you for your patience! While reviewing the code, I've cleaned up some things along the way and added a few more tests, but I think everything should be fine now.

Let's see if CI passes …

[amannn]

Yep, seems like all tests pass on CI! 🙌

@RomainGueffier would you mind having another look from your side at the code to see if everything matches your understanding?

[RomainGueffier]

Hello @amannn thanks for the feedback 😃 and little refactoring of my tests ! Looks good.
I would like to address these questions I had initially ⬇️

To discuss:

• why not using 301 redirections or 404
• how to test internal pathname not inside Next Intl config, not found, etc. to avoid breaking Nextjs default features
• code quality and refactoring to make code more readdable
• add more tests (catch-all paths, etc.)
• documentation
• update exemples with internal pathnames

I feel like my code is a bit noisy, maybe I could clarify it in dedicated functions ?
What about using 301 instead of 302 redirections ?
Also I did not run a real next js application to try out modifications and internal paths not covered by next intl config, such as sitemap, robots, or other ones. Should I add more paths into exemples?

[amannn]

I feel like my code is a bit noisy, maybe I could clarify it in dedicated functions ?

I think for the time being it's ok. I'd eventually like to do a cleanup pass on the whole middleware logic to split it into chunks, but I'm waiting a bit with this currently since it's a bigger task and I think there are currently other priorities.

What about using 301 instead of 302 redirections ?

I think 301 is a bit more risky since it can be cached. What we're handling here should be an edge case and not a URL that is available in a sitemap for example. I think sticking with 302 can be helpful in case a user adds a localized route that matches the internal one and this route suddenly becomes valid.

Also I did not run a real next js application to try out modifications

I ran the example in the repo and the result looked good to me!

internal paths not covered by next intl config, such as sitemap, robots, or other ones. Should I add more paths into exemples?

Maybe another test where a request is invoked that matches neither an internal nor an external pathname could be a good idea, yes! I think it should work as expected, but better safe than sorry :). So definitely add one if you feel like it!

---

Let me know what you think!

[RomainGueffier]

Ok sounds good!

Maybe another test where a request is invoked that matches neither an internal nor an external pathname could be a good idea, yes! I think it should work as expected, but better safe than sorry :). So definitely add one if you feel like it!

I tried to write a test, but it always rewrites or redirects to locale:

it('metadata paths should not redirect to a locale', () => {
      middlewareWithPathnames(
        createMockRequest('/sitemap.xml') // seems to consider this path as a internal route.
      );
      expect(MockedNextResponse.next).not.toHaveBeenCalled();
      expect(MockedNextResponse.rewrite).not.toHaveBeenCalled();
      expect(MockedNextResponse.redirect).not.toHaveBeenCalled();
      expect(MockedNextResponse.[??].toBe(
        'http://localhost:3000/sitemap.xml'
      );
});

The mock function does not seems to allow to configure an i18n layout further down the tree or support the next js metadata paths such as sitemap.xml or robots.txt. Do you have an idea? I'm ready for merge if this is complicated and not a critical test.

[amannn]

@RomainGueffier A rewrite being triggered in this case is correct. But for the /sitemap.xml request the middleware shouldn't run in the first place, based on the matcher config.

I think this is good to go then!

[amannn]

Many thanks for this PR @RomainGueffier! 👏👏