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

feat: i18n locale redirects support #227

Merged
merged 8 commits into from
May 8, 2023

Conversation

james-elicx
Copy link
Contributor

This PR does the following:

  • Fixes an issue where redirects weren't breaking out of routing at the right point.
  • Adds a utility function for parsing the accept-language header value.
  • Introduces support for the source route locale option and the next.config.js option for i18n.
  • Adds tests for the new functionality.

It is somewhat of a continuation of #222 as that fixed a major blocker for i18n.

So, there is a little quirk with how the locale redirects have to be handled. There are two different types of ways internationalization works. These are sub-path routing and domain routing. The build output config route.src value for sub-path routing is simply /, which causes issues.

The Next.js docs state that automatic locale detection occurs when a user visits the root of the website, which would mean that the regex should be ^/$, except it isn't. Therefore, we have to add custom behaviour to check whether the route.src is a regex, and if it isn't, check that it is equal to the current path before determining whether we should apply the locale redirects.

Sub-path routing entry:

{
	src: '/',
	locale: {
		redirect: { en: '/', fr: '/fr', nl: '/nl', es: '/es' },
		cookie: 'NEXT_LOCALE',
	},
	continue: true,
}

Domain routing entry:

{
	src: '^//?(?:en|fr|nl|es)?/?$',
	locale: {
		redirect: { es: 'https://example.es/' },
		cookie: 'NEXT_LOCALE',
	},
	continue: true,
}

As you can see, the two values for route.src are very different. I find this to be rather bizarre behaviour, and to me, it looks like the Vercel build output isn't generating the correct matching patterns. Anyway, so to address the quirks, I have done what I detailed above.

Also tested it using test apps for plain Next.js i18n, next-translate, and next-i18n. The test app shouldn't actually matter anyway, because they all use next.config.js i18n under the hood, which is what this PR introduces better support for.

closes #28
closes #37
closes #157

@changeset-bot
Copy link

changeset-bot bot commented May 8, 2023

🦋 Changeset detected

Latest commit: 956c38e

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@cloudflare/next-on-pages Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions
Copy link
Contributor

github-actions bot commented May 8, 2023

🧪 A prerelease is available for testing 🧪

You can install this latest build in your project with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/next-on-pages/runs/4920020835/npm-package-next-on-pages-227

Or you can immediately run this with npx:

npx https://prerelease-registry.devprod.cloudflare.dev/next-on-pages/runs/4920020835/npm-package-next-on-pages-227

@james-elicx
Copy link
Contributor Author

james-elicx commented May 8, 2023

We identified an issue where there is a record in the miss phase that rewrites all requests containing the default locale to /.

To prevent this behaviour that we don't want, we modify the source route's src regex to be locale friendly using any of the previously found locales.

Problematic record example:

{
  "src": "/en",
  "dest": "/",
  "check": true
},

This should instead have an src value of ^/en$, so that it only remaps paths for exactly the default locales to /.

Copy link
Member

@dario-piotrowicz dario-piotrowicz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So awesome!!! thank you very much! ❤️

@dario-piotrowicz dario-piotrowicz merged commit 86df485 into cloudflare:main May 8, 2023
@james-elicx james-elicx deleted the james/i18n-support branch May 9, 2023 09:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants