-
Notifications
You must be signed in to change notification settings - Fork 329
fix(link): warn on repeated forward slashes in href #1563
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
base: main
Are you sure you want to change the base?
Changes from 1 commit
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 |
|---|---|---|
|
|
@@ -152,6 +152,40 @@ function resolveHref(href: LinkProps["href"]): string { | |
| return url; | ||
| } | ||
|
|
||
| // Track hrefs we have already warned about so we do not spam the console on | ||
| // every re-render. Mirrors React's behaviour for repeated warnings. | ||
| const warnedRepeatedSlashHrefs = new Set<string>(); | ||
|
|
||
| /** | ||
| * Emit Next.js's "Invalid href" console.error when `href` contains repeated | ||
| * forward slashes or backslashes in its path portion. | ||
| * | ||
| * Ported from Next.js: packages/next/src/client/resolve-href.ts | ||
| * https://github.com/vercel/next.js/blob/canary/packages/next/src/client/resolve-href.ts | ||
| * | ||
| * Matches the message asserted by: | ||
| * test/e2e/repeated-forward-slashes-error/repeated-forward-slashes-error.test.ts | ||
| */ | ||
| function warnOnRepeatedSlashesInHref(urlAsString: string): void { | ||
| // Strip any protocol prefix (e.g. "https://") so we do not flag the | ||
| // legitimate `//` that separates the scheme from the authority. | ||
| const urlProtoMatch = urlAsString.match(/^[a-z][a-z0-9+.-]*:\/\//i); | ||
| const urlAsStringNoProto = urlProtoMatch | ||
| ? urlAsString.slice(urlProtoMatch[0].length) | ||
| : urlAsString; | ||
| const urlParts = urlAsStringNoProto.split("?", 1); | ||
| if (!(urlParts[0] || "").match(/(\/\/|\\)/)) return; | ||
|
|
||
| if (warnedRepeatedSlashHrefs.has(urlAsString)) return; | ||
| warnedRepeatedSlashHrefs.add(urlAsString); | ||
|
|
||
| const pathname = | ||
| typeof window !== "undefined" && window.location ? window.location.pathname : "/"; | ||
| console.error( | ||
| `Invalid href '${urlAsString}' passed to next/router in page: '${pathname}'. Repeated forward-slashes (//) or backslashes \\ are not valid in the href.`, | ||
| ); | ||
| } | ||
|
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. Next.js also normalizes the URL after warning — it calls Fine to ship the warning first and follow up with normalization, but worth filing a follow-up issue. |
||
|
|
||
| export function resolveLinkPrefetchMode( | ||
| prefetchProp: LinkProps["prefetch"], | ||
| isDangerous: boolean, | ||
|
|
@@ -537,6 +571,13 @@ const Link = forwardRef<HTMLAnchorElement, LinkProps>(function Link( | |
| // where href is a route pattern like "/user/[id]" and as is "/user/1") | ||
| const resolvedHref = as ?? resolveHref(href); | ||
|
|
||
| // Mirror Next.js: emit a console.error when the href contains repeated | ||
| // forward-slashes (e.g. "/foo//bar") or backslashes. Next.js does not block | ||
| // navigation — it only warns. See packages/next/src/client/resolve-href.ts. | ||
| if (typeof resolvedHref === "string") { | ||
| warnOnRepeatedSlashesInHref(resolvedHref); | ||
| } | ||
|
|
||
| const isDangerous = typeof resolvedHref === "string" && isDangerousScheme(resolvedHref); | ||
|
|
||
| // Apply locale prefix if specified (safe even for dangerous hrefs since we | ||
|
|
||
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 dedup
Setdiverges from Next.js, which firesconsole.erroron every call toresolveHref— no dedup. Consider removing it for exact parity. If the dedup is intentional, document it as a deliberate divergence and export a test helper to clear state between test runs (the module-levelSetpersists across tests within a Vitest worker).