fix(pages-router): dedupe in-flight _next/data fetches by URL (#1464)#1684
Conversation
|
/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.
Clean implementation. The dedup helper is well-isolated, the Response.clone() approach is spec-correct (tee doesn't disturb the stream, so N clones from the same original are legal), and the signal/abort design is sound — dropping the signal from the shared fetch while keeping pre-await guards is the right call for dedup semantics.
One minor inconsistency to clean up (non-blocking), noted inline.
| res = await fetch(target.dataHref, { | ||
| res = await dedupedPagesDataFetch(target.dataHref, { | ||
| headers: { Accept: "application/json", "x-nextjs-data": "1" }, | ||
| signal: controller.signal, |
There was a problem hiding this comment.
Nit: this AbortError check is now dead code — dedupedPagesDataFetch never passes a signal to fetch(), so the underlying request cannot throw AbortError. The equivalent check was already correctly removed from resolveMiddlewareDataRedirect (the catch block there was simplified to bare catch { return null; }), so this is an inconsistency between the two call sites.
Suggest either removing the try/catch entirely (the DOMException branch is unreachable, and re-throwing everything is the default behavior without a try/catch), or simplifying to match the other site. Not blocking — the dead branch is harmless — but it would be nice for both sites to tell the same story.
|
Review posted. Here's the summary: Approved PR #1684 with one inline nit. Review findings:
|
Concurrent `Router.push` calls (or back-to-back `<Link>` clicks) to the same gSSP route each fired their own `_next/data/<id>/<page>.json` fetch. The Next.js parity test asserts `hit: 1` after 4 rapid navigations to a slow page; vinext was producing `hit: N`. Add a module-scoped in-flight Promise cache keyed by the resolved data URL, mirroring `fetchNextData()`'s `inflightCache` in `packages/next/src/shared/lib/router/router.ts`. Concurrent callers share one `fetch()` invocation and each receive a cloned Response. Entries self-evict on settle so the next navigation re-fetches fresh. The shared fetch deliberately ignores per-caller `AbortSignal`s: stale navigations are already detected via `_navigationId` + `assertStillCurrent()` checkpoints after the await, and aborting the shared request on behalf of one superseded caller would destroy the dedup gain for every other concurrent caller. Pre-await abort still surfaces so callers see the documented `NavigationCancelledError` / `AbortError` when supersession happened before the fetch was even attempted. Wired into both `navigateClientData()` (the JSON fast path) and `resolveMiddlewareDataRedirect()` (the soft-redirect probe).
de7c291 to
44a3f6e
Compare
|
Rebased on origin/main; resolved adjacent-import conflict in |
1 participant
Summary
_next/datarequest deduplication not implemented #1464: client-side_next/datarequest dedup parity for the Pages Router.Router.pushcalls (or back-to-back<Link>clicks) for the same destination now share a single underlying fetch instead of each firing their own request.shims/internal/pages-data-fetch-dedup.ts, keyed by the resolved/_next/data/<id>/<page>.jsonURL, mirroringfetchNextData()'sinflightCacheinpackages/next/src/shared/lib/router/router.ts.navigateClientData()(the JSON navigation fast path) andresolveMiddlewareDataRedirect()(the soft-redirect probe). Each caller gets a cloned Response so bodies remain independently readable.Design notes
AbortSignals: stale navigations are already detected via_navigationId+assertStillCurrent()checkpoints after the await. Aborting the shared request on behalf of one superseded caller would destroy the dedup gain for every other concurrent caller.NavigationCancelledError/AbortErrorwhen supersession happened before the fetch was even attempted.Test plan
tests/pages-data-fetch-dedup.test.ts) covers: shared fetch under concurrency, sequential re-fetch, per-URL keying, and rejection eviction.pnpm test tests/pages-data-fetch-dedup.test.ts tests/pages-data-url.test.ts tests/pages-data-route.test.ts tests/router-prefetch-invalid-url.test.ts tests/link-navigation.test.ts tests/navigation-runtime.test.ts tests/shims.test.ts— 1101 tests pass.pnpm run check— format, lint, and type clean.Ported from Next.js: https://github.com/vercel/next.js/blob/canary/test/e2e/getserversideprops/test/index.test.ts (
should dedupe server data requests).