Skip to content

fix(pages): emit canonical __NEXT_DATA__ JSON - #2043

Merged
james-elicx merged 7 commits into
mainfrom
codex/fix-prerender-data-routes
Jun 15, 2026
Merged

fix(pages): emit canonical __NEXT_DATA__ JSON#2043
james-elicx merged 7 commits into
mainfrom
codex/fix-prerender-data-routes

Conversation

@james-elicx

Copy link
Copy Markdown
Member

Prerender canonical __NEXT_DATA__ parity

Source and scope

  • Source run: GitHub Actions 27514800656, test/e2e/prerender.test.ts (20 failed records).
  • Base: origin/main at a3d2f921520ff140a826224616df5e0db4ed0186.
  • Next.js reference: v16.2.6, commit/build commit ee6e79b1792a4d401ddf2480f40a83549fe8e722.
  • Focused root cause: Pages Router HTML used executable <script>window.__NEXT_DATA__ = ...</script> rather than Next.js-compatible <script id="__NEXT_DATA__" type="application/json">...</script>.
  • Included related payload correction: omit gsp when the page does not export getStaticProps; Next.js does not serialize gsp: false.
  • Explicitly excluded: query propagation, mismatched href/as _next/data, fallback rewrites, caching headers, invalid JSON behavior, navigation failures, no-revalidate behavior, preview/on-demand ISR, and all PR fix(pages): align on-demand ISR regeneration semantics #2027 scope.

Original failure classification

The cohesive cluster contained eight assertions:

  1. should SSR incremental page correctly
  2. should SSR blocking path correctly (blocking)
  3. should SSR blocking path correctly (pre-rendered)
  4. should have gsp in __NEXT_DATA__
  5. should not have gsp in __NEXT_DATA__ for non-GSP page
  6. should support prerendered catchall route
  7. should support prerendered catchall-explicit route (nested)
  8. should support prerendered catchall-explicit route (single)

Seven initially failed with Unexpected end of JSON input because the upstream tests read script#__NEXT_DATA__.text(). After canonical markup, the eighth reached its real payload assertion and showed that vinext serialized gsp: false instead of omitting the field.

The other 12 records were classified as separate product gaps or cascade symptoms and were not changed in this work.

Implementation

  • Emit canonical JSON __NEXT_DATA__ markup in Pages dev and production responses, including nonce support.
  • Initialize window.__NEXT_DATA__ and locale globals explicitly from the JSON element before hydration.
  • Parse both canonical JSON and legacy assignment markup during client navigation for compatibility.
  • Replace canonical or legacy __NEXT_DATA__ blocks during cached Pages regeneration.
  • Omit gsp when false while preserving gsp: true readiness behavior.

Local validation

Passed:

  • tests/shims.test.ts: 3 targeted tests.
  • tests/pages-page-response.test.ts: 3 targeted tests.
  • tests/pages-page-data.test.ts: 9 targeted tests.
  • tests/entry-templates.test.ts: 27 tests.
  • tests/pages-router.test.ts: 8 targeted integration tests.
  • Scoped vp check: formatting, lint, and type checks passed for all nine changed source/test files.
  • git diff --check passed.

Independent review found four stale CSP integration assertions that still expected the former executable assignment markup. They now assert the canonical nonced JSON element while preserving the existing cache, preload, and content checks.

Review-fix validation passed:

  • tests/pages-router.test.ts: 4 exact CSP/nonced __NEXT_DATA__ tests.
  • tests/shims.test.ts: 2 vinext next data client helpers tests.
  • tests/pages-page-response.test.ts: 3 targeted nonce/cache/HTML recording tests.
  • tests/pages-page-data.test.ts: 43 tests.
  • tests/entry-templates.test.ts: 27 tests.
  • Scoped vp check tests/pages-router.test.ts passed.
  • git diff --check passed.

The initial frozen vp install populated dependencies and built the core packages; an unrelated Fumadocs example postinstall failed once. A subsequent wrapper install completed successfully.

Exact Next.js deploy validation

Command:

NEXTJS_PREPARE=0 NEXT_TEST_CONCURRENCY=1 \
  vp env exec --node 24 \
  ./scripts/run-nextjs-deploy-suite.sh \
  /Users/jamesanderson/Developer/vinext/.nextjs-ref \
  --retries 0 -c 1 --debug \
  test/e2e/prerender.test.ts

The wrapper does not forward a Jest name filter, so the single targeted file ran in full. Deployment succeeded and assertions were reached.

