fix(link): warn on repeated forward slashes in href#1563
Conversation
Closes #1554 When a `<Link href>` contains repeated forward-slashes (e.g. `/foo//bar`) or a backslash, Next.js's `resolveHref` emits a `console.error` so the developer notices the invalid URL during dev/CI. vinext's Link shim did not surface that warning, which caused the Next.js Deploy Suite test `test/e2e/repeated-forward-slashes-error` to fail waiting for the message in the CLI output. Add a small `warnOnRepeatedSlashesInHref` helper that mirrors the regex and message used by Next.js, and call it from the Link body after the href is resolved. The href continues to render and navigate unchanged — Next.js does not block, it only warns. Ported from Next.js: packages/next/src/client/resolve-href.ts
|
/bigbonk review |
commit: |
|
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Good change that closes the parity gap from #1554. The warning regex is correct, the test coverage is solid, and CI is green. Three items to address before merging:
1. Module-level dedup Set is a test-ordering hazard and a Next.js parity gap
warnedRepeatedSlashHrefs is a module-level Set that is never exported and never cleared. This means:
- Test flakiness risk: If a future test reuses the same bad href (e.g.
/hello//world) after another test already rendered it, the warning is silently suppressed. The current tests happen to use unique hrefs so this doesn't fail today, but it's fragile. - Parity gap: Next.js does not deduplicate this warning. It fires
console.errorevery timeresolveHrefencounters repeated slashes — on every render, every navigation. The comment says "Mirrors React's behaviour for repeated warnings" but that's not what Next.js does here.
Two options depending on how strongly you want parity:
- (a) Remove the
Setentirely to match Next.js (simplest, exact parity). - (b) Keep it but export a
_resetRepeatedSlashWarnings()test helper and call it inafterEach, and add a comment documenting the intentional divergence from Next.js.
I'd lean toward (a) since the dedup is only ~3 lines and removing it gets exact parity.
2. Missing URL normalization after the warning
Next.js doesn't just warn — it also normalizes the URL after detecting repeated slashes. In packages/next/src/client/resolve-href.ts:
const normalizedUrl = normalizeRepeatedSlashes(urlAsStringNoProto)
urlAsString = (urlProtoMatch ? urlProtoMatch[0] : '') + normalizedUrlThis means in Next.js, <Link href="/hello//world"> renders href="/hello/world" (normalized). In vinext, it renders href="/hello//world" (raw). This is a functional difference — the browser will navigate to different paths.
This doesn't need to block this PR if you file a follow-up, but it should be tracked since it affects actual navigation behavior, not just the warning message.
3. router.pathname vs window.location.pathname in the message
Minor: Next.js uses router.pathname (the route pattern, e.g. /posts/[id]) in the warning message. vinext uses window.location.pathname (the actual browser URL, e.g. /posts/42) or "/" during SSR. This difference is cosmetic and unlikely to affect the compat test, but worth noting in a comment if you keep the current approach.
|
|
||
| // 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>(); |
There was a problem hiding this comment.
The dedup Set diverges from Next.js, which fires console.error on every call to resolveHref — 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-level Set persists across tests within a Vitest worker).
| console.error( | ||
| `Invalid href '${urlAsString}' passed to next/router in page: '${pathname}'. Repeated forward-slashes (//) or backslashes \\ are not valid in the href.`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Next.js also normalizes the URL after warning — it calls normalizeRepeatedSlashes() to collapse // → / and \\ → /, then continues routing with the cleaned URL. Without that step, <Link href="/hello//world"> renders href="/hello//world" in the DOM (vinext) vs href="/hello/world" (Next.js). This affects actual navigation, not just the warning.
Fine to ship the warning first and follow up with normalization, but worth filing a follow-up issue.
|
Review posted on PR #1563. Here's a summary of the findings: Overall: Good change that closes the parity gap from #1554. The warning regex is correct (identical to Next.js), tests cover the key cases (positive match, backslash, protocol prefix, clean hrefs, query string), and CI is nearly all green. Three items flagged (requested changes):
|
Address review feedback on #1563: - Remove the module-level `warnedRepeatedSlashHrefs` Set. Next.js fires this warning on every `resolveHref` call, so dedup was an unintended divergence and a latent test-ordering hazard. - Add `normalizeRepeatedSlashes` (port of packages/next/src/shared/lib/utils/normalize-repeated-slashes.ts) and apply it after warning, so `<Link href="/a//b">` renders `href="/a/b"` exactly as Next.js does. - Preserve protocol-relative URLs (`//example.com/...`) unchanged — vinext treats those as external via `isAbsoluteOrProtocolRelativeUrl`, so we skip both the warning and the normalization for them to avoid regressing the existing "locale does not mangle protocol-relative URLs" test. - Document the `window.location.pathname` fallback (Next.js uses `router.pathname`; the difference is cosmetic and not asserted by the compat test). - Add tests covering no-dedup behaviour, href normalization (including backslashes, query-string preservation, and protocol-prefixed URLs).
|
/bigbonk review |
There was a problem hiding this comment.
All three items from the previous review are resolved in the follow-up commit:
-
Dedup
Setremoved — No trace ofwarnedRepeatedSlashHrefsremains. Warning fires on every render, matching Next.js. The "no dedup" behavior is explicitly tested ("fires on every render") and documented in the JSDoc. -
URL normalization implemented —
normalizeRepeatedSlashes()ported from Next.js collapses//→/and\\→/in the path portion, preserving query strings and protocol prefixes. The renderedhrefattribute now matches Next.js behavior. Tests cover: slash collapsing, backslash normalization, query string preservation, and protocol preservation for absolute URLs. -
window.location.pathnamefallback documented — JSDoc explicitly notes the cosmetic difference fromrouter.pathnameand that the Next.js compat test doesn't assert on this segment.
Implementation looks correct. The protocol-relative URL early return (startsWith("//")) is a reasonable divergence since vinext treats those as external URLs — and it's documented inline. Test coverage is thorough with 10 test cases covering positive/negative/edge cases.
CI: Check, all E2E projects, and deploys are green. Vitest shards still running but the relevant link.test.ts changes are unit-level and already validated by Check.
Ship it.
|
Review posted — approved PR #1563. All three items from the previous round (dedup |
1 participant
Closes #1554
Summary
console.errormatching Next.js when<Link href>contains repeated forward slashes (e.g.//foo//).Test plan
vp checkpasses