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.

Sign up for GitHub

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

Jump to bottom

Re-add domain locale redirect handling #18274

Merged
merged 3 commits into from Oct 27, 2020
Merged

Re-add domain locale redirect handling #18274

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
431ad4b
Update domain locale handling to re-add restricing locales to domain
ijjk Oct 26, 2020
b6fe1c0
Make sure to only redirect domain on locale root
ijjk Oct 26, 2020
e14ea58
Merge branch 'canary' into i18n/domain-locale-redirecting
kodiakhq[bot] Oct 27, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
30 changes: 13 additions & 17 deletions packages/next/build/webpack/loaders/next-serverless-loader.ts
View file
Open in desktop
Expand Up @@ -262,33 +262,29 @@ const nextServerlessLoader: loader.Loader = function () {
...parsedUrl,
pathname: localePathResult.pathname,
})
req.__nextStrippedLocale = true
parsedUrl.pathname = localePathResult.pathname
}

// If a detected locale is a domain specific locale and we aren't already
// on that domain and path prefix redirect to it to prevent duplicate
// content from multiple domains
if (detectedDomain) {
const localeToCheck = localePathResult.detectedLocale
? detectedLocale
: acceptPreferredLocale

// check if the locale prefix matches a domain's defaultLocale
// and we're on a locale specific domain if so redirect to that domain
// if (detectedDomain) {
// const matchedDomain = detectDomainLocale(
// i18n.domains,
// undefined,
// detectedLocale
// )

// if (matchedDomain) {
// localeDomainRedirect = \`http\${
// matchedDomain.http ? '' : 's'
// }://\${matchedDomain.domain}\`
// }
// }
} else if (detectedDomain) {
const matchedDomain = detectDomainLocale(
i18n.domains,
undefined,
acceptPreferredLocale
localeToCheck
)

if (matchedDomain && matchedDomain.domain !== detectedDomain.domain) {
localeDomainRedirect = \`http\${matchedDomain.http ? '' : 's'}://\${
matchedDomain.domain
}/\${
localeToCheck === matchedDomain.defaultLocale ? '' : localeToCheck
}\`
}
}
Expand Down
9 changes: 8 additions & 1 deletion packages/next/next-server/lib/i18n/detect-domain-locale.ts
View file
Open in desktop
Expand Up @@ -3,6 +3,7 @@ export function detectDomainLocale(
| Array<{
http?: boolean
domain: string
locales?: string[]
defaultLocale: string
}>
| undefined,
Expand All @@ -13,17 +14,23 @@ export function detectDomainLocale(
| {
http?: boolean
domain: string
locales?: string[]
defaultLocale: string
}
| undefined

if (domainItems) {
if (detectedLocale) {
detectedLocale = detectedLocale.toLowerCase()
}

for (const item of domainItems) {
// remove port if present
const domainHostname = item.domain?.split(':')[0].toLowerCase()
if (
hostname === domainHostname ||
detectedLocale?.toLowerCase() === item.defaultLocale.toLowerCase()
detectedLocale === item.defaultLocale.toLowerCase() ||
item.locales?.some((locale) => locale.toLowerCase() === detectedLocale)
) {
domainItem = item
break
Expand Down
21 changes: 20 additions & 1 deletion packages/next/next-server/server/config.ts
View file
Open in desktop
Expand Up @@ -340,7 +340,26 @@ function assignDefaults(userConfig: { [key: string]: any }) {
if (!item.defaultLocale) return true
if (!item.domain || typeof item.domain !== 'string') return true

return false
let hasInvalidLocale = false

if (Array.isArray(item.locales)) {
for (const locale of item.locales) {
if (typeof locale !== 'string') hasInvalidLocale = true

for (const domainItem of i18n.domains) {
if (domainItem === item) continue
if (domainItem.locales && domainItem.locales.includes(locale)) {
console.warn(
`Both ${item.domain} and ${domainItem.domain} configured the locale (${locale}) but only one can. Remove it from one i18n.domains config to continue`
)
hasInvalidLocale = true
break
Copy link

Choose a reason for hiding this comment

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

@Timer What if we want to have 2 domains translated in the same language?
Exampe:

example.es 
  defaultLocale: 'es'
  locales: 'en'
example.fr 
  defaultLocale: 'fr'
  locales: 'en'

Is this possible?

Copy link

Choose a reason for hiding this comment

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

Another example more realistic. What if we have two domains for Belgium and Switzerland. In both countries French is a spoken language and we want to offer the content in french in example.ch and example.be.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hi, if you want the domain to be available for multiple domains you can not configure it for any domains and only in the main locales array and it will fallback to path based locale detection. Note: this can potentially negatively affect SEO since it can register as duplicate content on separate domains

Copy link

Choose a reason for hiding this comment

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

Hi @ijjk I opened an issue as recommended by @Timer :) #21892

}
}
}
}

return hasInvalidLocale
})

if (invalidDomainItems.length > 0) {
Expand Down
36 changes: 18 additions & 18 deletions packages/next/next-server/server/next-server.ts
View file
Open in desktop
Expand Up @@ -342,32 +342,32 @@ export default class Server {
})
;(req as any).__nextStrippedLocale = true
parsedUrl.pathname = localePathResult.pathname
}

// If a detected locale is a domain specific locale and we aren't already
// on that domain and path prefix redirect to it to prevent duplicate
// content from multiple domains
if (detectedDomain && parsedUrl.pathname === '/') {
const localeToCheck = acceptPreferredLocale
// const localeToCheck = localePathResult.detectedLocale
// ? detectedLocale
// : acceptPreferredLocale

// check if the locale prefix matches a domain's defaultLocale
// and we're on a locale specific domain if so redirect to that domain
// if (detectedDomain) {
// const matchedDomain = detectDomainLocale(
// i18n.domains,
// undefined,
// detectedLocale
// )

// if (matchedDomain) {
// localeDomainRedirect = `http${matchedDomain.http ? '' : 's'}://${
// matchedDomain?.domain
// }`
// }
// }
} else if (detectedDomain) {
const matchedDomain = detectDomainLocale(
i18n.domains,
undefined,
acceptPreferredLocale
localeToCheck
)

if (matchedDomain && matchedDomain.domain !== detectedDomain.domain) {
if (
matchedDomain &&
(matchedDomain.domain !== detectedDomain.domain ||
localeToCheck !== matchedDomain.defaultLocale)
) {
localeDomainRedirect = `http${matchedDomain.http ? '' : 's'}://${
matchedDomain.domain
}/${
localeToCheck === matchedDomain.defaultLocale ? '' : localeToCheck
}`
}
}
Expand Down
2 changes: 2 additions & 0 deletions test/integration/i18n-support/next.config.js
View file
Open in desktop
Expand Up @@ -11,11 +11,13 @@ module.exports = {
http: true,
domain: 'example.be',
defaultLocale: 'nl-BE',
locales: ['nl', 'nl-NL', 'nl-BE'],
},
{
http: true,
domain: 'example.fr',
defaultLocale: 'fr',
locales: ['fr-BE'],
},
],
},
Expand Down
76 changes: 55 additions & 21 deletions test/integration/i18n-support/test/index.test.js
View file
Open in desktop
Expand Up @@ -52,11 +52,13 @@ function runTests(isDev) {
http: true,
domain: 'example.be',
defaultLocale: 'nl-BE',
locales: ['nl', 'nl-NL', 'nl-BE'],
},
{
http: true,
domain: 'example.fr',
defaultLocale: 'fr',
locales: ['fr-BE'],
},
],
})
Expand Down Expand Up @@ -661,27 +663,56 @@ function runTests(isDev) {
})

