-
Notifications
You must be signed in to change notification settings - Fork 221
fix: skip locale prefix for absolute and protocol-relative Link hrefs #455
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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -242,6 +242,30 @@ describe("Link locale handling", () => { | |
| // Should not become /fr/fr/about | ||
| expect(html).toContain('href="/fr/about"'); | ||
| }); | ||
|
|
||
| it("locale does not mangle absolute same-origin URLs", () => { | ||
| // An absolute URL like https://example.com/about should not become | ||
| // /fr/https://example.com/about — locale prefix only applies to paths | ||
| const html = ReactDOMServer.renderToString( | ||
| React.createElement(Link, { href: "https://example.com/about", locale: "fr" } as any, "x"), | ||
| ); | ||
| expect(html).toContain('href="https://example.com/about"'); | ||
| }); | ||
|
|
||
| it("locale does not mangle protocol-relative URLs", () => { | ||
| // //example.com/about should not become /fr///example.com/about | ||
| const html = ReactDOMServer.renderToString( | ||
| React.createElement(Link, { href: "//example.com/about", locale: "fr" } as any, "x"), | ||
| ); | ||
| expect(html).toContain('href="//example.com/about"'); | ||
| }); | ||
|
|
||
| it("locale does not mangle http:// URLs", () => { | ||
| const html = ReactDOMServer.renderToString( | ||
| React.createElement(Link, { href: "http://example.com/path", locale: "de" } as any, "x"), | ||
| ); | ||
| expect(html).toContain('href="http://example.com/path"'); | ||
| }); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good coverage — three distinct URL patterns (https, protocol-relative, http) with clear comments explaining the expected behavior. Minor: might be worth adding a test for |
||
| }); | ||
|
|
||
| // ─── toSameOriginPath ──────────────────────────────────────────────────── | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fix looks correct and is well-placed — right after the
undefined/falseearly returns and before the locale-prefixing logic.Note: the sibling function
applyNavigationLocale()inrouter.ts:125-132has the same bug. It's used byRouter.push()andRouter.replace()when alocaleoption is passed. Consider adding the same guard there (and a corresponding test) — either in this PR or as an immediate follow-up:(The
startsWithversion you have is perfectly fine too — just showing the regex alternative for brevity. No change needed here.)