fix(pages-router): preserve query string in Link and router.push#1572
fix(pages-router): preserve query string in Link and router.push#1572james-elicx wants to merge 1 commit into
Conversation
`parseQueryString` and the parseQuery template embedded in the Pages
Router worker entry both used `url.split("?")[1]`, which silently drops
everything after the second `?`. URLs like `/linker?href=/about?hello=world`
have query `href=/about?hello=world` per RFC 3986, so the previous
implementation truncated `query.href` to `/about` and `<Link>` rendered
without the embedded query string. Switching to an `indexOf`-based slice
keeps the full query value intact.
Closes #1471
|
/bigbonk review |
commit: |
|
There was a problem hiding this comment.
Clean, correct fix. The root cause analysis is accurate — split("?")[1] drops everything after the second ?, violating RFC 3986 where only the first ? delimits path from query.
Both parseQueryString (shared utility) and the inline parseQuery in the generated Pages entry now correctly use indexOf("?") + slice(), and the hash-stripping logic is correct (indexOf("#", queryIndex + 1) searches only within the query portion).
Tests: Good coverage — unit tests for parseQueryString (basic parsing, multi-value, embedded ?, hash stripping) plus integration tests for the SSR <Link> render path with and without trailing slash. The fixture mirrors Next.js's test/e2e/trailing-slashes/pages/linker.js, which is exactly what AGENTS.md asks for.
Dev/prod parity: I checked all server entry points. dev-server.ts imports the shared parseQueryString (already fixed). prod-server.ts delegates query parsing to the built entry's parseQuery (already fixed). App Router entries don't parse query strings into objects — they use the URL constructor. No parity gap.
One remaining instance: packages/vinext/src/index.ts:2801 still has url.split("?")[1] in the image optimization dev passthrough. It's low-risk in practice since <Image> URL-encodes the url parameter, but it's technically susceptible to the same bug and inconsistent with the rest of the codebase. Worth a follow-up.
Ship it.
|
Review posted and approved on PR #1572. Here's the summary: Verdict: Approved. The fix is correct and well-tested.
|
Closes #1471
Summary
<Link>androuter.push()now preserve query strings in href and navigation URL, matching Next.js. Trailing-slash normalisation preserves the query.The root cause was
parseQueryString(and the parallelparseQuerydefinition embedded into the Pages Router worker entry) usingurl.split("?")[1], which drops everything after the second?. URLs like/linker?href=/about?hello=worldhave queryhref=/about?hello=worldper RFC 3986, so the previous code truncatedquery.hrefto/aboutand<Link>rendered without the embedded query string.Test plan
vp checkpasses