fix(router): share Pages Router context across chunks - #2010
Conversation
commit: |
|
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: share Pages Router context across chunks
Reviewed both changed files, the consumers of RouterContext, and the App Router pattern this mirrors. The fix is correct and well-scoped. Scoped vp check and the new regression test both pass locally (1086 shim tests, 1 run / 1085 skipped).
Correctness
The change applies the same Symbol.for()-keyed globalThis identity strategy already used in app-router-context.ts:35-61. This is the right fix for the duplicate-module problem: when Vite evaluates next/router in both the client entry and a page chunk, the module-local createContext(null) produced two distinct context objects. PagesRouterProvider populated one (router.ts:2431) while a fallback page chunk read the other and fell through to the non-reactive Router singleton (router.ts:2353), so deferred isReady never propagated. Sharing the context object via globalThis[ROUTER_CONTEXT_KEY] resolves this.
All consumers benefit and none regress, since they all reference the same shared object:
router.ts:2339,2432— provider +useRoutercompat-router.ts:23—next/compat/routererror.tsx:245—useContext(RouterContext) !== nullPages-vs-App detection (this was previously a latent split-chunk bug too: a separate module-local context would have always returnednullhere)
No regression vs. the App Router pattern
app-router-context.ts adds a typeof React.createContext !== "function" guard and can return null. This PR omits that guard, which is correct: the original RouterContext was createContext(null) with no guard and was always treated as a valid, non-null context. The App Router guard exists because those contexts are imported into the RSC environment where createContext may be unavailable; the Pages Router context is client/SSR-only, so the guard isn't needed and adding nullability would force needless null checks on every consumer. Preserving the always-non-null type is the right call.
Test
tests/shims.test.ts:20934 correctly validates the fix: vi.resetModules() clears the registry to force a genuinely fresh re-import, and the toBe identity assertion only holds because of the globalThis symbol storage. It fails against the old createContext(null) implementation. Placed appropriately in the existing "Pages Router runtime state sharing" describe block.
Verification performed
vp check packages/vinext/src/shims/internal/router-context.ts tests/shims.test.ts— pass (format/lint/types)vp test run tests/shims.test.ts -t "shares RouterContext"— pass
No issues found. LGTM.
|
Review posted on PR #2010. SummaryI reviewed PR #2010 ( Verdict: LGTM, no issues found. The fix is correct and well-scoped:
I verified locally that |
Summary
RouterContextacross duplicated client-entry/page-chunk module instancesrouter.isReadyupdates for Pages Router pages rendered through the App Router hooks fixtureCI regression
This fixes a newly failing deploy-suite delta between:
5ace0db7ac49a3ea3317f682370085fea85e6d37de9d03a84688aebf1ece90a2bc83353960820112The affected upstream test is
test/e2e/app-dir/hooks/hooks.test.ts, specifically the/adapter-hooks/staticand/adapter-hooks/1/accountcases timing out while waiting for#router-ready.Endpoint independence
This failure is unrelated to
next-data-api-endpoint.vercel.app:test/e2e/app-dir/hookscontains nonext-data-api-endpoint.vercel.app,vercel.app, or HTTP URL referencesrouter.isReadyassertion timeouts, not fetch, deployment, or shared-server cascadesI excluded the middleware method-forwarding and rewrite-body candidates because their fixtures directly call the Vercel endpoint. I also excluded the segment-cache candidate because it failed locally at the declared good SHA.
Root cause
4baf85admade the Pages Router wrapper component types and readiness state shared across duplicatednext/routerbundles, butRouterContextstayed module-local. An HTML-fallback page chunk could therefore read a different context from the shared provider and fall back to the non-reactive singleton. Deferred-ready routes stayed atisReady=falseeven after the shared readiness bit flipped.This patch gives the Pages Router context the same symbol-keyed shared identity strategy already used by the App Router contexts. It does not change readiness semantics.
Next.js parity
Verified against pinned Next.js
v16.2.6/ee6e79b1792a4d401ddf2480f40a83549fe8e722:packages/next/src/shared/lib/router/router.tsdefines the initialisReadypredicatetest/integration/router-is-ready/test/index.test.tscovers immediate GSSP/GSP readiness and delayed GSP readiness with queriestest/e2e/app-dir/hooks/hooks.test.tsrequires the Pages Router hook to reach ready stateLocal reproduction
Fresh disposable Next.js checkout, Node 24, Next.js pinned to
ee6e79b1792a4d401ddf2480f40a83549fe8e722:env -u VINEXT_BUILD \ NEXTJS_PREPARE=0 \ NEXT_TEST_CONCURRENCY=1 \ vp env exec --node 24 -- \ ./scripts/run-nextjs-deploy-suite.sh /tmp/nextjs-v16.2.6-20260614-153154 \ --retries 0 -c 1 --debug \ test/e2e/app-dir/hooks/hooks.test.tsResults:
5ace0db7: vinext built, deployment succeeded, 26/26 passedde9d03a8: vinext built, deployment succeeded, 2 failed / 24 passed with matching#router-readytimeoutsd1a80cd6passes;4baf85adfails the same two assertionsValidation
vp check packages/vinext/src/shims/internal/router-context.ts tests/shims.test.ts vp test run tests/shims.test.ts