feat: add multiDomainLocales: { isolate: true } for regional brand domains - #4101
feat: add multiDomainLocales: { isolate: true } for regional brand domains#4101Vincentdevreede wants to merge 2 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe change adds Estimated code review effort: 4 (Complex) | ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
src/runtime/plugins/i18n.ts (1)
63-70: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant
useRequestURLcall — reuse the already-computedhost.
hostis already computed once at line 31 in this samesetup()closure; recomputing it ascurrentHostinside thecomposer.localescomputed getter is redundant and could theoretically diverge ifuseRequestURL()were ever called from a different context than at setup time.♻️ Proposed simplification
composer.locales = computed(() => { const locales = runtimeI18n.locales.map(locale => withRuntimeDomain(locale, runtimeI18n.domainLocales)) if (!__I18N_ISOLATE_MULTIDOMAINLOCALES__) { return locales } - const currentHost = useRequestURL({ xForwardedHost: true }).host return locales.filter( - l => typeof l === 'string' || isLocaleServedOnHost(locales as NormalizedLocaleObject[], currentHost, l.code), + l => typeof l === 'string' || isLocaleServedOnHost(locales as NormalizedLocaleObject[], host, l.code), ) })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/runtime/plugins/i18n.ts` around lines 63 - 70, Update the composer.locales computed getter to reuse the existing host value from the setup() closure instead of calling useRequestURL({ xForwardedHost: true }) again or defining currentHost. Keep the existing locale filtering behavior unchanged.src/runtime/routing/domain.ts (1)
31-44: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winIsolate pruning logic checks out.
The new branch correctly runs before the
usesDefaultVariantearly exit, so pruning applies to all route-localization strategies before domain default variants are adjusted. Coverage for isolate withprefix/no_prefixwould still be useful for edge cases like same-path, differently-named routes, but the current implementation behavior is sound for the testedprefix_except_defaultpaths.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/runtime/routing/domain.ts` around lines 31 - 44, The isolate pruning logic in the route iteration is correct; no implementation changes are required. Preserve the branch ordering before the usesDefaultVariant early exit and retain the existing route removal behavior for locales not served on the host.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/content/docs/02.guide/10.multi-domain-locales.md`:
- Line 13: Correct the misspelled configuration option in the multi-domain
locales documentation: update `multiDomainLocaes` to `multiDomainLocales` in the
`isolate` caveat while preserving the surrounding guidance unchanged.
- Line 202: Update the multi-domain configuration example to use
multiDomainLocales: true instead of the isolated object form, preserving the
cross-domain redirects and off-domain locale links; leave the later isolated
example unchanged.
In `@docs/content/docs/04.api/00.options.md`:
- Line 356: Update the option documentation to describe locale `domains` and
`defaultForDomains` as optional fields, while retaining the array-of-objects
requirement and multi-domain locales reference. Clarify that locales without
`domains` are served on all configured domains and avoid wording that requires
either field.
In `@specs/multi_domains_locales/multi_domains_locales_redirect_scope.spec.ts`:
- Around line 44-49: The redirect test should follow the redirect chain before
asserting host locality. Update the test “an ambiguous domain with no signal
redirects within its own host, not to defaultLocale’s domain” to assert the
final destination after `/en`, or configure brand-c with a default locale and
assert that local route instead of validating only the first-hop location.
In `@src/runtime/server/plugin.ts`:
- Around line 147-149: Update the initialLocale condition in the server request
handling flow to suppress locale detection only when the current request is an
actual root redirect, rather than whenever rootRedirect is configured. Preserve
browser-detected cross-domain relocation for non-root paths, including /about,
while retaining the existing redirectOn behavior.
In `@src/runtime/shared/detection.ts`:
- Around line 155-156: Update the detection flow around the
detectors.cookie/header/navigator chain so it evaluates each source in order and
returns the first locale that passes isSupported, rather than stopping at an
unsupported truthy cookie. Preserve undefined when no detector yields a
supported locale, and add a regression case covering an unsupported stale cookie
followed by a supported header locale.
---
Nitpick comments:
In `@src/runtime/plugins/i18n.ts`:
- Around line 63-70: Update the composer.locales computed getter to reuse the
existing host value from the setup() closure instead of calling useRequestURL({
xForwardedHost: true }) again or defining currentHost. Keep the existing locale
filtering behavior unchanged.
In `@src/runtime/routing/domain.ts`:
- Around line 31-44: The isolate pruning logic in the route iteration is
correct; no implementation changes are required. Preserve the branch ordering
before the usesDefaultVariant early exit and retain the existing route removal
behavior for locales not served on the host.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: af4af9a6-1647-4ce8-b5f1-c1e9b7aa6abf
📒 Files selected for processing (23)
docs/content/docs/02.guide/09.different-domains.mddocs/content/docs/02.guide/10.multi-domain-locales.mddocs/content/docs/04.api/00.options.mdspecs/different_domains/different_domains_multi_locales_prefix_except_default.spec.tsspecs/multi_domains_locales/multi_domains_locales_isolate.spec.tsspecs/multi_domains_locales/multi_domains_locales_multi_locales.spec.tsspecs/multi_domains_locales/multi_domains_locales_redirect_scope.spec.tsspecs/multi_domains_locales/multi_domains_locales_root_redirect_scope.spec.tssrc/bundler.tssrc/env.d.tssrc/pages.tssrc/runtime/plugins/i18n.tssrc/runtime/routing/domain.tssrc/runtime/server/plugin.tssrc/runtime/server/utils/redirect.tssrc/runtime/shared/detection.tssrc/types.tstest/detection.test.tstest/kit.test.tstest/pages/localize_routes.test.tstest/redirect.test.tstest/routing-head.test.tstest/setup.ts
| test('an ambiguous domain with no signal redirects within its own host, not to `defaultLocale`\'s domain', async () => { | ||
| const res = await undiciRequest('/', { headers: { Host: 'brand-c.nuxt-app.localhost' } }) | ||
|
|
||
| expect(res.statusCode).toBe(302) | ||
| expect(res.headers.location).toBe('/en') | ||
| }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Follow the redirect chain before claiming the visit stays host-local.
This test only checks the first hop to /en; Lines 51-56 establish that /en on brand-c then relocates to brand-a. Assert the final destination, or configure a brand-c default locale and assert that local route instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@specs/multi_domains_locales/multi_domains_locales_redirect_scope.spec.ts`
around lines 44 - 49, The redirect test should follow the redirect chain before
asserting host locality. Update the test “an ambiguous domain with no signal
redirects within its own host, not to defaultLocale’s domain” to assert the
final destination after `/en`, or configure brand-c with a default locale and
assert that local route instead of validating only the first-hop location.
|
Thanks for the PR! Same as with #4080 this covers a few behaviors at once, could you split the fixes from the feature? As far as I can tell only one part of this fixes behavior that's expected today, the rest changes or adds behavior, and that's hard to prioritize in one PR. From what I can tell the detection change isn't restoring behavior we broke. I think #4083 may have introduced a regression, Could you open Could you also check this against the edge release ( |
|
@BobbieGoede I think the detection change is restoring behavior that is broken along the way. I can make a separate PR for the isolate feature because it was all in 1 commit, but i don't think it makes much sense if cross domain will be removed anyway? Second commit is the cross domain in combination with the isolation feature and the other commits where just Coderabbit suggestions that it made. Also, I wasn’t running a released version of i18n. I had checked out the |
|
@BobbieGoede Okay i did some research. History of crossdomain redirects2022-11-03 - #1606, "fix: different domain loop redirection" if (process.client) {
if (state.value !== redirectPath) {
state.value = ''
window.location.assign(redirectPath)
}
} else if (process.server) {
state.value = redirectPath
}
2023-08-15 - #2318, "fix: loop redirection on differentDomains" if (process.client) {
if (state.value && state.value !== redirectPath) {Now the client branch also needs 2023-09-13 - #2404, "fix: erratic redirection on different domains on server mode" if (state.value && state.value !== redirectPath) {
if (process.client) {
state.value = ''
window.location.assign(redirectPath)
} else if (process.server) {
state.value = redirectPath
}
}This is the one that actually kills it. The server-side setter is now behind the same 2023-12-25 - v8.0.0 ships 2025-05-20 - #3622, "refactor: remove dead code for different domains redirection" ConclusionThe intent was clearly that cross-origin redirects on detection should work for differentDomains, that's the only reason the useRedirectState / window.location.assign bypass exists at all, since Vue Router has no way to navigate across origins on its own. |
|
@Vincentdevreede thanks for the deep dive! 🙏 You're right, it seems that at least redirection/navigation was broken for a long time for domain features.. (#4105 restores the cross-domain redirect) And our tests covered less than I thought they did. I'm also not sure how this went unreported for such a long time 🤔 I also found out that one of the things that broke language switching between domains is because NuxtLink (since nuxt v3) adds We can deal with that with a modified A second issue is I suspect the redirect state tracking was added to work around the symptoms of both the I would like to get a release out this week (it includes a few fixes and some major perf improvements), but want to get The fixes and hardening of the domain features are on #4105, feel free to try out the preview release and report feedback 🙏 |
|
@BobbieGoede I see the new release is live! 👍 What about the isolation function, I think is a fair concern. The “different brands sharing a single build” example may have made the feature sound more unrelated to i18n than I intended. It was mainly the clearest example I could think of to explain the isolation behaviour in the documentation. The intended use case is not necessarily a set of completely unrelated websites. It can also be the same company or product operating under different names in different countries. A well-known example would be Lay’s, which is known as Walkers, Smith’s or Sabritas in different markets. Those websites may still share the same application, content structure and translations, but not every configured locale should be considered available on every domain. Even when the visitor’s language is technically available elsewhere in the build, you may not want to redirect them to another country’s domain or brand name. For example, a visitor on the Walkers website should not necessarily be redirected to the Lay’s or Smith’s domain just because their preferred language is available there. For that domain, the locale should instead be treated as unavailable. That is why I see this as part of the i18n domain functionality rather than only as a build optimization. multiDomainLocales already defines which locales belong to which domains. Isolation makes that relationship consistent by ensuring that locale detection, routing and navigation only use the locales that are actually available for the current domain. The shared build is useful for this setup, but it is not the main purpose of the feature. The main purpose is controlling locale availability per domain and preventing unintended navigation between different regional identities of the same product or organisation. |
7b55e96 to
4473427
Compare
|
@BobbieGoede I’ve updated my PR. I used my original PR as a reference and reworked the feature from scratch on top of v10.6.0. I no longer refer to different brands, but instead use the example of a single brand operating under different regional names. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/content/docs/02.guide/10.multi-domain-locales.md (1)
13-14: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse the standard spelling
referrer.Line 13 refers to referrer information, not the HTTP header field name. Change
referertoreferrerto avoid ambiguity.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/content/docs/02.guide/10.multi-domain-locales.md` around lines 13 - 14, Update the referrer-policy sentence in the multi-domain locale documentation to use the standard spelling “referrer” instead of “referer,” while leaving the HTTP/header behavior and surrounding guidance unchanged.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/runtime/plugins/i18n.ts`:
- Around line 74-79: Update the composer localeCodes derivation to use the same
host-filtered locales produced by the isolation-aware composer.locales logic, so
off-host codes are excluded when multidomain isolation is enabled while
preserving all codes otherwise. Add an isolation-mode assertion covering
composer.localeCodes and its exclusion of locales removed by pruneOffHostRoutes.
---
Outside diff comments:
In `@docs/content/docs/02.guide/10.multi-domain-locales.md`:
- Around line 13-14: Update the referrer-policy sentence in the multi-domain
locale documentation to use the standard spelling “referrer” instead of
“referer,” while leaving the HTTP/header behavior and surrounding guidance
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1a800fd4-f0f2-4e7a-9613-474373642c43
📒 Files selected for processing (17)
docs/content/docs/02.guide/10.multi-domain-locales.mddocs/content/docs/04.api/00.options.mdspecs/multi_domains_locales/multi_domains_locales_isolate.spec.tssrc/bundler.tssrc/env.d.tssrc/pages.tssrc/runtime/plugins/i18n.tssrc/runtime/routing/domain.tssrc/runtime/server/plugin.tssrc/runtime/server/utils/redirect.tssrc/runtime/shared/detection.tssrc/runtime/utils.tssrc/types.tstest/detection.test.tstest/kit.test.tstest/redirect.test.tstest/setup.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- test/setup.ts
- src/env.d.ts
- src/types.ts
- docs/content/docs/04.api/00.options.md
- src/pages.ts
- src/bundler.ts
Vincentdevreede
changed the title
|
Thank you for the further clarification 🙏 The intended use case is clear to me, and I think I managed to narrow my concerns about its implementation. Today the module assumes a project has a single identity: every configured locale is an alternate representation of the same content, and the config describes where those alternates live. Looking at the diff makes explicit what isolation indirectly does:
Within each selected sub-config the module's behavior is unchanged; everything new happens in the selection step. So what this feature really is, is configuration determined at runtime: which locales exist, which is the default, and how detection behaves would depend on the incoming request. (Not Nuxt's That puts it in a family with several open requests we've been collecting:
We're not opposed to some form of this, but it conflicts with how much of what the module produces is fixed at build time, and any real version of it has to confront these:
To be fair, the proposed isolation feature avoids most of these problems, precisely because it is still configured at build time: every per-host sub-config is known up front, and they all share the same routes, messages and pages. That constraint is what makes it much easier to implement than general runtime configuration. But it gets there by leaking the identity semantic into places that shouldn't carry it: the locale config now encodes which identity a locale belongs to, and detection, redirects, the switcher and the head alternates each have to interpret that meaning, instead of the config selection happening in one explicit step. And the shortcut only holds while the sub-configs share everything; the moment the identities need different pages or translations, the built resources disagree with the selected config. So rather than merging isolation as a flag that implicitly implements runtime config selection for one use case, I'd prefer to treat runtime-determined configuration as its own design question: decide which subset is honestly supportable given the constraints above, and state its boundaries up front. I'm keeping this use case on the table as part of that, but I don't want to land it in its current shape. |
Exercises the nuxt-modules#4101 use case in userland: one deployment hosting unrelated brands, each host serving only its own locale cluster. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Great feature, also waiting for this to be merged. |
3 participants
Summary
Adds
multiDomainLocales: { isolate: true }for the case where each domain sells the same product under its own regional brand rather than one brand serving every region. Each domain shouldbehave like a fully self-contained site. It should never redirect to or link into another domain's locale.
With
isolate: true:detectBrowserLanguage.cookieDomainspanning multiple domains never cross domains. A first visit detected as another domain's locale stays on the currentdomain in its own locale instead of relocating.
@BobbieGoede, feel free to accept this or not, and make any changes you see fit.
Summary by CodeRabbit
New Features
multiDomainLocales: { isolate: true }.Documentation
Tests
SummaryAddmultiDomainLocales: { isolate: true }for hosting genuinely unrelated brands on one shared build. A locale not served on the current domain now 404s instead of redirecting cross-domain, and the locale switcher and hreflang alternates only list locales actually served on the current domain.Revert part of upstream fix(domain): resolve locale availability from host membership #4083, which madedetectBrowserLanguageunconditionally stay within the current domain for every domain setup. That's the right call for{ isolate: true }, since unrelated brands must never reveal a connection, but it was wrong for plainmultiDomainLocales/differentDomains. The entire point ofdifferentDomainsis one product spanning several regional domains, so a detected locale should be able to send a visitor to the domain that actually serves it, exactly like an explicit locale-prefixed path already does. fix(domain): resolve locale availability from host membership #4083's actual bug was narrower than that: an adopted off-host locale built a broken path on the current domain instead of relocating anywhere. This restores the intended cross-domain behavior fordifferentDomainsand plainmultiDomainLocaleswithout reintroducing that original bug, and keeps{ isolate: true }exactly as strict as before.This is an older summary from before I force-pushed the changes. I’m just keeping it here for reference.