fix(shims/script): emit <link rel="stylesheet"> for next/script stylesheets prop#1646
fix(shims/script): emit <link rel="stylesheet"> for next/script stylesheets prop#1646james-elicx wants to merge 1 commit into
Conversation
…sheets prop Refs #1517. The vinext `next/script` shim ignored the `stylesheets` prop, so an `<Script src="/x.js" stylesheets={['/x.css']} />` element emitted only the `<script>`/preload and silently dropped the associated stylesheet (and worse, leaked the prop onto the rendered `<script>` as a `stylesheets="..."` attribute on the beforeInteractive branch). Mirror the App-Router branch of Next.js's component (`.nextjs-ref/packages/next/src/client/script.tsx:309-313` and `:48-59`): - SSR: call `ReactDOM.preinit(href, { as: 'style' })` for each entry — React Float hoists `<link rel="stylesheet">` into `<head>`. - Client load path (`loadClientScript` + `handleClientScriptLoad`): same preinit preferred; fall back to direct `document.head.appendChild` of `<link rel="stylesheet">` when `ReactDOM.preinit` is unavailable. - Destructure `stylesheets` out of `rest` everywhere so it never lands as an attribute on the emitted `<script>`. - Add `stylesheets` to the `RESERVED` set used by the hoisted-script attribute collector for inline beforeInteractive scripts. Scope: stylesheets prop only. Nonce propagation (the other half of #1517) is already handled across SSR (`buildBeforeInteractiveScriptProps`, `ReactDOM.preload`) and client (`loadClientScript`) paths — see the existing "Script nonce resolution" suite. Bootstrap-script preinit is a separate, larger change tracked under #1328. Tests: extend `tests/script.test.ts` with a `Script stylesheets prop` suite covering SSR emission, multi-entry lists, prop leakage on the rendered `<script>`, omitted/empty lists, and the imperative `handleClientScriptLoad` path.
|
/bigbonk review |
commit: |
|
|
/bigbonk review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: fix(shims/script): emit <link rel="stylesheet"> for next/script stylesheets prop
Verdict: Approve
Clean, well-scoped PR. Verified against the Next.js reference implementation at .nextjs-ref/packages/next/src/client/script.tsx and .nextjs-ref/packages/next/src/client/set-attributes-from-props.ts.
Correctness
All integration points are handled:
- SSR:
preinitStylesheetsForSSRcallsReactDOM.preinit(href, { as: 'style' })for every strategy, matching Next.js's app-routerappDirblock (lines 309-313). - Client load:
insertClientStylesheetsmirrors Next.js'sinsertStylesheets(lines 29-60) — prefersReactDOM.preinitfor Float-based dedup, falls back to direct DOM<link>injection. - Attribute filtering:
stylesheetsis destructured out ofrestin bothScriptandloadClientScript, and added to theRESERVEDset incollectBeforeInteractiveAttributes. This matches Next.js'signorePropsarray inset-attributes-from-props.ts:16. - useEffect deps:
stylesheetsis correctly in the dependency array.
Intentional improvement over Next.js
In loadClientScript, insertClientStylesheets is called before the loadedScripts.has(key) early return. In Next.js's loadScript, insertStylesheets runs after the LoadCache early return (line 78-80 vs 151), meaning re-mounted scripts with the same key silently drop their stylesheets. The vinext version is more robust here — good call.
Tests
6 new tests cover the key scenarios: SSR emission, multi-stylesheet lists, attribute non-leakage, omitted/empty arrays, and client-side handleClientScriptLoad. All 28 script tests and all 8 head-ordering tests pass.
Looks good.
|
Review posted as APPROVE on PR #1646. The PR correctly implements the |
1 participant
Summary
Fixes part of #1517 — the
next/scriptstylesheetsprop is now honoured.The vinext shim ignored the
stylesheetsprop entirely.<Script src="/x.js" stylesheets={['/x.css']} />would (a) emit only the<script>/preload and silently drop the associated stylesheet and (b) leak the prop onto the rendered<script>as astylesheets="..."attribute forbeforeInteractive.Now mirrors the App-Router branch of Next.js's component (
.nextjs-ref/packages/next/src/client/script.tsx:309-313and:48-59):ReactDOM.preinit(href, { as: 'style' })for each entry — React Float hoists<link rel="stylesheet">into<head>.loadClientScript+handleClientScriptLoad): preinit preferred; falls back to directdocument.head.appendChildof<link rel="stylesheet">whenReactDOM.preinitis unavailable.stylesheetsout ofresteverywhere so it never lands as an attribute on the emitted<script>.stylesheetsto theRESERVEDset used by the hoisted-script attribute collector for inline beforeInteractive scripts.Scope choice
Issue #1517 lists three sub-items. Per the issue workflow (prefer smaller PRs scoped to one part):
buildBeforeInteractiveScriptProps, theReactDOM.preloadnonce branch,loadClientScript'sel.setAttribute('nonce', …), and the existing "Script nonce resolution" test suite (tests/script.test.ts:325-468, plus #1608/#1607). Spot-checked: SSR<script>tags, the React Float preload link, and the client-imperative<script>all receive the resolved nonce.Test plan
pnpm test tests/script.test.ts— 28 pass (4 new tests added for the stylesheets prop).pnpm test tests/script-head-ordering.test.ts— 8 pass (no regression on inline beforeInteractive ordering).pnpm run check— formatter, lint, and type checks all clean.