fix: add basePath and locale properties to NextURL#516
fix: add basePath and locale properties to NextURL#516NathanDrake2406 wants to merge 5 commits intocloudflare:mainfrom
Conversation
commit: |
|
/bigbonk review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Good feature addition — NextURL has been missing these properties and real middleware depends on them. The overall approach (strip basePath/locale from internal pathname, reconstruct on href/toString()) is sound and matches how Next.js works internally. The test coverage is solid.
I found two bugs and one minor issue. The basePath setter bug is the most impactful — it silently corrupts URLs when someone tries to clear basePath.
See inline comments.
packages/vinext/src/shims/server.ts
Outdated
| return this._basePath; | ||
| } | ||
| set basePath(value: string) { | ||
| this._basePath = value.startsWith("/") ? value : "/" + value; |
There was a problem hiding this comment.
Bug: Setting basePath = "" (to clear basePath) produces "/" because "".startsWith("/") is false, so it becomes "/" + "" = "/". Then _formatPathname() treats "/" as a truthy basePath and prepends it to every pathname, producing double-slashes like "//about".
| this._basePath = value.startsWith("/") ? value : "/" + value; | |
| this._basePath = value === "" ? "" : value.startsWith("/") ? value : "/" + value; |
| } | ||
| set href(value: string) { | ||
| this._url.href = value; | ||
| if (this._locales) this._analyzeLocale(this._locales); |
There was a problem hiding this comment.
Bug: The href setter re-analyzes locale but doesn't re-strip basePath. If someone sets url.href = "http://localhost/app/fr/about" and basePath is "/app", the internal pathname becomes "/app/fr/about" — basePath is not stripped, locale analysis finds "app" as the first segment (not a locale), and everything breaks.
The existing test avoids this by only setting href to URLs without basePath, but real middleware code would hit this.
Consider adding basePath stripping before locale analysis here:
| if (this._locales) this._analyzeLocale(this._locales); | |
| this._url.href = value; | |
| // Re-strip basePath from the new pathname | |
| if (this._basePath && this._url.pathname.startsWith(this._basePath + "/")) { | |
| this._url.pathname = this._url.pathname.slice(this._basePath.length) || "/"; | |
| } else if (this._basePath && this._url.pathname === this._basePath) { | |
| this._url.pathname = "/"; | |
| } | |
| if (this._locales) this._analyzeLocale(this._locales); |
And add a test case like:
it("href setter re-strips basePath and re-analyzes locale", async () => {
const { NextURL } = await import("../packages/vinext/src/shims/server.js");
const url = new NextURL("http://localhost/app/fr/about", undefined, {
basePath: "/app",
...i18nConfig,
});
url.href = "http://localhost/app/de/contact";
expect(url.locale).toBe("de");
expect(url.pathname).toBe("/contact");
});
packages/vinext/src/shims/server.ts
Outdated
| const match = candidate ? locales.find((l) => l.toLowerCase() === candidate) : undefined; | ||
| if (match) { | ||
| this._locale = match; | ||
| this._url.pathname = "/" + segments.slice(2).join("/") || "/"; |
There was a problem hiding this comment.
Nit: The || "/" is unreachable. "/" + segments.slice(2).join("/") always produces at least "/" (when the join is empty), which is truthy. The fallback never executes.
Not a bug, just dead code — mentioning for awareness.
packages/vinext/src/shims/server.ts
Outdated
| constructor(input: URL | RequestInfo, init?: RequestInit) { | ||
| constructor( | ||
| input: URL | RequestInfo, | ||
| init?: RequestInit & { nextConfig?: NextURLConfig["nextConfig"] & { basePath?: string } }, |
There was a problem hiding this comment.
Minor type issue: NextURLConfig["nextConfig"] resolves to { i18n?: ... } | undefined. The intersection undefined & { basePath?: string } is never, which means TypeScript technically considers { basePath: "/app" } without i18n as invalid when nextConfig is undefined. In practice this works at runtime because init?.nextConfig short-circuits, but the type is misleading.
Consider simplifying to a dedicated interface:
init?: RequestInit & {
nextConfig?: {
basePath?: string;
i18n?: { locales: string[]; defaultLocale: string };
};
}| const nextRequest = mwRequest instanceof NextRequest ? mwRequest : new NextRequest(mwRequest); | ||
| const nextConfig = | ||
| basePath || i18nConfig | ||
| ? { basePath: basePath ?? "", i18n: i18nConfig ?? undefined } |
There was a problem hiding this comment.
When basePath is undefined and i18nConfig is truthy, this produces { basePath: "", i18n: ... }. That empty-string basePath is correct (matches the NextURL constructor's config?.basePath ?? "" default), but it's worth noting the intentionality here since the condition on line 439 uses basePath || i18nConfig — meaning this block also runs for the i18n-only case, where basePath being "" is the right default.
|
Review submitted. Here's a summary of the feedback: Two bugs found:
One minor item:
|
|
/bigbonk re-review |
There was a problem hiding this comment.
Good work — this fills a real gap in the NextURL shim. The overall approach of stripping basePath/locale from the internal pathname on construction and reconstructing on output is correct and matches how Next.js works internally. The test coverage is solid and the bonk review feedback has been addressed in the second commit.
I have a few items below, mostly nits and one behavioral difference worth thinking about. Nothing blocking.
| const urlConfig: NextURLConfig | undefined = nextConfig | ||
| ? { basePath: nextConfig.basePath, nextConfig: { i18n: nextConfig.i18n } } | ||
| : undefined; | ||
| this._nextUrl = new NextURL(url, undefined, urlConfig); |
There was a problem hiding this comment.
The init object (which now contains nextConfig) gets passed through to the Request constructor via super(input, init) on line 45 and the ...init spread on line 42. Request ignores unknown properties, so this is harmless at runtime, but it would be cleaner to strip nextConfig before passing to super to avoid leaking vinext-internal config into the underlying Request.
Not blocking — just flagging for awareness.
packages/vinext/src/shims/server.ts
Outdated
| this._stripBasePath(); | ||
| const i18n = config?.nextConfig?.i18n; | ||
| if (i18n) { | ||
| this._locales = i18n.locales; |
There was a problem hiding this comment.
Minor: this._locales = i18n.locales stores a direct reference to the caller's array. If the caller ever mutates the array after construction, the internal state silently changes. The locales getter defensively copies on read, but _analyzeLocale and the locale setter validation operate on the shared reference.
In practice this is fine because every callsite constructs the config inline, but for consistency with the defensive copy pattern used in clone() and the locales getter, consider:
| this._locales = i18n.locales; | |
| this._locales = [...i18n.locales]; |
packages/vinext/src/shims/server.ts
Outdated
| this._basePath = value === "" ? "" : value.startsWith("/") ? value : "/" + value; | ||
| } | ||
|
|
||
| get locale(): string | undefined { |
There was a problem hiding this comment.
Minor behavioral difference from Next.js: when no i18n config is present, Next.js's NextURL.locale returns '' (empty string), not undefined. Their implementation does this[Internal].locale ?? ''. Middleware code that does if (url.locale) will behave the same (both are falsy), but code that does typeof url.locale === 'string' or url.locale === undefined would differ.
Worth considering whether to match Next.js here. If you decide to, the getter would be:
| get locale(): string | undefined { | |
| get locale(): string { | |
| return this._locale ?? ""; | |
| } |
But this also changes the return type. Up to you whether the Next.js parity is worth it — most real middleware probably won't hit this.
| const match = candidate ? locales.find((l) => l.toLowerCase() === candidate) : undefined; | ||
| if (match) { | ||
| this._locale = match; | ||
| this._url.pathname = "/" + segments.slice(2).join("/"); |
There was a problem hiding this comment.
Nit: the || "/" fallback noted in the bonk review as unreachable is gone (or was never there in the actual code), so that's fine. But there's a related subtlety: when segments.slice(2) is empty (e.g., input /fr), this produces "/" + "" = "/". That's correct. Just confirming I traced through it.
|
|
||
| get href(): string { | ||
| return this._url.href; | ||
| const formatted = this._formatPathname(); |
There was a problem hiding this comment.
Performance nit: this allocates a new URL object on every href get when basePath or locale is active (which is the common case when these features are configured). If nextUrl.href is accessed in a hot loop or multiple times per request, the allocations add up.
A simple optimization would be to cache the formatted href and invalidate it when _url, _locale, or _basePath change. Not a blocker for this PR — just noting for if this shows up in profiles later.
| locales: ["en", "fr"], | ||
| defaultLocale: "en", | ||
| }, | ||
| }, |
There was a problem hiding this comment.
The NextRequest integration test passes a string URL. But in the actual middleware callsites (app-rsc-entry, pages-server-entry, middleware.ts), the input is always a Request object — e.g., new NextRequest(mwRequest, ...) where mwRequest is a Request. Consider adding a test case with a Request input to cover that code path:
it("NextRequest passes config through when input is a Request object", async () => {
const { NextRequest } = await import("../packages/vinext/src/shims/server.js");
const raw = new Request("http://localhost/app/fr/dashboard");
const req = new NextRequest(raw, {
nextConfig: {
basePath: "/app",
i18n: { locales: ["en", "fr"], defaultLocale: "en" },
},
});
expect(req.nextUrl.basePath).toBe("/app");
expect(req.nextUrl.locale).toBe("fr");
expect(req.nextUrl.pathname).toBe("/dashboard");
});| * Reconstruct the full pathname with basePath + locale prefix. | ||
| * Mirrors Next.js's internal formatPathname(). | ||
| */ | ||
| private _formatPathname(): string { |
There was a problem hiding this comment.
Worth noting as a follow-up: Next.js supports a forceLocale option on NextURL that, when true, causes formatPathname to include the default locale prefix in the output (i.e., /en/about instead of /about when en is default). The current implementation correctly omits the default locale prefix for the non-forceLocale case, but if middleware code relies on forceLocale, it won't work. Fine to skip for now but worth a TODO or issue.
|
Review submitted successfully. Here's a summary of my review on PR #516: Overall assessment: The PR is solid. The approach of stripping basePath/locale from the internal pathname on construction and reconstructing on output correctly matches Next.js's internal Feedback posted (nothing blocking):
|
|
/bigbonk review |
|
@james-elicx Bonk workflow was cancelled. View workflow run · To retry, trigger Bonk again. |
|
/bigbonk review |
|
/bigbonk review again |
There was a problem hiding this comment.
Solid PR — the approach of stripping basePath/locale from the internal pathname on construction and reconstructing on output correctly matches Next.js's internal NextURL implementation. The bugs from the first bonk review have been addressed. Test coverage is good with 21 new tests.
I found one bug and a few smaller items. See inline comments.
packages/vinext/src/shims/server.ts
Outdated
| } | ||
| // Add basePath prefix | ||
| if (this._basePath) { | ||
| pathname = this._basePath + pathname; |
There was a problem hiding this comment.
Bug: trailing slash on root path. When basePath is "/app" and the internal pathname is "/" (the root), this produces "/app/" (trailing slash). Next.js produces "/app" without it.
This means:
const url = new NextURL("http://localhost/app", undefined, { basePath: "/app" });
url.href; // => "http://localhost/app/" — trailing slashThis can cause redirect loops or exact-match failures in middleware that compares URLs. Consider:
| pathname = this._basePath + pathname; | |
| pathname = this._basePath + (pathname === "/" ? "" : pathname); |
| this._url.port = value; | ||
| } | ||
|
|
||
| /** Returns the pathname WITHOUT basePath or locale prefix. */ |
There was a problem hiding this comment.
The pathname setter writes directly to _url.pathname but doesn't interact with the locale system at all. This is arguably correct (the doc comment says it's the pathname WITHOUT locale prefix), but there's a subtle hazard: if someone sets url.pathname = "/fr/contact" where "fr" is a configured locale, the internal state is inconsistent — _locale still holds the old value, and _formatPathname() will double-prefix the locale in href.
In Next.js, the pathname setter on NextURL behaves the same way (it's the inner pathname, not the full one), so this is correct behavior. But worth adding a test that exercises pathname setter + href getter to lock in the expected behavior and document the contract. Something like:
it("pathname setter does not re-analyze locale", async () => {
const url = new NextURL("http://localhost/fr/about", undefined, i18nConfig);
url.pathname = "/contact";
expect(url.locale).toBe("fr"); // unchanged
expect(url.pathname).toBe("/contact");
expect(url.href).toBe("http://localhost/fr/contact");
});
packages/vinext/src/shims/server.ts
Outdated
| } | ||
|
|
||
| get locale(): string | undefined { | ||
| return this._locale; |
There was a problem hiding this comment.
Behavioral difference from Next.js: when no i18n config is present, Next.js's NextURL.locale returns '' (empty string), not undefined. Their implementation does this[Internal].locale ?? ''.
Middleware that does if (url.locale) works fine (both are falsy), but typeof url.locale === 'string' diverges. Minor parity gap — not blocking, but worth noting as a potential follow-up if middleware libraries rely on it being a string.
| set href(value: string) { | ||
| this._url.href = value; | ||
| this._stripBasePath(); | ||
| if (this._locales) this._analyzeLocale(this._locales); |
There was a problem hiding this comment.
The href setter correctly re-strips basePath and re-analyzes locale. Good fix from the bonk review feedback.
|
|
||
| it("href includes basePath prefix", async () => { | ||
| const { NextURL } = await import("../packages/vinext/src/shims/server.js"); | ||
| const url = new NextURL("http://localhost/dashboard", undefined, { |
There was a problem hiding this comment.
This test constructs NextURL("http://localhost/dashboard", ...) — the input URL does not contain the basePath. This tests the _formatPathname() output path but not the _stripBasePath() input path for the basePath-only (no i18n) case.
Consider adding a test where the input URL includes the basePath to exercise stripping:
it("basePath is stripped from input URL", async () => {
const { NextURL } = await import("../packages/vinext/src/shims/server.js");
const url = new NextURL("http://localhost/app/dashboard", undefined, {
basePath: "/app",
});
expect(url.pathname).toBe("/dashboard");
expect(url.basePath).toBe("/app");
expect(url.href).toBe("http://localhost/app/dashboard");
});| mwUrl.pathname = cleanPathname; | ||
| const mwRequest = new Request(mwUrl, request); | ||
| const nextRequest = mwRequest instanceof NextRequest ? mwRequest : new NextRequest(mwRequest); | ||
| const __mwNextConfig = (__basePath || __i18nConfig) ? { basePath: __basePath, i18n: __i18nConfig ?? undefined } : undefined; |
There was a problem hiding this comment.
Nit: when nextConfig.basePath is undefined, __basePath is undefined, so { basePath: __basePath, ... } produces { basePath: undefined, ... }. The NextURL constructor handles this correctly via config?.basePath ?? "". Just confirming this works as intended — it does.
|
The review was submitted successfully. Here's a summary of my review on PR #516: Overall assessment: The PR is solid. The approach of stripping basePath/locale from the internal pathname on construction and reconstructing on output correctly matches Next.js's internal Feedback posted (6 inline comments):
|
NextURL was missing basePath, locale, defaultLocale, and locales properties that Next.js documents and real middleware depends on. - Add NextURLConfig interface for passing basePath and i18n config - Extract locale from pathname on construction (case-insensitive) - Reconstruct full URL (with locale + basePath) in href/toString() - Validate locale setter against configured locales - Thread config through NextRequest → NextURL in all middleware paths: dev middleware runner, App Router RSC entry, Pages Router prod entry
- Fix basePath setter: setting to "" no longer produces "/" (URL corruption) - Fix href setter: re-strip basePath before locale re-analysis - Extract _stripBasePath() to avoid duplicated stripping logic - Simplify NextRequest constructor type (remove misleading intersection) - Validate locale setter against configured locales (throw TypeError) - Remove dead `|| "/"` fallback in _analyzeLocale - Add tests for basePath clear and href setter with basePath
- Strip nextConfig from init before passing to Request super() - Defensive copy of i18n.locales on construction - Add test for NextRequest with Request object input (real callsite pattern) - Silence unused parameter diagnostic in test
- Fix trailing slash bug: _formatPathname produced /app/ instead of /app at root pathname - Fix locale getter to return "" instead of undefined when no i18n config, matching Next.js's behavior - Eliminate URL allocation in href getter via string slicing - Consolidate _stripBasePath into single condition/assignment - Remove unnecessary ternary guard in _analyzeLocale - Use defensive locale copy consistently in constructor - Add regression tests for trailing slash (basePath-only and basePath + default locale), basePath input stripping, pathname setter contract, and href with port/search/hash
NathanDrake2406
force-pushed
the
fix/nexturl-basepath-locale
branch
from
7bfcac7 to
46ec93f
Compare
Third round of review feedback addressedRebased onto Bugs fixed
Performance
Code quality
Tests added
Intentionally skipped
|
|
/bigbonk review |
There was a problem hiding this comment.
Review
This is a well-structured PR. The approach of stripping basePath/locale from the internal pathname on construction and reconstructing on output via _formatPathname() is the right design — it mirrors how Next.js's internal NextURL works. The bugs caught in earlier review rounds have been addressed, the nextConfig is properly stripped before super(), and the test coverage is thorough at 29 new tests.
I have two substantive issues (one bug, one correctness gap) and two minor items.
Bug: locale setter accepts undefined without resetting to defaultLocale
When i18n is configured and you call url.locale = undefined, it bypasses validation (the if (value && ...) guard is falsy) and stores undefined as _locale. This means:
url.localereturns""(via the?? ""fallback)_formatPathname()omits the locale prefix entirely (sinceundefinedis falsy)- The URL now has no locale, which is an invalid state when i18n is configured
In Next.js, the locale setter validates and rejects undefined/empty when locales are configured. The fix would be to also throw on falsy values when _locales is set, or fall back to defaultLocale.
This is unlikely to be hit by real middleware today, so not blocking, but worth fixing before this ships.
Missing: searchParams mutations + href getter interaction test
The href getter's string-slicing approach reads search and hash from this._url at access time, which is correct — mutations via url.searchParams.set("key", "val") will be reflected. But there's no test exercising this path when basePath/locale are active. A test like url.searchParams.set("q", "2"); expect(url.href).toBe("http://localhost/app/fr/about?q=2") would lock this in.
| `The locale "${value}" is not in the configured locales: ${this._locales.join(", ")}`, | ||
| ); | ||
| } | ||
| this._locale = value; |
There was a problem hiding this comment.
Bug (minor): The locale setter allows undefined (and "") to pass through when _locales is configured, putting the URL in an invalid no-locale state. In Next.js, setting locale to a falsy value when i18n is configured either throws or falls back to defaultLocale.
This means url.locale = undefined followed by url.href silently drops the locale prefix, which could cause subtle routing bugs in middleware that conditionally clears the locale.
Consider:
| this._locale = value; | |
| set locale(value: string | undefined) { | |
| if (this._locales) { | |
| if (!value) { | |
| this._locale = this._defaultLocale; | |
| return; | |
| } | |
| if (!this._locales.includes(value)) { | |
| throw new TypeError( | |
| `The locale "${value}" is not in the configured locales: ${this._locales.join(", ")}`, | |
| ); | |
| } | |
| } | |
| this._locale = value; | |
| } |
| } | ||
| if (!prefix) return this._url.pathname; | ||
| const inner = this._url.pathname; | ||
| return inner === "/" ? prefix : prefix + inner; |
There was a problem hiding this comment.
Nit: the _formatPathname logic is clean. One thing worth noting for future readers: when prefix is just this._basePath (e.g. "/app") and inner is "/", this returns "/app" — no trailing slash. But when inner is "/" and there's a locale prefix (e.g. prefix is "/app/fr"), this also returns "/app/fr" — which is correct.
However, if someone calls url.pathname = "/" after construction (which doesn't re-analyze locale), and the locale is non-default, _formatPathname would return e.g. "/app/fr" — meaning href omits the trailing slash for the root. This matches Next.js, just confirming it's intentional.
| // Replace pathname in href via string slicing — avoids URL allocation. | ||
| // URL.href is always <origin+auth><pathname><search><hash>. | ||
| const { href, pathname, search, hash } = this._url; | ||
| const baseEnd = href.length - pathname.length - search.length - hash.length; |
There was a problem hiding this comment.
The string-slicing optimization to avoid new URL() allocation is nice. The arithmetic is correct: URL.href is always <scheme+auth+host+port><pathname><search><hash>, and you're computing baseEnd as href.length - pathname.length - search.length - hash.length.
One edge case to be aware of for future work: if the URL has a username:password@ in it, the origin property doesn't include those, but href does — and the slicing approach handles that correctly since it's purely length-based subtraction from the right. Good.
| const nextRequest = mwRequest instanceof NextRequest ? mwRequest : new NextRequest(mwRequest); | ||
| const nextConfig = | ||
| basePath || i18nConfig | ||
| ? { basePath: basePath ?? "", i18n: i18nConfig ?? undefined } |
There was a problem hiding this comment.
Nit: basePath ?? "" when basePath is undefined produces "", and i18nConfig ?? undefined when it's null produces undefined. Both are correct defaults that align with the NextURL constructor expectations. The basePath || i18nConfig guard on line 440 means this block also runs for the i18n-only case (where basePath is undefined → ""), which is the right behavior.
The parallel code in app-rsc-entry.ts and pages-server-entry.ts uses the same pattern, so all three middleware construction paths are consistent.
| ) { | ||
| // Strip nextConfig before passing to super() — it's vinext-internal, | ||
| // not a valid RequestInit property. | ||
| const { nextConfig: _nextConfig, ...requestInit } = init ?? {}; |
There was a problem hiding this comment.
Good fix — stripping nextConfig before passing to super() is the right call. The destructuring + rest pattern is clean.
Minor note: when init is undefined, init ?? {} produces {}, and destructuring gives { nextConfig: undefined, ...{} } — so requestInit is {} which is a valid RequestInit. No issue, just noting the empty-object path works.
| expect(req.nextUrl.basePath).toBe("/app"); | ||
| expect(req.nextUrl.locale).toBe("fr"); | ||
| expect(req.nextUrl.defaultLocale).toBe("en"); | ||
| expect(req.nextUrl.pathname).toBe("/dashboard"); |
There was a problem hiding this comment.
This test passes a string URL to NextRequest. The companion test below (line 4272) covers the Request object input path. Both are good.
One gap: neither test verifies that req.nextUrl.href reconstructs correctly with the basePath included. The string-input test URL "http://localhost/fr/dashboard" doesn't contain the basePath, so href should be "http://localhost/app/fr/dashboard". Consider adding:
expect(req.nextUrl.href).toBe("http://localhost/app/fr/dashboard");to confirm the full round-trip through NextRequest → NextURL → _formatPathname.
|
Review submitted successfully. Here's a summary of my feedback on PR #516: Overall assessment: This is a well-structured PR. The architecture of stripping basePath/locale from the internal pathname on construction and reconstructing on output via Key feedback posted (6 inline comments):
|
…onfigured Addresses 4th round of review feedback on PR cloudflare#516: - Bug: locale setter now falls back to defaultLocale instead of allowing undefined/empty when i18n is configured (prevents invalid no-locale state) - Add tests for searchParams mutations reflected in href with basePath/locale - Add nextUrl.href round-trip assertions to NextRequest integration tests
|
@james-elicx once this is merged, this will be my 69th PR. I think it's the perfect number to stop at! It's been fun going on this side quest with you, hardest working OSS maintainer ever! Cloudflare really should pay you all the money. |
|
/bigbonk review |
|
Thanks for all your contributions! |
There was a problem hiding this comment.
Review
This is a well-executed PR. The architecture — strip basePath/locale from the internal pathname on construction, reconstruct via _formatPathname() on output — correctly mirrors Next.js's internal NextURL implementation. The bugs caught in earlier bonk reviews have all been addressed (trailing slash fix, href setter re-stripping basePath, locale getter returning "" instead of undefined, nextConfig stripping before super(), locale setter falling back to defaultLocale for falsy values).
The string-slicing href optimization is correct and clean — I verified the arithmetic holds for URLs with auth info, ports, search params, and hash fragments.
Test coverage is thorough: 29 tests covering construction, property access, setters, clone, href reconstruction, edge cases (root paths, case-insensitive matching, basePath-only, i18n-only, combined), and NextRequest integration with both string and Request inputs.
All three middleware construction paths (dev runner in middleware.ts, App Router codegen in app-rsc-entry.ts, Pages Router codegen in pages-server-entry.ts) are updated consistently.
I have two minor observations below — neither is blocking.
| ); | ||
| } | ||
| } | ||
| this._locale = value; |
There was a problem hiding this comment.
Minor: when _locales is not configured (no i18n), the setter falls through to this._locale = value unconditionally. This means someone could do url.locale = "fr" on a non-i18n NextURL and _formatPathname() would start prepending /fr to every href. In Next.js, NextURL.locale is effectively a no-op setter when i18n isn't configured.
Unlikely to matter in practice (middleware doesn't set locale without i18n config), but could tighten it:
| this._locale = value; | |
| this._locale = this._locales ? value : this._locale; |
Or leave as-is — it's a "you get what you ask for" situation.
| const match = locales.find((l) => l.toLowerCase() === candidate); | ||
| if (match) { | ||
| this._locale = match; | ||
| this._url.pathname = "/" + segments.slice(2).join("/"); |
There was a problem hiding this comment.
Nit: when segments.slice(2) is empty (input /fr), this produces "/" + "" = "/". That's correct. Just confirming the edge case — the test at line 4155 ("root locale path /fr produces pathname /") covers this.
|
Review submitted successfully. Here's a summary: Verdict: Approved This is a solid PR that fills a real gap in the
I left two minor inline comments (neither blocking):
|
3 participants
Summary
NextURLwas missingbasePath,locale,defaultLocale, andlocalesproperties that Next.js documents and real middleware depends onhref/toString()reconstruct the full URL with locale + basePath prefixes — matching Next.jsformatPathname()semanticsNextRequest→NextURLin all three middleware construction paths (dev runner, App Router RSC entry codegen, Pages Router prod entry codegen)localesetter validates against configured locales (throwsTypeErroron invalid, matching Next.js)localesgetter returns a defensive copy to prevent internal mutationclone()passes config through the constructor for proper re-analysisTest plan
href/toString()reconstruction, locale setter validation, clone preservation, and NextRequest integrationtsgo --noEmit)oxlint)