feat: Automatically prefer localized pathnames that are more specific

[fkapsahili]

This PR fixes the current behavior of the internal route handling of the next-intl middleware that does not prioritize static route paths over dynamic routes. This fix makes sure that static routes (e.g. /products/new) are preferred over dynamic routes such as /products/[slug].

To make sure the current behavior does not lead to any unexpected issues, I thought it would be good if next-intl would prioritize the specified pathnames in the following order:

1. static pathnames, e.g. /products/new
2. dynamic routes (non-catch-all), e.g. /products/[slug]
3. catch-all routes, e.g. /products/[...slug] or optional catch-all routes, e.g. /products/[[...slug]]

Prerequisites

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

Content

• Added getSortedPathnames to handle the sorting of the specified pathnames in the middleware
• Updated the docs to make sure the changes are reflected in the documentation

Tests

• Added tests to the middleware that verify that static routes are prioritized over dynamic routes and catch-all routes
• Added tests to the middleware utils to make sure that getSortedPathnames handles the route prioritization as intended
• Added playwright tests to the example-app-router-playground to make sure the route prioritization works as intended

Closes #913

[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	Apr 26, 2024 0:04am
next-intl-example-app-router	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Apr 26, 2024 0:04am

[vercel]

@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]

Many thanks for looking into this and another great PR @fkapsahili! 👏👏

On a first glance, this looks like just the right implementation. I have a somewhat busy week ahead, but I'll likely be able to take a thorough look on Friday—hope that works for you!

I'll be in touch! 🙌

[fkapsahili]

Many thanks for looking into this and another great PR @fkapsahili! 👏👏

On a first glance, this looks like just the right implementation. I have a somewhat busy week ahead, but I'll likely be able to take a thorough look on Friday—hope that works for you!

I'll be in touch! 🙌

Thanks for the feedback! Yes of course, that works well for me 🙂. The tests are all working now, so I'm setting the PR to ready for review 😄.

[amannn]

Diving a bit deeper into this, I'm not sure these flags are sufficient to be honest.

E.g. consider this case:

/articles/[category]/[articleSlug]
/articles/[category]/just-in

In this case, the routes would be considered equal in regard to priority, and probably the natural ordering would be preserved. However, the second route is more specific and should match.

The number of dynamic segments could be another indicator, but I think this won't solve the problem cleanly as e.g. /foo/[d1] and /foo/bar/[d1] have the same number of segments.

It seems like to be able to tell which route is really more specific, we have to consider routes in a hierarchical way, e.g. find the most specific route in /articles, then try to find the most specific route in ./[category], etc.

Do you by chance know if Next.js handles these cases correctly? Maybe we can take some inspiration from them.

What do you think?

[amannn]

Or maybe the number of dynamic segments could in fact suffice? 🤔

In the example /foo/[d1] and /foo/bar/[d1], the routes have the same priority, but only one of them matches eventually—so maybe this isn't an issue.

Do you think you could a few more tests for edge cases so we can gain some clarity about those?

[fkapsahili]

Ah yes, thanks for the hint! You were absolutely right, my solution didn't take into account the case where we have nested dynamic routes, e.g: /articles/[category]/[slug] and a static route in the same place 😅.

According to my tests, Next.js handles these cases correctly but I'm not very familiar with the Next.js repo itself, but at first glance I believe the code you referenced in the corresponding issue is actually where Next does the route sorting:
https://github.com/vercel/next.js/blob/415cd74b9a220b6f50da64da68c13043e9b02995/packages/next/src/shared/lib/router/utils/sorted-routes.ts

When looking at Next's code I realized that it is necessary that the individual route segments are compared with each other and have now implemented this additionally so that the failed tests that I added at the beginning of this update now pass.

If there are any other edge cases you can think of, I'm happy to add more tests! 🙂

[amannn]

Great refactor, this looks really solid to me and I think this is the right approach 👏.

I've left some inline comments about minor nits, but I also think there might be a chance to simplify this function slightly.

Let me know what you think! 🙌

[amannn]

Hmm, what is your intention with getExternalPath here? I don't really understand why we need to extract firstLocalePath. It seems a bit odd that we take the first localized pathname. Why don't we just use the internal route name?

Maybe to take a step back: I would assume that users use localized pathnames to localize individual segments, but the number and also the name of dynamic segments should always match. Maybe some validation could help in regard to that to avoid typos (e.g. '/[category]': '/[categry]', but generally, in the context of sorting routes, we should only have to consider the internal pathname I think.

Furthermore, I'm not sure we need to run .filter—did you have a specific case for this in mind?

Considering these two points, I think we could simplify to this:

Suggested change

	const pathA = getExternalPath(a[1]).split('/').filter(Boolean);
	const pathB = getExternalPath(b[1]).split('/').filter(Boolean);
	const pathA = a[0].split('/');
	const pathB = b[0].split('/');

Based on this, if we only consider the internal pathname, maybe we could simplify the sort function a bit:

expect(
  getSortedPathnames(
    ['/categories/[slug]', '/categories/new']
  )
).toEqual(
  ['/categories/new', '/categories/[slug]']
);

Might make the tests a bit more minimalistic.

[fkapsahili]

Ah yes, that makes sense! I think the getExternalPath comes from a first attempt where I tried to separate everything very strictly. Just taking the internal route name is a good idea! I'll try to clean that up a bit so we have an easier to read test suite and a more minimalistic approach in the utils 👍🏼.

[fkapsahili]

@amannn I have now simplified the sorting function of getSortedPathnames so that only the internal pathnames are passed as arguments, which makes the tests look a bit easier. Just let me know if there is anything I can improve!

[amannn]

Looks fantastic, a really cool quality-of-life improvement! 👏

I'll just add one commit with a minor renaming nit I spotted during the final review.