it('should handle locales with domain', async () => {
const checkDomainLocales = async (domainDefault = '', domain = '') => {
for (const locale of locales) {
// skip other domains' default locale since we redirect these
if (['fr', 'nl-BE'].includes(locale) && locale !== domainDefault) {
continue
}

const res = await fetchViaHTTP(
appPort,
`/${locale === domainDefault ? '' : locale}`,
undefined,
{
headers: {
host: domain,
},
redirect: 'manual',
}
)
const domainItems = [
{
// used for testing, this should not be needed in most cases
// as production domains should always use https
http: true,
domain: 'example.be',
defaultLocale: 'nl-BE',
locales: ['nl', 'nl-NL', 'nl-BE'],
},
{
http: true,
domain: 'example.fr',
defaultLocale: 'fr',
locales: ['fr-BE'],
},
]
const domainLocales = domainItems.reduce((prev, cur) => {
return [...prev, ...cur.locales]
}, [])

const checkDomainLocales = async (
domainDefault = '',
domain = '',
locale = ''
) => {
const res = await fetchViaHTTP(appPort, `/`, undefined, {
headers: {
host: domain,
'accept-language': locale,
},
redirect: 'manual',
})
const expectedDomainItem = domainItems.find(
(item) => item.defaultLocale === locale || item.locales.includes(locale)
)
const shouldRedirect =
expectedDomainItem.domain !== domain ||
locale !== expectedDomainItem.defaultLocale

expect(res.status).toBe(200)
expect(res.status).toBe(shouldRedirect ? 307 : 200)

if (shouldRedirect) {
const parsedUrl = url.parse(res.headers.get('location'), true)

expect(parsedUrl.pathname).toBe(
`/${expectedDomainItem.defaultLocale === locale ? '' : locale}`
)
expect(parsedUrl.query).toEqual({})
expect(parsedUrl.hostname).toBe(expectedDomainItem.domain)
} else {
const html = await res.text()
const $ = cheerio.load(html)

Expand All @@ -691,8 +722,11 @@ function runTests(isDev) {
}
}

await checkDomainLocales('nl-BE', 'example.be')
await checkDomainLocales('fr', 'example.fr')
for (const item of domainItems) {
for (const locale of domainLocales) {
await checkDomainLocales(item.defaultLocale, item.domain, locale)
}
}
})

it('should generate AMP pages with all locales', async () => {
Expand Down