fix: add basePath and locale properties to NextURL#516
fix: add basePath and locale properties to NextURL#516NathanDrake2406 wants to merge 3 commits intocloudflare:mainfrom
Conversation
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
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:
|
- 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
|
/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]; |
| 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):
|
- 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
|
/bigbonk review |
|
@james-elicx Bonk workflow was cancelled. View workflow run · To retry, trigger Bonk again. |
|
/bigbonk review |
2 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)