fix(pages): match Next.js script loader placement#2566
Conversation
commit: |
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Performance benchmarksCompared 1 improved · 0 regressed · 5 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
|
/bigbonk review for issues |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: fix(pages): match Next.js script loader placement
Reviewed the script-loader extraction, the span-stripping placement logic, and dev/prod/Worker parity. One real dev/prod parity bug, plus a couple of notes.
1. Dev/prod parity gap: page-level beforeInteractive scripts are not hoisted in dev
In production (and Workers), renderPagesPageResponse wraps the page tree in a BeforeInteractiveContext.Provider collector and hoists page-level <Script strategy="beforeInteractive"> into <head>:
// packages/vinext/src/server/pages-page-response.ts
const beforeInteractiveScripts: BeforeInteractiveInlineScript[] = [];
const wrapWithBeforeInteractiveCollector = (element) =>
React.createElement(BeforeInteractiveContext.Provider, { value(script) { beforeInteractiveScripts.push(script); } }, element);
...
wrapPageElement: wrapWithBeforeInteractiveCollector, // passed to runDocumentRenderPage
...
wrapWithBeforeInteractiveCollector(React.createElement(React.Fragment, null, options.createPageElement(renderProps))) // streaming pathThe dev server (packages/vinext/src/server/dev-server.ts) does not do this. It only collects _document-registered scripts (documentBeforeInteractiveScripts) via DocumentScriptContext, and renders the page element with no BeforeInteractiveContext.Provider:
runDocumentRenderPage({ ... })is called withoutwrapPageElement(compare pages-page-response.ts:599).- The streaming page
element(dev-server.ts:414, and the tree built at dev-server.ts:1717-1730) is passed tostreamPageToResponseunwrapped (onlywithScriptNonce).
Consequence: a page-level <Script strategy="beforeInteractive"> (i.e. defined in a page/component, not in _document) is hoisted into <head> in prod but rendered inline in the body in dev. This is exactly the behavior the PR is trying to fix, and prod has a unit test for it (tests/pages-page-response.test.ts:215 "hoists legacy page beforeInteractive scripts ahead of Pages asset chunks") but dev has no equivalent and no coverage.
Per AGENTS.md ("Always check dev and prod server parity"), the dev path should mirror the prod collection/hoisting. Suggest:
- add a
wrapWithBeforeInteractiveCollectorin the dev server, pass it aswrapPageElementtorunDocumentRenderPage, and wrap the streamingelementwith it; - inject the collected page-level scripts into
<head>alongsidedocumentBeforeInteractiveHTML; - add a dev-mode e2e/unit test that a page-level
beforeInteractivescript lands in<head>(the existingscript-before-ready.tsxfixture uses the defaultafterInteractivestrategy, so it does not cover this).
Note the ordering is also not identical even for the _document scripts: dev injects documentBeforeInteractiveHTML immediately after <head> opens (before fontHeadHTML/ssrHeadHTML), while prod prepends them to assetTags and injects them after fontHeadHTML/ssrHeadHTML (${fontHeadHTML}${ssrHeadHTML}\n ${assetTags}\n</head>). Worth confirming which position matches Next.js and aligning the two paths.
2. CodeQL "polynomial regex" alert appears already resolved
The github-advanced-security comment flags a polynomial regex on <span . That was the original 9dd1cba implementation:
html.replace(/<span(?:\s[^>]*)?>\s*<!-- __NEXT_SCRIPTS__ -->\s*<\/span>/, ...)HEAD has since replaced this with indexOf/lastIndexOf-based scanning in both pages-page-response.ts and dev-server.ts, so the ReDoS surface is gone. The alert should clear on the next scan — no action needed beyond confirming it does. The VALID_ATTR_NAME regex in before-interactive-head.ts is anchored and linear, so it is not a concern.
3. optimizeDeps.include now always set (index.ts)
config() changed from conditionally spreading include to always emitting include: [...incomingInclude, react, react-dom, react-dom/client, react/jsx-runtime, react/jsx-dev-runtime]. This is deduped via new Set, so it's safe, but it's a behavioral change for every project (previously include was omitted when empty). Worth a one-line rationale in the PR body on why the new script-loader client import made this necessary — otherwise it reads as unrelated to the stated change.
Things that look correct
loadClientScriptdedup semantics (LoadCache/ScriptCache,onReadyNOT firing on the in-flight dedup path) match Next.jsloadScriptexactly.initScriptLoaderorder (runhandleClientScriptLoadper item, then seedbeforeInteractive/beforePageRenderfrom the DOM) matches Next.jsinitScriptLoader+addBeforeInteractiveToCache.- Client-side suppression of already-hoisted
beforeInteractivescripts (hasHoistedScript || hasAppNavigationRuntimeBootstrap()) and theonReady-on-remount fix (seedingloadedScriptsfrom the hoisted DOM node) are sound and covered by the new e2e tests. - Span-stripping guards (
onlyWhitespaceBefore/onlyWhitespaceAfter,spanClose + 7for</span>) correctly target only theNextScriptwrapper span and no-op safely when absent.
Targeted tests pass locally: tests/script.test.ts + tests/pages-page-response.test.ts (102), tests/entry-templates.test.ts + tests/build-optimization.test.ts + tests/pages-router.test.ts (539), and vp check is clean on the changed source files.
|
Review posted to PR #2566. Summary of findings: Review summary1 real bug (dev/prod parity): Page-level CodeQL alert (already resolved): The flagged polynomial regex on Note: Verified correct: dedup semantics ( |
|
/bigbonk review for issues |
|
Exact-head CI exposed a related streaming regression from the Validation: targeted streaming 3/3, Pages dev script 3/3, Pages prod 14/14, exact Next.js |
There was a problem hiding this comment.
Review: fix(pages): match Next.js script loader placement
Re-reviewed after the latest commits. The previous review's #1 (page-level beforeInteractive not hoisted in dev) is now resolved — the dev server wraps the page tree with beforeInteractiveCollector.wrapPageElement and injects into <head>, with matching dev + prod e2e coverage. Good.
However, the newest commit (8d7858f "await suspended scripts before document render") introduces a production streaming regression plus two threshold failures. vp test run tests/pages-router.test.ts currently fails (5 tests).
1. BLOCKER — await ...allReady breaks incremental SSR streaming in production
renderPagesPageResponse now awaits the full render before building the shell:
// packages/vinext/src/server/pages-page-response.ts:629-632
bodyStream = await options.renderToReadableStream(pageElement);
...
await waitForBeforeInteractiveCollection(bodyStream); // awaits bodyStream.allReadywaitForBeforeInteractiveCollection awaits stream.allReady, which only resolves once every Suspense boundary has resolved. Because this happens before buildPagesCompositeStream runs, the shell prefix is no longer flushed ahead of deferred content — the whole page is fully rendered before the first byte goes out. On main there is no such await; the body stream is passed straight through and streamed incrementally.
This is confirmed by the failing production streaming tests:
streams Pages SSR responses incrementally in production with br compression—firstChunkMsno longer< 400, and the first chunk now contains"Delayed stream content loaded"instead of the"Loading delayed chunk..."fallback....with gzip compression— same.strips stale content-length from streamed Pages SSR responses when gSSP sets one— same.strips middleware-provided content-length when rewriting to a streamed Pages SSR response— first chunk now contains the fully-resolved<head>+ resolved body.
Note this over-shoots Next.js: the App Router path in this same PR (app-ssr-entry.ts:697-704) explicitly documents that beforeInteractive ordering "applies to scripts rendered in the initial shell" and that scripts rendered later inside a Suspense boundary fall back to their inline location. Blocking the entire stream on allReady to hoist a beforeInteractive script that lives inside a suspended boundary is a behavior Next.js itself does not guarantee, and it costs incremental streaming (and TTFB) on every Pages SSR response, suspended script or not.
Suggested direction: don't block the shell on allReady. Collect only the scripts available at shell-flush time (mirroring the App Router snapshot semantics), and let a page-level beforeInteractive script that renders inside a late Suspense boundary fall back to inline — matching the documented App Router contract. The dev server (dev-server.ts:427) has the identical await waitForBeforeInteractiveCollection(bodyStream) and the same streaming hazard; fix both together for dev/prod parity per AGENTS.md.
2. Client entry bundle exceeds the 25 KB budget
produces client bundle with page chunks and SSR manifest fails: expected 25643 to be less than 25600. Pulling initScriptLoader (and transitively script-loader.ts, ~261 lines) into pages-client-entry.ts pushed the hydration-bootstrap entry chunk just over the 25 KB budget (the test comment: "just the hydration bootstrap, not the entire React framework").
This is a real regression against a deliberate budget, not just a stale threshold. Prefer keeping the entry lean — e.g. dynamically import the script-loader init only when nextData.scriptLoader is present, so pages with no <Script> usage don't pay for it — rather than bumping the limit. If the size is genuinely justified, that should be called out explicitly and the threshold updated with a rationale.
3. Dev/prod head-position divergence for hoisted beforeInteractive scripts
Even setting aside the streaming bug, the injection position differs between the two paths:
- dev (
dev-server.ts:543-552) splicesbeforeInteractiveHTMLimmediately after<head>opens, beforefontHeadHTML/ssrHeadHTML. - prod (
pages-page-response.ts:372, 406-407) prepends the scripts toassetTagsand injects them afterfontHeadHTML/ssrHeadHTML(${fontHeadHTML}${ssrHeadHTML}\n ${assetTags}).
The App Router reference path in this repo splices beforeInteractive scripts "immediately after <head ...> opens" (app-ssr-stream.ts:426-428, before-interactive-head.ts:13), specifically so the script precedes React-hoisted resource hints and font <link rel="stylesheet"> (the no-flash pattern the shim comment at script.tsx:426-433 describes). The dev position matches that; the prod position (after fonts) diverges from both dev and the App Router contract. The current e2e tests don't catch this because they only assert pageBeforeDocument / pageInHead, not position relative to font links. Align prod to inject before fontHeadHTML/ssrHeadHTML (matching dev + App Router) and add an assertion pinning script-before-font ordering.
Things that look correct
loadClientScriptdedup semantics (LoadCache/ScriptCache,onReadyNOT firing on the in-flight dedup path) match Next.jsloadScript.initScriptLoaderordering (runhandleClientScriptLoadper item, then seedbeforeInteractive/beforePageRenderfrom the DOM) matches Next.js.- Client-side suppression of already-hoisted scripts (
hasHoistedScript || hasAppNavigationRuntimeBootstrap()) and theonReady-on-remount seeding are sound. - Span-stripping guards (
onlyWhitespaceBefore/onlyWhitespaceAfter,spanClose + 7) correctly target only theNextScriptwrapper span and no-op safely. The double.replace(placeholder, scripts)after the span-strip branch is safe (placeholder already removed). - CodeQL polynomial-regex alert is stale — HEAD replaced the
<span ...>regex with linearindexOf/lastIndexOfscanning in bothpages-page-response.tsanddev-server.ts.VALID_ATTR_NAMEis anchored/linear. optimizeDeps.includenow always set with React deps — deduped vianew Set, safe; still worth a one-line rationale in the PR body.
tests/script.test.ts + tests/pages-page-response.test.ts (102) pass; tests/pages-router.test.ts fails (5) as detailed above.
|
Review posted to PR #2566. SummaryThe previous review's main concern (page-level 1 BLOCKER — production streaming broken. Bundle budget exceeded. Pulling Dev/prod head-position divergence. Dev injects hoisted scripts immediately after Verified correct: dedup semantics, |
|
The latest Bonk review is stale relative to the exact commit it labels: all findings reference /bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(pages): match Next.js script loader placement
Re-reviewed at exact head 23081da7e. The prior Bonk review is confirmed stale — all its findings referenced waitForBeforeInteractiveCollection and the suspended-script fixture from 8d7858f, both of which the final commit (23081da "preserve suspense streaming for scripts") deletes.
Stale findings — verified resolved at head
- BLOCKER #1 (
await ...allReadybreaks streaming) — resolved.waitForBeforeInteractiveCollectionand both call sites (pages-page-response.ts,dev-server.ts) are gone (git diff 8d7858f..23081da). Both paths now readbeforeInteractiveCollector.scriptsimmediately afterawait renderToReadableStream(...), which resolves at shell-ready time — so shell-levelbeforeInteractivescripts are collected and hoisted, while scripts inside a still-pending Suspense boundary fall back to inline. That matches the documented App Router shell-snapshot contract and restores incremental streaming.tests/pages-router.test.tsnow passes 352/352, including all four production streaming tests (firstChunkMs < 400, first chunk carries the Suspense fallback). - #2 (bundle budget 25643 > 25600) — no longer failing. The budget assertion at
tests/pages-router.test.ts:4565is unchanged (< 25 * 1024) and the "produces client bundle with page chunks and SSR manifest" test passes. (Note below: the entry still eagerly importsinitScriptLoader; headroom is thin.) - CodeQL polynomial regex — stale. HEAD scans with
indexOf/lastIndexOfin bothpages-page-response.ts:381-392anddev-server.ts:495-506;VALID_ATTR_NAMEis anchored/linear.
1. Dev/prod parity: hoisted beforeInteractive position diverges (still present)
This is the one substantive finding that survives at head — and it directly undercuts the PR's stated goal. The prod Pages path and the dev path inject hoisted beforeInteractive scripts at different positions within <head>:
- dev (
dev-server.ts:538-547) splicesbeforeInteractiveHTMLimmediately after<head ...>opens, beforefontHeadHTML/headHTML/asset tags. - prod (
pages-page-response.ts:368-378) prepends the scripts toassetTagsand injects the whole block after${fontHeadHTML}${ssrHeadHTML}(<head>→ fonts → next/head → beforeInteractive → other asset tags).
So in prod the beforeInteractive script runs after font stylesheets and next/head <link rel="stylesheet">; in dev it runs before them. This breaks the exact no-flash contract this PR documents:
before-interactive-head.ts:13— "ready to splice immediately after<head ...>opens"script.tsx:426-433— "beforeInteractive scripts need to run BEFORE any stylesheets, modulepreload links, or other resource hints ... the SSR pipeline emits it immediately after<head>opens — guaranteeing it precedes every React-emitted hint"- App Router reference path (
app-ssr-stream.ts:426-428) does exactly this.
Dev matches the contract; prod diverges. This is a regression introduced by this PR — on base (aa1b666) prod assetTags had no beforeInteractive prefix and the App Router before-interactive-head helper wasn't used for Pages. Per the AGENTS.md dev/prod parity rule, align prod to inject before ${fontHeadHTML}${ssrHeadHTML} (matching dev + App Router).
The existing e2e (tests/e2e/pages-router/script.spec.ts) can't catch this — it asserts pageInHead/pageBeforeDocument/documentBeforeVite but never position relative to font/stylesheet links. Add an assertion pinning beforeInteractive-before-font-stylesheet ordering so the two paths can't silently drift again.
2. Note: initScriptLoader is still imported eagerly (thin budget headroom)
pages-client-entry.ts:145 statically imports initScriptLoader and always calls it (:220). The prior review's suggestion to gate it on nextData.scriptLoader was not adopted. The budget test passes today, but the margin is small; a lazy import for pages with no <Script> usage would give headroom and keep the hydration bootstrap lean. Non-blocking.
3. Note: optimizeDeps.include now always set (index.ts:2918)
config() always emits include: [react, react-dom, react-dom/client, react/jsx-runtime, react/jsx-dev-runtime, ...incoming] (previously omitted when empty). Deduped via new Set, so safe, but it's a behavioral change for every project and reads as unrelated to script loading. Worth a one-line rationale in the PR body on why the client script-loader import made it necessary.
Verified correct
loadClientScriptdedup semantics — in-flightscriptCachehit does not fireonReady, matching Next.jsloadScript(script-loader.ts:158-172).initScriptLoaderordering — runshandleClientScriptLoadper item, then seedsbeforeInteractive/beforePageRenderfrom the DOM (script-loader.ts:247-261), matching Next.js.- Client-side suppression of already-hoisted scripts +
onReady-on-remount seeding are sound and e2e-covered. - Span-stripping guards (
onlyWhitespaceBefore/onlyWhitespaceAfter,spanClose + 7) target only theNextScriptwrapper span and no-op safely when absent, in both dev and prod. - Streaming shell-snapshot semantics are correct: reading
beforeInteractiveCollector.scriptsafter the shell-readyawaitcollects shell scripts without blocking onallReady.
Tests run at head 23081da7e
tests/pages-router.test.ts— 352 passed (incl. 4 prod streaming, first chunk < 400ms with fallback)tests/script.test.ts+tests/pages-page-response.test.ts+tests/entry-templates.test.ts— 143 passedtests/build-optimization.test.ts(bundle) budget unchanged and green
Verdict: the streaming BLOCKER is genuinely fixed and the prior review is stale. The remaining actionable item is the prod-vs-dev beforeInteractive head-position divergence (#1 above), which should be aligned + pinned with a test before merge since it's exactly the placement this PR is meant to fix.
|
Review complete. Posted a fresh independent review to PR #2566 at exact head SummaryYour claim is verified. The prior Bonk review was stale — every finding referenced Stale findings confirmed resolved:
One real finding that survives at head (and undercuts the PR goal):
Two non-blocking notes: Verdict: the streaming BLOCKER is genuinely fixed and the prior review is stale; the head-position parity gap is the remaining actionable item. |
|
Addressed the exact-head Bonk placement finding at Validation: response + production streaming 3/3, Pages dev script 3/3, pinned Next.js |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(pages): match Next.js script loader placement
Re-reviewed at exact head 73e558a6e ("align beforeInteractive head ordering"). The prior review's one surviving finding — the dev/prod beforeInteractive head-position divergence — is now fixed, with test coverage. No blockers found at head.
Resolved: prod beforeInteractive head position now matches dev + App Router
On the previously reviewed commit, prod folded hoisted beforeInteractive scripts into assetTags (injected after ${fontHeadHTML}${ssrHeadHTML}), while dev spliced them immediately after <head> opens. This commit routes both paths through the same insertAfterHeadOpen seam:
- prod (
pages-page-response.ts:399,:408,:416) now callsinsertBeforeInteractiveScripts(html, beforeInteractiveScripts)→insertAfterHeadOpen(before-interactive-head.ts:74-81), splicing right after<head>opens, before fonts /next/head/ asset tags.assetTagsno longer carries abeforeInteractiveprefix. - dev (
dev-server.ts:542) uses the identicalinsertAfterHeadOpenon the transformed shell.
Both now match the App Router no-flash contract documented at script.tsx:426-433 and before-interactive-head.ts:11-13.
Ordering is now pinned by tests (the prior review's explicit ask):
- Unit:
pages-page-response.test.ts:243-245asserts the hoisted script index < font stylesheet (/font.css) <next/headtag < asset chunk. - E2E:
tests/e2e/pages-router/script.spec.ts:13asserts raw-HTMLid="page-before"precedesname="description", plus DOMpageInHead/pageBeforeDocument/documentBeforeViteandbodyCopies: 0.
Streaming shell-snapshot semantics verified correct
beforeInteractiveCollector.scripts is read (pages-page-response.ts:658) after await renderToReadableStream(pageElement) (:626), which resolves at shell-ready under react-dom/server.edge. Shell-level beforeInteractive scripts hoist; scripts inside a still-pending Suspense boundary fall back to inline — matching the App Router contract, without blocking the stream. No double collection: runDocumentRenderPage short-circuits skipped (pages-document-initial-props.ts:180-186) unless a user getInitialProps+renderPage (CSS-in-JS) path is active, in which case the streaming block (:610-627) is skipped.
Prod/Worker share renderPagesPageResponse via pages-page-handler.ts:761; dev mirrors it in dev-server.ts. All three paths aligned per the AGENTS.md dev/prod parity rule.
Stale findings from prior Bonk reviews — confirmed resolved at head
- BLOCKER
await ...allReadybreaks streaming — gone.waitForBeforeInteractiveCollectionand both call sites deleted. Prod streaming tests pass (first chunk< 400ms, carries the Suspense fallback). - Bundle budget 25643 > 25600 — no longer failing;
< 25 KBassertion (pages-router.test.ts:4565) is green. - CodeQL polynomial regex on
<span— stale. Bothpages-page-response.ts:383anddev-server.ts:500scan with linearindexOf/lastIndexOf;VALID_ATTR_NAME(before-interactive-head.ts:9) is anchored/linear.
Non-blocking notes (unchanged from prior review, not adopted)
initScriptLoaderstill eagerly imported (pages-client-entry.ts:145, called at:220). Budget headroom is thin; a lazy import gated onnextData.scriptLoaderwould keep the hydration bootstrap lean for pages with no<Script>. On an emptyscriptLoaderarrayinitScriptLoaderstill runs twoquerySelectorAllcalls — trivial, but avoidable.optimizeDeps.includenow always set (index.ts:2921-2930) with React deps (previously omitted whenincomingIncludewas empty). Deduped vianew Set, so safe, but it reads as unrelated to script-loader work — worth a one-line rationale in the PR body.
Verified correct
loadClientScriptdedup — in-flightscriptCachehit does not fireonReady(script-loader.ts:158-172), matching Next.jsloadScript.initScriptLoaderordering — per-itemhandleClientScriptLoad, then seedbeforeInteractive/beforePageRenderfrom the DOM (script-loader.ts:247-261).- Client-side suppression of hoisted scripts (
hasHoistedScript || hasAppNavigationRuntimeBootstrap(),script.tsx:489-494) +onReady-on-remount seeding (seedHoistedBeforeInteractiveScript) — sound and e2e-covered. - Span-stripping guards (
onlyWhitespaceBefore/onlyWhitespaceAfter,spanClose + 7) target only theNextScriptwrapper span and no-op safely in both dev and prod. data-nscript="beforeInteractive"+data-vinext-script-keyemitted on hoisted scripts, matching Next.js's DOM dedupe contract.
Tests run at head 73e558a6e
tests/script.test.ts+tests/pages-page-response.test.ts— 102 passedtests/pages-router.test.tsprod streaming (br + gzip) — passed (first chunk 656ms/br with fallback)tests/pages-router.test.ts"produces client bundle with page chunks and SSR manifest" (budget) — passedtests/entry-templates.test.ts+tests/build-optimization.test.ts— 187 passedvp checkon all 8 changed source files — clean
Verdict: The head-position parity gap the PR was meant to fix is now genuinely aligned across dev/prod/Worker and pinned by unit + e2e ordering assertions. All prior blockers are resolved. Remaining items are non-blocking nits (eager initScriptLoader import, unconditional optimizeDeps.include rationale). LGTM.
|
Review posted to PR #2566 at exact head SummaryThe prior review's one surviving finding is fixed. The dev/prod
No blockers at head. All prior Bonk blockers are confirmed stale/resolved:
Two non-blocking nits carried over (not adopted):
Verified correct: streaming shell-snapshot semantics, dedup, Verdict: LGTM. Tests pass (102 + prod streaming + budget + 187), |
2 participants
Summary
afterInteractiveandlazyOnloadscripts as body siblings after#__NEXT_DATA__data-nscript="beforeInteractive"to server-rendered Pages scriptsbeforeInteractivescripts ahead of framework asset chunksNext.js parity
Ported from Next.js
v16.3.0-canary.80:test/e2e/script-loader/script-loader.test.tspackages/next/src/client/script.tsxpackages/next/src/pages/_document.tsxpackages/next/src/server/render.tsxThe Pages response helper is shared by dev, production, and Worker request paths, so the placement and collection behavior stays aligned across runtimes.
Validation
vp test run tests/script.test.ts tests/pages-page-response.test.ts— 88 passedvp check packages/vinext/src/shims/script.tsx packages/vinext/src/server/pages-document-initial-props.ts packages/vinext/src/server/pages-page-response.ts tests/script.test.ts tests/pages-page-response.test.ts— cleanvp run vinext#build— passed/Users/jamesanderson/Developer/vinext/.nextjs-ref-v16.3.0-canary.80:test/e2e/script-loader/script-loader.test.ts— 11 passedafterInteractive,lazyOnload, modernbeforeInteractive, and older page-levelbeforeInteractiveRun source:
28938793088.