Skip to content

fix: add basePath and locale properties to NextURL #516

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

Merged
james-elicx merged 6 commits into from
Mar 18, 2026
Merged

fix: add basePath and locale properties to NextURL #516

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
79a24b0
fix: add basePath and locale properties to NextURL
NathanDrake2406 Mar 13, 2026
93e8084
fix: address review feedback on NextURL
NathanDrake2406 Mar 13, 2026
81e258d
fix: address second round of review feedback
NathanDrake2406 Mar 13, 2026
46ec93f
fix: address third round of review feedback on NextURL
NathanDrake2406 Mar 16, 2026
a365cf8
fix: locale setter resets to defaultLocale on falsy value when i18n c…
NathanDrake2406 Mar 17, 2026
57b8534
Update packages/vinext/src/shims/server.ts
james-elicx Mar 18, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion packages/vinext/src/entries/app-rsc-entry.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1631,7 +1631,8 @@ async function _handleRequest(request, __reqCtx, _mwCtx) {
const mwUrl = new URL(request.url);
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;
Copy link
Contributor

@ask-bonk ask-bonk bot Mar 16, 2026

Choose a reason for hiding this comment

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

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.

const nextRequest = mwRequest instanceof NextRequest ? mwRequest : new NextRequest(mwRequest, __mwNextConfig ? { nextConfig: __mwNextConfig } : undefined);
const mwFetchEvent = new NextFetchEvent({ page: cleanPathname });
const mwResponse = await middlewareFn(nextRequest, mwFetchEvent);
const _mwWaitUntil = mwFetchEvent.drainWaitUntil();
Expand Down
3 changes: 2 additions & 1 deletion packages/vinext/src/entries/pages-server-entry.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,7 +196,8 @@ async function _runMiddleware(request) {
mwUrl.pathname = normalizedPathname;
mwRequest = new Request(mwUrl, request);
}
var nextRequest = mwRequest instanceof NextRequest ? mwRequest : new NextRequest(mwRequest);
var __mwNextConfig = (vinextConfig.basePath || i18nConfig) ? { basePath: vinextConfig.basePath, i18n: i18nConfig || undefined } : undefined;
var nextRequest = mwRequest instanceof NextRequest ? mwRequest : new NextRequest(mwRequest, __mwNextConfig ? { nextConfig: __mwNextConfig } : undefined);
var fetchEvent = new NextFetchEvent({ page: normalizedPathname });
var response;
try { response = await middlewareFn(nextRequest, fetchEvent); }
Expand Down
1 change: 1 addition & 0 deletions packages/vinext/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2766,6 +2766,7 @@ export default function vinext(options: VinextOptions = {}): PluginOption[] {
middlewarePath,
middlewareRequest,
nextConfig?.i18n,
nextConfig?.basePath,
);

if (!result.continue) {
Expand Down
10 changes: 9 additions & 1 deletion packages/vinext/src/server/middleware.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -394,6 +394,7 @@ export async function runMiddleware(
middlewarePath: string,
request: Request,
i18nConfig?: NextI18nConfig | null,
basePath?: string,
): Promise<MiddlewareResult> {
// Load the middleware module via the direct-call ModuleRunner.
// This bypasses the hot channel entirely and is safe with all Vite plugin
Expand Down Expand Up @@ -435,7 +436,14 @@ export async function runMiddleware(
}

// Wrap in NextRequest so middleware gets .nextUrl, .cookies, .geo, .ip, etc.
const nextRequest = mwRequest instanceof NextRequest ? mwRequest : new NextRequest(mwRequest);
const nextConfig =
basePath || i18nConfig
? { basePath: basePath ?? "", i18n: i18nConfig ?? undefined }
Copy link
Contributor

@ask-bonk ask-bonk bot Mar 13, 2026

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@ask-bonk ask-bonk bot Mar 17, 2026

Choose a reason for hiding this comment

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

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.

: undefined;
const nextRequest =
mwRequest instanceof NextRequest
? mwRequest
: new NextRequest(mwRequest, nextConfig ? { nextConfig } : undefined);
const fetchEvent = new NextFetchEvent({ page: normalizedPathname });

// Execute the middleware
Expand Down
140 changes: 132 additions & 8 deletions packages/vinext/src/shims/server.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,18 @@ export class NextRequest extends Request {
private _nextUrl: NextURL;
private _cookies: RequestCookies;

constructor(input: URL | RequestInfo, init?: RequestInit) {
constructor(
input: URL | RequestInfo,
init?: RequestInit & {
nextConfig?: {
basePath?: string;
i18n?: { locales: string[]; defaultLocale: string };
};
},
) {
// Strip nextConfig before passing to super() — it's vinext-internal,
// not a valid RequestInit property.
const { nextConfig: _nextConfig, ...requestInit } = init ?? {};
Copy link
Contributor

@ask-bonk ask-bonk bot Mar 17, 2026

Choose a reason for hiding this comment

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

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.

// Handle the case where input is a Request object - we need to extract URL and init
// to avoid Node.js undici issues with passing Request objects directly to super()
if (input instanceof Request) {
Expand All @@ -70,18 +81,21 @@ export class NextRequest extends Request {
body: req.body,
// @ts-expect-error - duplex is not in RequestInit type but needed for streams
duplex: req.body ? "half" : undefined,
...init,
...requestInit,
});
} else {
super(input, init);
super(input, requestInit);
}
const url =
typeof input === "string"
? new URL(input, "http://localhost")
: input instanceof URL
? input
: new URL(input.url, "http://localhost");
this._nextUrl = new NextURL(url);
const urlConfig: NextURLConfig | undefined = _nextConfig
? { basePath: _nextConfig.basePath, nextConfig: { i18n: _nextConfig.i18n } }
: undefined;
this._nextUrl = new NextURL(url, undefined, urlConfig);
Copy link
Contributor

@ask-bonk ask-bonk bot Mar 13, 2026

Choose a reason for hiding this comment

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

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.

this._cookies = new RequestCookies(this.headers);
}

Expand Down Expand Up @@ -216,18 +230,86 @@ export class NextResponse<_Body = unknown> extends Response {
// NextURL — lightweight URL wrapper with pathname helpers
// ---------------------------------------------------------------------------

export interface NextURLConfig {
basePath?: string;
nextConfig?: {
i18n?: {
locales: string[];
defaultLocale: string;
};
};
}

export class NextURL {
/** Internal URL stores the pathname WITHOUT basePath or locale prefix. */
private _url: URL;
private _basePath: string;
private _locale: string | undefined;
private _defaultLocale: string | undefined;
private _locales: string[] | undefined;

constructor(input: string | URL, base?: string | URL) {
constructor(input: string | URL, base?: string | URL, config?: NextURLConfig) {
this._url = new URL(input.toString(), base);
this._basePath = config?.basePath ?? "";
this._stripBasePath();
const i18n = config?.nextConfig?.i18n;
if (i18n) {
this._locales = [...i18n.locales];
this._defaultLocale = i18n.defaultLocale;
this._analyzeLocale(this._locales);
}
}

/** Strip basePath prefix from the internal pathname. */
private _stripBasePath(): void {
if (!this._basePath) return;
const { pathname } = this._url;
if (pathname === this._basePath || pathname.startsWith(this._basePath + "/")) {
this._url.pathname = pathname.slice(this._basePath.length) || "/";
}
}

/** Extract locale from pathname, stripping it from the internal URL. */
private _analyzeLocale(locales: string[]): void {
const segments = this._url.pathname.split("/");
const candidate = segments[1]?.toLowerCase();
const match = locales.find((l) => l.toLowerCase() === candidate);
if (match) {
this._locale = match;
this._url.pathname = "/" + segments.slice(2).join("/");
Copy link
Contributor

@ask-bonk ask-bonk bot Mar 13, 2026

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@ask-bonk ask-bonk bot Mar 17, 2026

Choose a reason for hiding this comment

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

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.

} else {
this._locale = this._defaultLocale;
}
}

/**
* Reconstruct the full pathname with basePath + locale prefix.
* Mirrors Next.js's internal formatPathname().
*/
private _formatPathname(): string {
Copy link
Contributor

@ask-bonk ask-bonk bot Mar 13, 2026

Choose a reason for hiding this comment

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

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.

// Build prefix: basePath + locale (skip defaultLocale — Next.js omits it)
let prefix = this._basePath;
if (this._locale && this._locale !== this._defaultLocale) {
prefix += "/" + this._locale;
}
if (!prefix) return this._url.pathname;
const inner = this._url.pathname;
return inner === "/" ? prefix : prefix + inner;
Copy link
Contributor

@ask-bonk ask-bonk bot Mar 17, 2026

Choose a reason for hiding this comment

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

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.

}

get href(): string {
return this._url.href;
const formatted = this._formatPathname();
Copy link
Contributor

@ask-bonk ask-bonk bot Mar 13, 2026

Choose a reason for hiding this comment

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

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.

if (formatted === this._url.pathname) return this._url.href;
// 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;
Copy link
Contributor

@ask-bonk ask-bonk bot Mar 17, 2026

Choose a reason for hiding this comment

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

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.

return href.slice(0, baseEnd) + formatted + search + hash;
}
set href(value: string) {
this._url.href = value;
this._stripBasePath();
if (this._locales) this._analyzeLocale(this._locales);
Copy link
Contributor

@ask-bonk ask-bonk bot Mar 13, 2026

Choose a reason for hiding this comment

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

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:

Suggested change
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");
});

Copy link
Contributor

@ask-bonk ask-bonk bot Mar 16, 2026

Choose a reason for hiding this comment

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

The href setter correctly re-strips basePath and re-analyzes locale. Good fix from the bonk review feedback.

}

get origin(): string {
Expand Down Expand Up @@ -276,6 +358,7 @@ export class NextURL {
this._url.port = value;
}

/** Returns the pathname WITHOUT basePath or locale prefix. */
Copy link
Contributor

@ask-bonk ask-bonk bot Mar 16, 2026

Choose a reason for hiding this comment

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

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");
});

get pathname(): string {
return this._url.pathname;
}
Expand All @@ -301,12 +384,53 @@ export class NextURL {
this._url.hash = value;
}

get basePath(): string {
return this._basePath;
}
set basePath(value: string) {
this._basePath = value === "" ? "" : value.startsWith("/") ? value : "/" + value;
}

get locale(): string {
return this._locale ?? "";
}
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 = this._locales ? value : this._locale;
}

get defaultLocale(): string | undefined {
return this._defaultLocale;
}

get locales(): string[] | undefined {
return this._locales ? [...this._locales] : undefined;
}

clone(): NextURL {
return new NextURL(this._url.href);
const config: NextURLConfig = {
basePath: this._basePath,
nextConfig: this._locales
? { i18n: { locales: [...this._locales], defaultLocale: this._defaultLocale! } }
: undefined,
};
// Pass the full href (with locale/basePath re-added) so the constructor
// can re-analyze and extract locale correctly.
return new NextURL(this.href, undefined, config);
}

toString(): string {
return this._url.toString();
return this.href;
}

/**
Expand Down
6 changes: 4 additions & 2 deletions tests/__snapshots__/entry-templates.test.ts.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16609,7 +16609,8 @@ async function _handleRequest(request, __reqCtx, _mwCtx) {
const mwUrl = new URL(request.url);
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;
const nextRequest = mwRequest instanceof NextRequest ? mwRequest : new NextRequest(mwRequest, __mwNextConfig ? { nextConfig: __mwNextConfig } : undefined);
const mwFetchEvent = new NextFetchEvent({ page: cleanPathname });
const mwResponse = await middlewareFn(nextRequest, mwFetchEvent);
const _mwWaitUntil = mwFetchEvent.drainWaitUntil();
Expand Down Expand Up @@ -20099,7 +20100,8 @@ async function _runMiddleware(request) {
mwUrl.pathname = normalizedPathname;
mwRequest = new Request(mwUrl, request);
}
var nextRequest = mwRequest instanceof NextRequest ? mwRequest : new NextRequest(mwRequest);
var __mwNextConfig = (vinextConfig.basePath || i18nConfig) ? { basePath: vinextConfig.basePath, i18n: i18nConfig || undefined } : undefined;
var nextRequest = mwRequest instanceof NextRequest ? mwRequest : new NextRequest(mwRequest, __mwNextConfig ? { nextConfig: __mwNextConfig } : undefined);
var fetchEvent = new NextFetchEvent({ page: normalizedPathname });
var response;
try { response = await middlewareFn(nextRequest, fetchEvent); }
Expand Down
Loading
Loading