Scoped final results:

  • PASS — should SSR incremental page correctly
  • PASS — should SSR blocking path correctly (blocking)
  • PASS — should SSR blocking path correctly (pre-rendered)
  • PASS — should have gsp in __NEXT_DATA__
  • PASS — should not have gsp in __NEXT_DATA__ for non-GSP page
  • PASS — should support prerendered catchall route
  • PASS — should support prerendered catchall-explicit route (nested)
  • PASS — should support prerendered catchall-explicit route (single)

The full file remains red only for out-of-scope query, rewrite, _next/data, caching, navigation, invalid-JSON, and ISR assertions.

@github-actions

Copy link
Copy Markdown
Contributor
Example Preview Production Original
app-router-cloudflare preview production
pages-router-cloudflare preview production
app-router-playground preview production original
realworld-api-rest preview production
nextra-docs-template preview production
benchmarks preview production
hackernews preview production original
workers-cache preview production
web preview production

@james-elicx

Copy link
Copy Markdown
Member Author

/bigbonk review for issues

@ask-bonk ask-bonk Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: PR #2043 — canonical __NEXT_DATA__ JSON

The core change is correct and matches Next.js: emitting <script id="__NEXT_DATA__" type="application/json">...</script> with HTML-escaped JSON (via safeJsonStringify, which escapes </>/&), parsing it client-side, and omitting gsp when false. Next.js serializes the same canonical markup, and gsp: undefined is correctly stripped by both the production payload filter (pages-page-response.ts:273) and JSON.stringify in dev.

However, this PR will fail the required Vitest CI check. It changed the emitted markup but only updated stale assertions in 3 test files, leaving ~25 tests across at least 5 files that still extract __NEXT_DATA__ with the old executable-assignment pattern (window.__NEXT_DATA__ = {...}) against live server HTML.

Blocking — stale test assertions not updated (CI red)

Verified failing locally on this branch (pass on main):

  • tests/pages-router.test.ts — 14 failures. Lines 1552, 1659, 1670, 1682, 1696, 1712, 1867, 1927, 1947, 1956, 4773, 4925, 4935, 4953 all use /<script>window\.__NEXT_DATA__\s*=\s*({.*?})<\/script>/ or /__NEXT_DATA__\s*=\s*(\{.*?\})\s*[;<]/ against fetched HTML. e.g. includes isFallback: false in __NEXT_DATA__ (tests/pages-router.test.ts:1956) → expected null to be truthy. Note the new readNextData() helper added at tests/pages-router.test.ts:6355 was only applied to the router __NEXT_DATA__ correctness block, not these.
  • tests/features.test.ts — 6 failures. Lines 680, 1216, 1227, 4721, 4734, 5751 — incl. i18n routing > includes locale info in __NEXT_DATA__ script/...for default locale, __NEXT_DATA__ query contains dynamic params for static pages, __NEXT_DATA__ contains nested GSSP props.
  • tests/pages-router-concurrency.test.ts — 3 failures. Lines 78, 159, 174no __NEXT_DATA__ found in response.
  • tests/static-export.test.ts — 2 failures. Lines 183, 205expected null to be truthy.
  • tests/pages-page-response.test.ts — 2 failures. tests/pages-page-response.test.ts:181/:182 (asserts "window.__NEXT_DATA__" and "__VINEXT_LOCALE__" in server HTML) and :706 (window.__NEXT_DATA__). The PR updated the assertion at :310 in this same file but missed these.

The PR description says "Local validation: Passed" and lists tests/pages-router.test.ts: 8 targeted integration tests" and "tests/features.test.ts is not listed — these full files are red. All of these need to be migrated to extract the canonical id="__NEXT_DATA__" type="application/json" element (the readNextData() helper, or extractVinextNextDataJson, is the natural fit). Note :182's __VINEXT_LOCALE__ assertion is now legitimately gone from server HTML (locale globals moved client-side) and should be dropped, not just reformatted.

The test inputs at tests/pages-page-response.test.ts:840–1213 and tests/pages-page-data.test.ts:88–1246 are mock cached HTML feeding the legacy-fallback path in rewritePagesCachedHtml — those are fine to leave as legacy markup.

Non-blocking — duplicated locale-globals logic

The new initial-hydration code in entries/pages-client-entry.ts:164-169 and server/dev-server.ts:1572-1578 reimplements locale-global assignment inline and unconditionally:

window.__VINEXT_LOCALE__ = window.__NEXT_DATA__.locale;       // undefined on non-i18n pages
window.__VINEXT_LOCALES__ = window.__NEXT_DATA__.locales;
window.__VINEXT_DEFAULT_LOCALE__ = window.__NEXT_DATA__.defaultLocale;

The repo already has applyVinextLocaleGlobals() (client/vinext-next-data.ts:133) which sets each global only when defined and is used during client navigation (shims/router.ts:1696,1874). For initial load the unconditional undefined assignment is functionally equivalent, but reusing applyVinextLocaleGlobals would keep the initial-load and navigation paths in lockstep and avoid two more copies of this logic drifting. Consider routing both initialization sites through that helper.

