fix(script): register src beforeInteractive scripts and mark hoisted output - #2019
Conversation
commit: |
|
/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
The core fix is correct and matches Next.js parity well. The three defects are addressed cleanly:
srcbeforeInteractive scripts now flow throughBeforeInteractiveContextand get hoisted into<head>(the SSR registration condition(src || inlineContent !== null)and the client suppression condition mirror each other exactly — good).- The
data-nscript="beforeInteractive"marker is emitted server-side, and the client returnsnullfor both inline andsrcscripts under the App Router runtime — Pages Router still renders inline sincehasAppNavigationRuntimeBootstrap()is false. This is preserved correctly. crossOrigin/referrerPolicylowercasing is added toREACT_TO_HTML_ATTR.
The extraction of renderBeforeInteractiveInlineScripts into server/before-interactive-head.ts aligns with the AGENTS.md guidance to keep entry/SSR modules thin and unit-test the helper directly. The duplicate-data-nscript guard and the innerHTML ?? "" handling for src-only scripts are both correct. I verified the fix propagates to the streaming path (app-ssr-stream.ts consumes the same injectAfterHeadOpenHTML), and tests/script.test.ts passes locally (36/36).
Blocking issue
Two broken symlinks were accidentally committed — they point to a machine-local macOS temp path (/var/folders/t4/...) and are dangling on any other machine (confirmed broken on Linux CI). They are leaked test fixtures created by tests/oxlint-prefer-shared-utils.test.ts (which normally cleans them up in afterEach) and must be removed before merge:
packages/vinext/src/__lint_rule_fixtures__-vinext-lint-rule-fixtures-kgEEgCtests/__lint_rule_fixtures__-vinext-lint-rule-fixtures-kgEEgC
Neither exists on main; both are introduced by this PR. Please git rm them. It may also be worth adding __lint_rule_fixtures__-* to .gitignore to prevent this class of accidental commit in the future.
Everything else looks good.
| @@ -0,0 +1 @@ | |||
| /var/folders/t4/9xd5wqhx52d_3y9s8hstqk4w0000gn/T/vinext-lint-rule-fixtures-kgEEgC No newline at end of file | |||
There was a problem hiding this comment.
Accidentally committed broken symlink. It points to a machine-local macOS temp path (/var/folders/t4/9xd5wqhx52d_3y9s8hstqk4w0000gn/T/...) and is dangling everywhere else. This is a leaked fixture from tests/oxlint-prefer-shared-utils.test.ts (createFixtureDir), which normally removes it in afterEach. Please git rm this file — it should not be in the repo.
| @@ -0,0 +1 @@ | |||
| /var/folders/t4/9xd5wqhx52d_3y9s8hstqk4w0000gn/T/vinext-lint-rule-fixtures-kgEEgC No newline at end of file | |||
There was a problem hiding this comment.
Same broken symlink as the one under packages/vinext/src/. This is the test-side fixture link from writeTestFixture in tests/oxlint-prefer-shared-utils.test.ts. Please git rm it. Consider adding __lint_rule_fixtures__-* to .gitignore so a test interrupted mid-run can't leak these into a future commit.
|
Review submitted successfully. Here's a summary of my findings: Review Summary for PR #2019Verdict: Changes requested (one blocking issue) Core fix — correct ✅The three defects are addressed cleanly and match Next.js behavior:
I verified:
Blocking issue ❌Two broken symlinks were accidentally committed, pointing to a machine-local macOS temp path (
These are leaked fixtures from |
…output ## Summary - Register `beforeInteractive` Scripts with `src` through BeforeInteractiveContext so they are hoisted into <head> ahead of interactive, exactly like the inline form (previously they bypassed the registry and rendered in source order, after React Float's head content). - Suppress the client React render for both inline AND src beforeInteractive Scripts in the App Router so the hoisted tag is never duplicated. - Tag every hoisted script with Next.js's `data-nscript="beforeInteractive"` marker. - Map `crossOrigin` -> `crossorigin` and `referrerPolicy` -> `referrerpolicy` on hoisted scripts (parity with Next.js set-attributes-from-props). - Extract `renderBeforeInteractiveInlineScripts` into a dedicated, pure module (before-interactive-head.ts) so the emit path is unit-testable. ## Root Cause The beforeInteractive SSR branch computed `inlineContent = src ? null : ...` and only registered when `inlineContent !== null`, so any Script with a `src` was never captured by the registry and never spliced into <head>; it stayed where the user wrote it, behind React Float's hoisted resource hints, defeating the before-interactive guarantee. Hoisting src scripts then requires the client to stop rendering them through React (the React-created <script> never executes and would only add a duplicate DOM node), so the client dedupe condition is aligned with the SSR registration condition. The hoisted output also lacked Next.js's `data-nscript` marker, and REACT_TO_HTML_ATTR omitted crossOrigin/referrerPolicy, so those attribute names did not match Next.js's lowercased output. ## References - Fixes cloudflare#2016 - Next.js: .nextjs-ref/packages/next/src/client/script.tsx (data-nscript, src/inline registry parity), set-attributes-from-props.ts (lowercased attrs) - Ported: test/e2e/app-dir/script-before-interactive/script-before-interactive.test.ts ## Verification - CI=true pnpm test tests/script.test.ts tests/script-head-ordering.test.ts tests/nextjs-compat/script-nonce.test.ts tests/shims.test.ts -> 1139 passed - CI=true npx vp check on all changed files -> pass Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2553fa4 to
f7eba35
Compare
|
Good catch — those two Removed both with Thanks for verifying parity and the streaming path ( |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review
The fix is correct, internally consistent, and well-tested. The previously-flagged blocking issue (two committed broken symlinks) has been resolved in f7eba35 — the file list no longer contains them and .gitignore now guards against recurrence. Nothing blocking remains.
Verified
- SSR / client conditions mirror exactly. SSR registers when
(src || inlineContent !== null) && registerBeforeInteractive(script.tsx:601); the client suppresses when(src || inlineContent !== null) && hasAppNavigationRuntimeBootstrap()(script.tsx:651). In the App Router SSR entry theBeforeInteractiveContext.Provideris always wired (app-ssr-entry.ts:440-444), and the App Router client always has the navigation runtime bootstrap, so "what got hoisted" and "what the client suppresses" stay in lockstep. Pages Router has no provider and no bootstrap, so both paths render inline — also consistent. - No client double-execution. The client
useEffectearly-returns forbeforeInteractiveafter only inserting stylesheets (script.tsx:465-473); it never callsload(), so the server-hoisted<script src>is the sole loader. - Attribute mapping + escaping.
crossOrigin/referrerPolicyare lowercased viaREACT_TO_HTML_ATTR;nonce/src/idare RESERVED so they aren't double-emitted; values pass throughescapeHtmlAttr; thedata-nscriptdedupe guard is correct. - Helper extraction into
server/before-interactive-head.tsmatches the AGENTS.md "keep entries thin" guidance and is now directly unit-tested. tests/script.test.ts(36/36) andtests/script-head-ordering.test.ts(8/8) pass locally.
Parity note (non-blocking)
I checked the authoritative Next.js source (packages/next/src/client/script.tsx). In the App Router, Next.js does not emit a raw <script src data-nscript> into <head>. For both inline and src beforeInteractive scripts it renders a (self.__next_s=...).push([src|0, {...}]) queue entry; initScriptLoader/loadScript then creates the element, appends it to document.body, and applies el.setAttribute('data-nscript', strategy) at runtime. So the data-nscript marker is a client-applied DOM attribute on a body-injected element, not a server-hoisted head tag.
This PR continues vinext's pre-existing intentional divergence (hoisting a real tag into <head> for the no-flash pattern) and extends it from inline to src. That's a reasonable, self-consistent choice, but it is a deliberate divergence worth noting: a src script hoisted into <head> with neither async nor defer is render-blocking and executes in head order, whereas Next.js's runtime injection appends to <body>. Per AGENTS.md this kind of deliberate, documented divergence is acceptable — flagging it so it's a conscious decision rather than an accidental one.
Minor (optional)
Pages Router server-rendered beforeInteractive scripts (the buildBeforeInteractiveScriptProps path) still don't get the data-nscript="beforeInteractive" marker, whereas Next.js applies it in both routers. This is a pre-existing gap outside this PR's stated App Router scope, so it's fine to leave — just noting it for completeness.
LGTM once the parity divergence is acknowledged as intentional.
|
Review posted. Summary of my findings: Verdict: Approve (no blocking issues)The previously-flagged blocking issue — two committed broken symlinks pointing to a machine-local macOS temp path — has been resolved in The fix is correct and well-tested
One non-blocking parity noteI checked the authoritative Next.js source: in the App Router, Next.js does not hoist a raw I also noted a minor pre-existing gap (Pages Router beforeInteractive scripts lack the |
2 participants
Summary
srcbeforeInteractivenext/scripts (not just inline ones) so they are hoisted into<head>server-side, matching inline behavior.beforeInteractivescript withdata-nscript="beforeInteractive"and suppress the duplicate client render for both inline andsrcscripts, preventing double execution / hydration mismatch.crossOrigin→crossoriginandreferrerPolicy→referrerpolicywhen serializing hoisted script attributes.Root Cause
Three defects in App Router
beforeInteractivehandling:srcscripts bypassed the server registry. Inshims/script.tsxthe SSR branch computedinlineContent = src ? null : …and only registered wheninlineContent !== null, so externalsrcscripts were never captured and never hoisted into<head>before interactive — they stayed in source order behind React's resource hints. The condition now registers whensrcis present too.<Script>produced a second<script>node (double execution / hydration mismatch). Next.js marks server-renderedbeforeInteractivescripts withdata-nscript="beforeInteractive"and dedupes on the client; we now emit the same marker and returnnullon the client for both inline andsrcbeforeInteractivescripts in App Router (afterInteractive/lazyOnload paths are unchanged).REACT_TO_HTML_ATTRlackedcrossOrigin/referrerPolicy, so they round-tripped as camelCase; added the lowercase mappings to match Next.js' attribute serialization.For testability the pure emit logic (
renderBeforeInteractiveInlineScripts) was extracted from the virtual-import-heavyapp-ssr-entry.tsinto a newserver/before-interactive-head.ts; attribute values are HTML-escaped at that boundary.References
next/scriptbeforeInteractive: hydration duplicate,srcbypasses server registry, attr-map gaps #2016packages/next/src/client/script.tsx(data-nscriptmarker +addBeforeInteractiveToCachededupe onid || src)test/e2e/app-dir/script-before-interactive/script-before-interactive.test.ts(the multiple-scripts case)Verification
CI=true pnpm test tests/script.test.ts tests/script-head-ordering.test.ts— new tests cover:srcbeforeInteractivehoisted into<head>;crossOrigin/referrerPolicylowercased on hoisted tags; client does not re-render a hoistedsrcscript; every hoisted script carriesdata-nscript; multiple hoisted scripts (ported from Next.js). All green (red first).CI=true pnpm test— full battery green (only the pre-existing env flakesdeploy.test.ts > resolveWranglerBinandoxlint-prefer-shared-utils, reproduced identically onorigin/main).CI=true npx vp check— clean on all changed files.