Summary

Source change: looks correct and Next.js-aligned. Block on the test regressions — please update all the stale __NEXT_DATA__ extractors above so the full Vitest suite is green, and consider consolidating the locale-globals initialization onto applyVinextLocaleGlobals.

@ask-bonk

ask-bonk Bot commented Jun 15, 2026

Copy link
Copy Markdown
Contributor

Review posted to PR #2043.

Summary

The source change is correct and Next.js-aligned — canonical <script id="__NEXT_DATA__" type="application/json"> markup with HTML-escaped JSON, client-side parsing, and gsp omission when false (correctly stripped by both the production payload filter and JSON.stringify).

The blocking issue: the PR will fail the required Vitest CI check. It changed the emitted markup but only updated stale assertions in 3 test files, leaving ~27 test failures across 5 files that still extract __NEXT_DATA__ with the old window.__NEXT_DATA__ = {...} pattern against live server HTML. I verified these fail on the branch and pass on main:

  • tests/pages-router.test.ts — 14
  • tests/features.test.ts — 6
  • tests/pages-router-concurrency.test.ts — 3
  • tests/static-export.test.ts — 2
  • tests/pages-page-response.test.ts — 2 (incl. a now-stale __VINEXT_LOCALE__ assertion that should be removed)

I distinguished these live-response assertions from the legitimate legacy-markup test inputs (e.g. pages-page-data.test.ts mocks feeding the fallback path), which are fine to leave.

One non-blocking note: the new initial-hydration code duplicates locale-global assignment inline and unconditionally in two places, instead of reusing the existing applyVinextLocaleGlobals() helper used by the navigation path — a drift risk worth consolidating.

github run

@james-elicx

Copy link
Copy Markdown
Member Author

/bigbonk review for issues

@pkg-pr-new

pkg-pr-new Bot commented Jun 15, 2026

Copy link
Copy Markdown

Open in StackBlitz

npm i https://pkg.pr.new/@vinext/cloudflare@2043
npm i https://pkg.pr.new/vinext@2043

commit: 748325f

@ask-bonk ask-bonk Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: PR #2043 — canonical __NEXT_DATA__ JSON

I re-ran the affected files on the current branch HEAD (48798ac). The core source change is correct and Next.js-aligned (canonical <script id="__NEXT_DATA__" type="application/json">, HTML-escaped JSON, client-side parse, gsp omitted when false). But the PR will fail the required Vitest check, and the prior review's failure inventory is now partly stale and missed a real regression. Corrected findings below.

Blocking #1 — NEW regression: static next/router import removed (missed by prior review)

tests/pages-router.test.ts has exactly 1 failure, and it is not a __NEXT_DATA__ extractor — it's Virtual server entry generation > client entry statically imports next/router so window.next.router is set before hydration (tests/pages-router.test.ts:2434). Both assertions fail:

  • :2451 expects the entry to statically import Router, { wrapWithRouterContext } from "next/router" at module top
  • :2458 expects the lazy await import("next/router") to NOT be present

The diff in packages/vinext/src/entries/pages-client-entry.ts deleted the top-level static import and replaced it with:

const { default: Router, wrapWithRouterContext, _initializePagesRouterReadyFromNextData } =
  await import("next/router");

This is a behavioral regression, not just a stale test. Importing next/router runs installPagesRouterRuntime() + installWindowNext() as a module-eval side effect (shims/router.ts:3037-3056) — that stamps the initial history entry, attaches the popstate listener, and exposes window.next.router. The static import was deliberately the first vinext shim specifically to run that side effect before hydration and close the popstate race window (the deleted comment and Next.js client/next.ts reference document this). Deferring it via await import reopens that race and removes window.next.router availability before async work — which is exactly what the test guards against.

The motivation (parse __NEXT_DATA__ before the router reads readiness) is legitimate, but it shouldn't require dropping the static import. Keep the static import Router, { wrapWithRouterContext, _initializePagesRouterReadyFromNextData } from "next/router" at the top, parse __NEXT_DATA__ from the JSON element, then call _initializePagesRouterReadyFromNextData(window.__NEXT_DATA__). The readiness initializer is an explicit call already, so it does not depend on a dynamic import.

Blocking #2 — stale live-response __NEXT_DATA__ extractors

Verified failing on the branch:

  • tests/features.test.ts — 4 failures. :680 (window.__NEXT_DATA__ = (\{...\})), :1217 / :1228 (i18n locale info), :4735 (dynamic params for static pages), :5751 (nested GSSP props). All still match window.__NEXT_DATA__ = {...} against live HTML.
  • tests/pages-router-concurrency.test.ts — 3 failures. :78, :159, :174 use /__NEXT_DATA__\s*=\s*(\{[^<]+\})/no __NEXT_DATA__ found in response.
  • tests/static-export.test.ts — 2 failures. :183, :205 use /window\.__NEXT_DATA__\s*=\s*({[^<]+})/expected null to be truthy.

Migrate these to extract the canonical id="__NEXT_DATA__" type="application/json" element (reuse the readNextData() helper added at tests/pages-router.test.ts:6383, or extractVinextNextDataJson).

Corrections to the prior review

  • tests/pages-router.test.ts is not 14 failures — its __NEXT_DATA__ extractors are already migrated to the canonical regex (e.g. :1552, :1660, :1962). It fails only on the static-import test above.
  • tests/pages-page-response.test.ts passes now (40/40 — verified). The :181/:182/:706 and __VINEXT_LOCALE__ concerns were already addressed; the :182 block now asserts the canonical JSON element and parses locale fields out of it.

Non-blocking — duplicated locale-globals logic (confirmed)

entries/pages-client-entry.ts:154-156 and server/dev-server.ts reimplement locale-global assignment inline and unconditionally (window.__VINEXT_LOCALE__ = window.__NEXT_DATA__.locale, etc.), assigning undefined on non-i18n pages. applyVinextLocaleGlobals() (client/vinext-next-data.ts:133) already sets each only when defined and is used by the navigation path (shims/router.ts). Route both init sites through it to avoid drift.

Summary

Source change is correct. Block on: (1) restore the static next/router import (pages-router.test.ts:2434 regression — reopens the popstate race), and (2) migrate the remaining 9 stale live-response __NEXT_DATA__ extractors in features.test.ts, pages-router-concurrency.test.ts, and static-export.test.ts. Then the full Vitest suite should be green.

@ask-bonk

ask-bonk Bot commented Jun 15, 2026

Copy link
Copy Markdown
Contributor

Review posted to PR #2043.

Summary

The source change is correct and Next.js-aligned, but the PR will fail the required Vitest check. I re-ran the affected files on the current branch HEAD and the prior review's inventory was partly stale and missed a real regression:

Blocking #1 — NEW regression the prior review missed. tests/pages-router.test.ts does NOT have 14 failures (its __NEXT_DATA__ extractors were already migrated). It has exactly 1 failure: client entry statically imports next/router so window.next.router is set before hydration (:2434). The PR deleted the top-level static import Router, { wrapWithRouterContext } from "next/router" and replaced it with await import("next/router"). That's a behavioral regression — importing the router runs installPagesRouterRuntime() + installWindowNext() as a module-eval side effect (popstate listener, history stamp, window.next.router); deferring it reopens the popstate race the static import was designed to close. Fix: keep the static import and still call _initializePagesRouterReadyFromNextData() after parsing __NEXT_DATA__.

Blocking #2 — 9 stale live-response extractors (verified failing): features.test.ts (4), pages-router-concurrency.test.ts (3), static-export.test.ts (2). Migrate to the canonical JSON element.

Corrections to prior review: pages-page-response.test.ts actually passes now (40/40); the __VINEXT_LOCALE__/:182 concerns were already fixed.

Non-blocking: duplicated unconditional locale-globals assignment should reuse applyVinextLocaleGlobals().

github run

@james-elicx

Copy link
Copy Markdown
Member Author

/bigbonk review for issues

@james-elicx

Copy link
Copy Markdown
Member Author

/bigbonk review for issues

@ask-bonk

ask-bonk Bot commented Jun 15, 2026

Copy link
Copy Markdown
Contributor

@james-elicx Bonk workflow was cancelled.

View workflow run · To retry, trigger Bonk again.

@james-elicx

Copy link
Copy Markdown
Member Author

/bigbonk review for issues

@ask-bonk ask-bonk Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: PR #2043 — canonical __NEXT_DATA__ JSON

I re-ran the affected files on the current branch HEAD (75311fb). The source change is correct and Next.js-aligned, and the two previously-blocking findings are now resolved — but there is one new blocking test regression that all three prior reviews missed, introduced by the latest commit (75311fb fix(pages): preserve router bootstrap ordering).

Blocking — tests/entry-templates.test.ts:994 fails (required Vitest check)

Verified failing on this branch, passing on main:

tests/entry-templates.test.ts > Pages Router entry template > hydrates _app with the full Pages props envelope
AssertionError: expected '…' to contain 'import Router, { wrapWithRouterContext } from "next/router";'

tests/entry-templates.test.ts:994 asserts the generated client entry contains the exact single-line string:

expect(code).toContain('import Router, { wrapWithRouterContext } from "next/router";');

Commit 75311fb restored the static next/router import (good — that fixes the prior Blocking #1), but emitted it in a multi-line form and added a third symbol:

import Router, {
  wrapWithRouterContext,
  _initializePagesRouterReadyFromNextData,
} from "next/router";

The exact .toContain() no longer matches, so the test fails. This assertion existed and passed on main (a3d2f92), so it is a regression introduced by this PR, not a pre-existing failure.

Note the sibling test tests/pages-router.test.ts:2451 passes because it uses a tolerant regex (/^import\s+Router,\s*\{[^}]*\bwrapWithRouterContext\b[^}]*\}\s+from\s+["']next\/router["']/m). The fix is to bring entry-templates.test.ts:994 in line — either match the multi-line/multi-symbol import exactly, or switch to a regex like the one at pages-router.test.ts:2451.

Resolved since the prior reviews

  • Prior Blocking #1 (static next/router import removed): fixed by 75311fb. The static import Router, { wrapWithRouterContext, _initializePagesRouterReadyFromNextData } from "next/router" is back at module top in both entries/pages-client-entry.ts and server/dev-server.ts, and pages-router.test.ts:2434 ("client entry statically imports next/router…") now passes. The popstate-race concern is addressed.
  • Prior Blocking #2 (stale live-response __NEXT_DATA__ extractors): resolved. I ran tests/features.test.ts, tests/pages-router-concurrency.test.ts, and tests/static-export.test.ts together — all 347 tests pass. The extractors were migrated to the canonical <script id="__NEXT_DATA__" type="application/json">…</script> element. I also confirmed the remaining window.__NEXT_DATA__ = matches in the test suite are legitimate legacy inputs (mock cached HTML for the rewritePagesCachedHtml fallback path in pages-page-data.test.ts/pages-page-response.test.ts, the extractVinextNextDataJson legacy-fallback test in shims.test.ts:45, and safe-json.test.ts XSS-escaping inputs), not live-response assertions.

Source change — correct and Next.js-aligned

  • Canonical markup <script id="__NEXT_DATA__" type="application/json"…> with HTML-escaped JSON via safeJsonStringify (escapes </>/&/\u2028/\u2029) — matches Next.js and is XSS-safe in the JSON-script context.
  • gsp omission: pages-readiness.ts:45 now emits gsp: hasPageGsp ? true : undefined; undefined is stripped by both the production payload filter and JSON.stringify. The only consumer (shims/router.ts:1171) checks nextData.gsp !== true, so absent vs. false is behaviorally identical. Type-clean (vp check passes).
  • Client navigation parses both canonical and legacy markup via extractVinextNextDataJson + parseVinextNextDataJson; ISR cached-HTML rewrite (pages-page-data.ts) handles both canonical and legacy __NEXT_DATA__ blocks. Nonced canonical element is handled by lookahead-based regexes that tolerate attribute order.
  • vp check is clean on all 7 changed source files.

Non-blocking — duplicated locale-globals logic (still present)

entries/pages-client-entry.ts:164-169 and server/dev-server.ts:1573-1577 reimplement locale-global assignment inline and unconditionally (window.__VINEXT_LOCALE__ = window.__NEXT_DATA__.locale, etc., assigning undefined on non-i18n pages). The repo already has applyVinextLocaleGlobals() (client/vinext-next-data.ts:133), which assigns each global only when defined and is used by the navigation path (shims/router.ts:1705). Routing both init sites through that helper would keep initial-load and navigation in lockstep and avoid further drift. Functionally equivalent today, so non-blocking.

Summary

Block on the single regression: update tests/entry-templates.test.ts:994 to match the new multi-line next/router import (the latest commit changed the generated import shape but left this exact-string assertion stale). Everything else — source correctness, the static-import restoration, and the migrated live-response extractors — is in good shape and the rest of the affected suite is green.

@ask-bonk

ask-bonk Bot commented Jun 15, 2026

Copy link
Copy Markdown
Contributor

Review posted to PR #2043.

Summary

The source change is correct and Next.js-aligned, and the two previously-blocking findings are now resolved by commit 75311fb. But I found one new blocking test regression that all three prior reviews missed — introduced by that same latest commit.

Blocking — tests/entry-templates.test.ts:994 fails (required Vitest check). Verified failing on the branch, passing on main. The test asserts the generated client entry contains the exact single-line string import Router, { wrapWithRouterContext } from "next/router";. Commit 75311fb correctly restored the static import (fixing prior Blocking #1) but emitted it as a multi-line import with a third symbol (_initializePagesRouterReadyFromNextData), so the exact .toContain() no longer matches. The sibling test at pages-router.test.ts:2451 passes because it uses a tolerant regex. Fix: update :994 to match the new import shape (or switch to a regex).

Resolved since prior reviews:

Non-blocking: the duplicated unconditional locale-globals assignment in pages-client-entry.ts and dev-server.ts should reuse applyVinextLocaleGlobals() to avoid drift.

The core implementation (canonical JSON markup, XSS-safe escaping, gsp omission, dual canonical/legacy parsing, nonce handling) is sound, and vp check is clean on all changed source files.

github run

@james-elicx

Copy link
Copy Markdown
Member Author

/bigbonk review for issues

@james-elicx
james-elicx marked this pull request as ready for review June 15, 2026 10:56

@ask-bonk ask-bonk Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: PR #2043 — canonical __NEXT_DATA__ JSON

Re-reviewed at branch HEAD (f0b2434 test(pages): expect readiness bootstrap import). The prior blocking finding is resolved and the affected Vitest suite is now green. I verified all changed source files lint/type-clean and ran the full set of affected test files — 1826 tests pass across entry-templates, pages-router, features, pages-router-concurrency, static-export, pages-page-response, pages-page-data, and shims. No blocking issues found.

Prior blocking finding — resolved

  • Prior Blocking (tests/entry-templates.test.ts:994 exact-string import Router, { wrapWithRouterContext } from "next/router";): fixed by f0b2434. The assertion now matches the multi-line/three-symbol import shape (import Router, { + wrapWithRouterContext, + _initializePagesRouterReadyFromNextData, + } from "next/router";) and additionally asserts _initializePagesRouterReadyFromNextData(window.__NEXT_DATA__);. Verified passing on the branch; was failing before. The static next/router import restoration (prior Blocking #1) and the migrated live-response extractors (prior Blocking #2) remain in good shape.

Source change — correct and Next.js-aligned

  • Canonical markup <script id="__NEXT_DATA__" type="application/json"…> with HTML-escaped JSON via safeJsonStringify (escapes </>/&/\u2028/\u2029), so an embedded </script> serializes as \u003c/script\u003e — XSS-safe in the JSON-script context and matches Next.js's htmlEscapeJsonString behavior.
  • Because the element is type="application/json" (non-executable) and the hydration entry is an ES module (deferred), document.getElementById("__NEXT_DATA__") resolves it regardless of DOM order relative to the module script. Dev (dev-server.ts) and prod (pages-page-response.tsbuildPagesShellHtml) both inject it, and both client entries (generated pages-client-entry.ts + dev inline) parse it and call _initializePagesRouterReadyFromNextData. Dev/prod parity holds.
  • The new readiness seam is necessary: routerRuntimeState is computed at module-eval of next/router (shims/router.ts:464:439 shouldDeferInitialPagesRouterReady()), which reads window.__NEXT_DATA__. Since the canonical element is no longer executable, __NEXT_DATA__ is undefined at that point, so the explicit _initializePagesRouterReadyFromNextData(window.__NEXT_DATA__) after parsing correctly re-derives the ready bit. Verified by pages-router.test.ts readiness tests passing.
  • gsp omission (pages-readiness.ts:45gsp: hasPageGsp ? true : undefined): undefined is stripped by both the production payload filter and JSON.stringify; the only consumer (shims/router.ts:1171) checks nextData.gsp !== true, so absent vs. false is behaviorally identical. Correct.
  • Legacy + canonical dual-parse: extractVinextNextDataJson (vinext-next-data.ts) tries the canonical lookahead regex first, then the legacy assignment; rewritePagesCachedHtml (pages-page-data.ts:553-558) replaces whichever block is present. Attribute-order-tolerant lookaheads handle the nonced element.

Non-blocking — duplicated locale-globals logic (still present)

entries/pages-client-entry.ts:158-160 and server/dev-server.ts:1575-1577 reimplement locale-global assignment inline and unconditionally (window.__VINEXT_LOCALE__ = window.__NEXT_DATA__.locale, etc.), assigning undefined on non-i18n pages. applyVinextLocaleGlobals() (client/vinext-next-data.ts:133) already assigns each global only when defined and defensively copies locales ([...nextData.locales]); it is used by the navigation path (shims/router.ts:1705, :1882). Routing both init sites through it would keep initial-load and navigation in lockstep and avoid further drift. Functionally equivalent today.

Non-blocking — unconditional readiness call when __NEXT_DATA__ is absent

entries/pages-client-entry.ts:163 calls _initializePagesRouterReadyFromNextData(window.__NEXT_DATA__) unconditionally, even when nextDataElement?.textContent is falsy and window.__NEXT_DATA__ is therefore undefined. initializePagesRouterReadyFromNextData (shims/router.ts:1217) dereferences nextData.page with no guard, so it would throw Cannot read properties of undefined (reading 'page'). The very next function, hydrate(), explicitly guards if (!nextData) { console.error("[vinext] No __NEXT_DATA__ found"); return; } (:167) — so the codebase already anticipates a missing __NEXT_DATA__, but the new readiness call now crashes before that graceful path is reached. This is new code (on main, __NEXT_DATA__ was set by the executable inline script before next/router ran, so no separate call existed). Low severity since a Pages page should always carry __NEXT_DATA__, but consider moving the call inside the if (nextDataElement?.textContent) block (or guarding it) to preserve the existing graceful-degradation behavior. (The dev-server copy at :1581 already dereferences nextData.props unguarded, so it degrades the same way regardless.)

Summary

No blocking issues — the previously-failing entry-templates.test.ts:994 is fixed and the affected suite is green. Source change is correct, XSS-safe, and Next.js-aligned. Two non-blocking nits: consolidate the initial-load locale-globals onto applyVinextLocaleGlobals(), and guard the unconditional _initializePagesRouterReadyFromNextData call against a missing __NEXT_DATA__ so the existing graceful-degradation path in hydrate() is reachable.

@ask-bonk

ask-bonk Bot commented Jun 15, 2026

Copy link
Copy Markdown
Contributor

Review posted to PR #2043.

Summary

No blocking issues. The prior blocking finding (tests/entry-templates.test.ts:994 stale exact-string import assertion) was fixed by the latest commit f0b2434. I verified the affected Vitest suite is green — 1826 tests pass across all 8 affected files — and vp check is clean on all 7 changed source files.

Source change is correct and Next.js-aligned:

  • Canonical <script id="__NEXT_DATA__" type="application/json"> markup with XSS-safe HTML-escaped JSON (safeJsonStringify escapes </>/&).
  • The new _initializePagesRouterReadyFromNextData seam is genuinely needed: since the canonical element is non-executable, __NEXT_DATA__ is undefined at next/router module-eval, so the explicit re-derivation of the readiness bit after parsing is the correct fix (and closes the prior popstate-race regression by keeping the static import).
  • gsp omission, dual canonical/legacy parsing, nonce handling, and dev/prod parity all verified.

Two non-blocking notes:

  1. Initial-load locale-globals are assigned inline and unconditionally in both pages-client-entry.ts and dev-server.ts instead of reusing the existing applyVinextLocaleGlobals() helper (used by the navigation path) — a drift risk.
  2. New, previously-unreported: pages-client-entry.ts:163 calls _initializePagesRouterReadyFromNextData(window.__NEXT_DATA__) unconditionally, which would throw Cannot read properties of undefined (reading 'page') if __NEXT_DATA__ is absent — reached before the graceful if (!nextData) guard in hydrate(). Low severity (a Pages page should always carry __NEXT_DATA__), but worth guarding to preserve the existing degradation path.

github run

@james-elicx

Copy link
Copy Markdown
Member Author

/bigbonk review for issues

Comment thread tests/entry-templates.test.ts Dismissed

@ask-bonk ask-bonk Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: PR #2043 — canonical __NEXT_DATA__ JSON

Re-reviewed at branch HEAD (748325f fix(pages): guard readiness bootstrap without next data). No blocking issues. The one remaining non-blocking concern from the prior review (unconditional _initializePagesRouterReadyFromNextData call when __NEXT_DATA__ is absent) is now fixed. The affected Vitest suite is green and vp check is clean on all 7 changed source files.

Verification

  • Affected suite green — 1854 tests pass. I ran entry-templates, pages-router (322), plus features, pages-router-concurrency, static-export, pages-page-response, pages-page-data, shims (1532). All pass on this branch.
  • vp check clean on all 7 changed source files (format, lint, types).

Prior blocking findings — all resolved

  • entry-templates.test.ts:994 stale exact-string import (resolved earlier by f0b2434): the assertion now matches the multi-line three-symbol next/router import shape and asserts _initializePagesRouterReadyFromNextData(nextData);. Passing.
  • Static next/router import restoration and migrated live-response __NEXT_DATA__ extractors: both still in good shape; the relevant tests pass.

Prior non-blocking #2 — now fixed by 748325f

entries/pages-client-entry.ts previously called _initializePagesRouterReadyFromNextData(window.__NEXT_DATA__) unconditionally (would throw Cannot read properties of undefined (reading 'page') when __NEXT_DATA__ is absent, before the graceful if (!nextData) return in hydrate()). The latest commit moves the call inside hydrate() after that guard (pages-client-entry.ts:165-170) and wraps the locale-globals assignment in if (nextDataElement?.textContent) (:156-161). A new focused test (entry-templates.test.ts:1015 "gracefully skips Pages Router initialization without NEXT_DATA") asserts the readiness initializer is not called and only the [vinext] No __NEXT_DATA__ found error is logged. Verified passing. Good fix with coverage.

Source change — correct and Next.js-aligned

  • Canonical markup <script id="__NEXT_DATA__" type="application/json"…> with HTML-escaped JSON via safeJsonStringify (escapes </>/&/\u2028/\u2029 → embedded </script> serializes as \u003c/script\u003e). XSS-safe in the JSON-script context and matches Next.js htmlEscapeJsonString (server/html.ts:21).
  • The readiness seam (shims/router.ts:1217 initializePagesRouterReadyFromNextData) is necessary: the canonical element is non-executable, so window.__NEXT_DATA__ is undefined at next/router module-eval; the explicit post-parse call re-derives the ready bit. Static next/router import preserved at module top in both entries, so the popstate-race concern stays closed.
  • gsp omission (pages-readiness.ts:45gsp: hasPageGsp ? true : undefined): undefined is stripped by both the production payload filter and JSON.stringify; the only consumer (shims/router.ts:1171) checks nextData.gsp !== true, so absent vs. false is behaviorally identical. Matches Next.js (which does not serialize gsp: false).
  • Dual canonical/legacy parse: extractVinextNextDataJson (client/vinext-next-data.ts:57) tries the attribute-order-tolerant canonical lookahead first, then a balanced-brace scan of the legacy assignment; rewritePagesCachedHtml (pages-page-data.ts:553-558) replaces whichever block is present. Nonce handled via createNonceAttribute.
  • Dev (dev-server.ts) and prod (pages-page-response.ts:293buildPagesShellHtml) both emit the canonical element and both client entries parse it before hydration. Dev/prod parity holds.

Remaining non-blocking note (carried over)

The initial-load locale-globals are still assigned inline and unconditionally in entries/pages-client-entry.ts:158-160 and server/dev-server.ts:1575-1577 (window.__VINEXT_LOCALE__ = window.__NEXT_DATA__.locale, etc., assigning undefined on non-i18n pages), rather than reusing applyVinextLocaleGlobals() (client/vinext-next-data.ts:133), which sets each only when defined and defensively copies locales. The navigation path already uses the helper (shims/router.ts:1705, :1882). Functionally equivalent today, but routing both init sites through the helper would keep initial-load and navigation in lockstep and avoid further drift. Optional cleanup, not a blocker.

(Note: the dev-server inline script at dev-server.ts:1580-1581 still dereferences nextData unguarded, but the dev path always injects __NEXT_DATA__ immediately above the hydration script, so it is not reachable in practice — same as before this PR.)

Summary

No blocking issues. The previously-noted unconditional readiness call is fixed with a dedicated test; source change is correct, XSS-safe, Next.js-aligned, and the affected Vitest suite + vp check are green. The single remaining nit (consolidate initial-load locale-globals onto applyVinextLocaleGlobals) is optional.

@ask-bonk

ask-bonk Bot commented Jun 15, 2026

Copy link
Copy Markdown
Contributor

Review posted to PR #2043.

Summary

No blocking issues. I re-reviewed at branch HEAD (748325f), which is one commit newer than the prior review — that commit resolves the only remaining concern.

Verification:

  • Affected Vitest suite is green — 1854 tests pass (entry-templates, pages-router, features, pages-router-concurrency, static-export, pages-page-response, pages-page-data, shims).
  • vp check clean on all 7 changed source files.

Prior non-blocking finding now fixed by 748325f: the unconditional _initializePagesRouterReadyFromNextData(window.__NEXT_DATA__) call (which would throw Cannot read properties of undefined when __NEXT_DATA__ is absent) is moved inside hydrate() after the if (!nextData) return guard, and the locale-globals assignment is wrapped in an if (nextDataElement?.textContent) check. A new focused test (entry-templates.test.ts:1015) asserts the readiness initializer is not called and only the graceful error logs. Good fix with coverage.

Source change is correct and Next.js-aligned: canonical <script id="__NEXT_DATA__" type="application/json"> markup with XSS-safe safeJsonStringify escaping, the necessary readiness seam, gsp omission when false, dual canonical/legacy parsing, nonce handling, and dev/prod parity all verified.

One optional non-blocking nit carried over: initial-load locale-globals are still assigned inline/unconditionally in pages-client-entry.ts and dev-server.ts instead of reusing applyVinextLocaleGlobals() — a minor drift risk, not a blocker.

github run

@james-elicx
james-elicx merged commit bacee9e into main Jun 15, 2026
57 checks passed
@james-elicx
james-elicx deleted the codex/fix-prerender-data-routes branch June 15, 2026 22:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants