-
Notifications
You must be signed in to change notification settings - Fork 326
fix(script): dedupe concurrent same-src loads #1263
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -49,8 +49,11 @@ export type ScriptProps = { | |||||||||||||||||||
| [key: string]: unknown; | ||||||||||||||||||||
| }; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| // Track scripts that have already been loaded to avoid duplicates | ||||||||||||||||||||
| // Track scripts that have already been loaded, plus remote scripts currently | ||||||||||||||||||||
| // loading, to avoid duplicate DOM insertion when same-src components mount | ||||||||||||||||||||
| // before the first load event fires. | ||||||||||||||||||||
| const loadedScripts = new Set<string>(); | ||||||||||||||||||||
| const loadingScripts = new Map<string, Promise<Event>>(); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| function getClientAutoNonce(): string | undefined { | ||||||||||||||||||||
| if (typeof document === "undefined") return undefined; | ||||||||||||||||||||
|
|
@@ -96,51 +99,118 @@ function buildBeforeInteractiveScriptProps(options: { | |||||||||||||||||||
| return scriptProps; | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| /** | ||||||||||||||||||||
| * Load a script imperatively (outside of React). | ||||||||||||||||||||
| */ | ||||||||||||||||||||
| export function handleClientScriptLoad(props: ScriptProps): void { | ||||||||||||||||||||
| function setScriptAttributes(el: HTMLScriptElement, rest: Record<string, unknown>): void { | ||||||||||||||||||||
| for (const [attr, value] of Object.entries(rest)) { | ||||||||||||||||||||
| if (attr === "dangerouslySetInnerHTML") continue; | ||||||||||||||||||||
| if (attr === "className") { | ||||||||||||||||||||
| el.setAttribute("class", String(value)); | ||||||||||||||||||||
| } else if (typeof value === "string") { | ||||||||||||||||||||
| el.setAttribute(attr, value); | ||||||||||||||||||||
| } else if (typeof value === "boolean" && value) { | ||||||||||||||||||||
| el.setAttribute(attr, ""); | ||||||||||||||||||||
| } | ||||||||||||||||||||
| } | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| function loadClientScript( | ||||||||||||||||||||
| props: ScriptProps, | ||||||||||||||||||||
| options: { | ||||||||||||||||||||
| resolvedNonce?: string; | ||||||||||||||||||||
| fireReadyWhenAlreadyLoaded: boolean; | ||||||||||||||||||||
| }, | ||||||||||||||||||||
| ): void { | ||||||||||||||||||||
| const { | ||||||||||||||||||||
| src, | ||||||||||||||||||||
| id, | ||||||||||||||||||||
| onLoad, | ||||||||||||||||||||
| onReady, | ||||||||||||||||||||
| onError, | ||||||||||||||||||||
| strategy: _strategy, | ||||||||||||||||||||
| onReady: _onReady, | ||||||||||||||||||||
| strategy = "afterInteractive", | ||||||||||||||||||||
| children, | ||||||||||||||||||||
| dangerouslySetInnerHTML, | ||||||||||||||||||||
| ...rest | ||||||||||||||||||||
| } = props; | ||||||||||||||||||||
| if (typeof window === "undefined") return; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| const key = id ?? src ?? ""; | ||||||||||||||||||||
| if (key && loadedScripts.has(key)) return; | ||||||||||||||||||||
| if (key && loadedScripts.has(key)) { | ||||||||||||||||||||
| if (options.fireReadyWhenAlreadyLoaded) { | ||||||||||||||||||||
| onReady?.(); | ||||||||||||||||||||
| } | ||||||||||||||||||||
| return; | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| if (src) { | ||||||||||||||||||||
| const existingLoad = loadingScripts.get(src); | ||||||||||||||||||||
| if (existingLoad) { | ||||||||||||||||||||
| void existingLoad.then( | ||||||||||||||||||||
| (event) => { | ||||||||||||||||||||
| if (key) loadedScripts.add(key); | ||||||||||||||||||||
| onLoad?.(event); | ||||||||||||||||||||
| onReady?.(); | ||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Worth noting: this is actually more correct than Next.js, which silently drops |
||||||||||||||||||||
| }, | ||||||||||||||||||||
| (event) => onError?.(event), | ||||||||||||||||||||
| ); | ||||||||||||||||||||
| return; | ||||||||||||||||||||
| } | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| const el = document.createElement("script"); | ||||||||||||||||||||
| if (src) el.src = src; | ||||||||||||||||||||
| if (id) el.id = id; | ||||||||||||||||||||
| const resolvedNonce = resolveScriptNonce(rest.nonce); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| for (const [attr, value] of Object.entries(rest)) { | ||||||||||||||||||||
| if (attr === "dangerouslySetInnerHTML" || attr === "className") continue; | ||||||||||||||||||||
| if (typeof value === "string") { | ||||||||||||||||||||
| el.setAttribute(attr, value); | ||||||||||||||||||||
| } else if (typeof value === "boolean" && value) { | ||||||||||||||||||||
| el.setAttribute(attr, ""); | ||||||||||||||||||||
| } | ||||||||||||||||||||
| setScriptAttributes(el, rest); | ||||||||||||||||||||
| if (options.resolvedNonce && !el.getAttribute("nonce")) { | ||||||||||||||||||||
| el.setAttribute("nonce", options.resolvedNonce); | ||||||||||||||||||||
| } | ||||||||||||||||||||
| if (resolvedNonce && !el.getAttribute("nonce")) { | ||||||||||||||||||||
| el.setAttribute("nonce", resolvedNonce); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| if (strategy === "worker") { | ||||||||||||||||||||
| el.setAttribute("type", "text/partytown"); | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| if (children && typeof children === "string") { | ||||||||||||||||||||
| const markLoaded = () => { | ||||||||||||||||||||
| if (key) loadedScripts.add(key); | ||||||||||||||||||||
| onReady?.(); | ||||||||||||||||||||
| }; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| if (dangerouslySetInnerHTML?.__html) { | ||||||||||||||||||||
| // Intentional: mirrors the Next.js <Script> API where dangerouslySetInnerHTML | ||||||||||||||||||||
| // is developer-supplied inline script content (not user input). The prop name | ||||||||||||||||||||
| // itself signals developer awareness of the XSS risk, consistent with React's | ||||||||||||||||||||
| // design. User-supplied data must never flow into this prop. | ||||||||||||||||||||
| el.innerHTML = dangerouslySetInnerHTML.__html; | ||||||||||||||||||||
| markLoaded(); | ||||||||||||||||||||
| } else if (children && typeof children === "string") { | ||||||||||||||||||||
| el.textContent = children; | ||||||||||||||||||||
| markLoaded(); | ||||||||||||||||||||
| } else if (src) { | ||||||||||||||||||||
| const loadPromise = new Promise<Event>((resolve, reject) => { | ||||||||||||||||||||
| el.addEventListener("load", (event) => { | ||||||||||||||||||||
| resolve(event); | ||||||||||||||||||||
| if (key) loadedScripts.add(key); | ||||||||||||||||||||
| onLoad?.(event); | ||||||||||||||||||||
| onReady?.(); | ||||||||||||||||||||
| }); | ||||||||||||||||||||
| el.addEventListener("error", (event) => { | ||||||||||||||||||||
| reject(event); | ||||||||||||||||||||
| onError?.(event); | ||||||||||||||||||||
| }); | ||||||||||||||||||||
| }); | ||||||||||||||||||||
| loadPromise.catch(() => undefined).finally(() => loadingScripts.delete(src)); | ||||||||||||||||||||
| loadingScripts.set(src, loadPromise); | ||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The
Clean up on both settle paths:
Suggested change
Alternatively, use
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit:
Suggested change
Purely cosmetic — the runtime behavior is identical since |
||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| if (onLoad) el.addEventListener("load", onLoad); | ||||||||||||||||||||
| if (onError) el.addEventListener("error", onError); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| document.body.appendChild(el); | ||||||||||||||||||||
| if (key) loadedScripts.add(key); | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| /** | ||||||||||||||||||||
| * Load a script imperatively (outside of React). | ||||||||||||||||||||
| */ | ||||||||||||||||||||
| export function handleClientScriptLoad(props: ScriptProps): void { | ||||||||||||||||||||
| loadClientScript(props, { | ||||||||||||||||||||
| resolvedNonce: resolveScriptNonce(props.nonce), | ||||||||||||||||||||
| fireReadyWhenAlreadyLoaded: false, | ||||||||||||||||||||
| }); | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| /** | ||||||||||||||||||||
|
|
@@ -192,48 +262,20 @@ function Script(props: ScriptProps): React.ReactElement | null { | |||||||||||||||||||
| return; | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| const el = document.createElement("script"); | ||||||||||||||||||||
| if (src) el.src = src; | ||||||||||||||||||||
| if (id) el.id = id; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| for (const [attr, value] of Object.entries(rest)) { | ||||||||||||||||||||
| if (attr === "className") { | ||||||||||||||||||||
| el.setAttribute("class", String(value)); | ||||||||||||||||||||
| } else if (typeof value === "string") { | ||||||||||||||||||||
| el.setAttribute(attr, value); | ||||||||||||||||||||
| } else if (typeof value === "boolean" && value) { | ||||||||||||||||||||
| el.setAttribute(attr, ""); | ||||||||||||||||||||
| } | ||||||||||||||||||||
| } | ||||||||||||||||||||
| if (resolvedNonce && !el.getAttribute("nonce")) { | ||||||||||||||||||||
| el.setAttribute("nonce", resolvedNonce); | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| if (strategy === "worker") { | ||||||||||||||||||||
| el.setAttribute("type", "text/partytown"); | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| if (dangerouslySetInnerHTML?.__html) { | ||||||||||||||||||||
| // Intentional: mirrors the Next.js <Script> API where dangerouslySetInnerHTML | ||||||||||||||||||||
| // is developer-supplied inline script content (not user input). The prop name | ||||||||||||||||||||
| // itself signals developer awareness of the XSS risk, consistent with React's | ||||||||||||||||||||
| // design. User-supplied data must never flow into this prop. | ||||||||||||||||||||
| el.innerHTML = dangerouslySetInnerHTML.__html as string; | ||||||||||||||||||||
| } else if (children && typeof children === "string") { | ||||||||||||||||||||
| el.textContent = children; | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| el.addEventListener("load", (e) => { | ||||||||||||||||||||
| if (key) loadedScripts.add(key); | ||||||||||||||||||||
| onLoad?.(e); | ||||||||||||||||||||
| onReady?.(); | ||||||||||||||||||||
| }); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| if (onError) { | ||||||||||||||||||||
| el.addEventListener("error", onError); | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| document.body.appendChild(el); | ||||||||||||||||||||
| loadClientScript( | ||||||||||||||||||||
| { | ||||||||||||||||||||
| src, | ||||||||||||||||||||
| id, | ||||||||||||||||||||
| strategy, | ||||||||||||||||||||
| onLoad, | ||||||||||||||||||||
| onReady, | ||||||||||||||||||||
| onError, | ||||||||||||||||||||
| children, | ||||||||||||||||||||
| dangerouslySetInnerHTML, | ||||||||||||||||||||
| ...rest, | ||||||||||||||||||||
| }, | ||||||||||||||||||||
| { resolvedNonce, fireReadyWhenAlreadyLoaded: true }, | ||||||||||||||||||||
| ); | ||||||||||||||||||||
| }; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| if (strategy === "lazyOnload") { | ||||||||||||||||||||
|
|
||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,19 @@ | ||||||||
| import { test, expect } from "@playwright/test"; | ||||||||
|
|
||||||||
| const BASE = "http://localhost:4173"; | ||||||||
|
|
||||||||
| test.describe("next/script", () => { | ||||||||
| // Ported from Next.js: packages/next/src/client/script.tsx | ||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: the reference comment says "Ported from"
Suggested change
Or reference the Next.js E2E test if there's a specific dedup test there. |
||||||||
| // https://github.com/vercel/next.js/blob/canary/packages/next/src/client/script.tsx | ||||||||
| // Next.js keeps a ScriptCache for in-flight remote scripts so same-src | ||||||||
| // components mounted together only append one DOM script. | ||||||||
| test("deduplicates simultaneous same-src scripts before load completes", async ({ page }) => { | ||||||||
| await page.goto(`${BASE}/script-dedupe`); | ||||||||
| await expect(page.getByRole("heading", { name: "Script Dedupe" })).toBeVisible(); | ||||||||
|
|
||||||||
| await expect.poll(() => page.locator('script[src="/dedupe-script.js"]').count()).toBe(1); | ||||||||
| await expect | ||||||||
| .poll(() => page.evaluate(() => Reflect.get(window, "__vinextScriptDedupeExecutions"))) | ||||||||
| .toBe(1); | ||||||||
| }); | ||||||||
| }); | ||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| import Script from "next/script"; | ||
|
|
||
| export default function ScriptDedupePage() { | ||
| return ( | ||
| <main> | ||
| <h1>Script Dedupe</h1> | ||
| <Script src="/dedupe-script.js" /> | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider also testing the |
||
| <Script src="/dedupe-script.js" /> | ||
| </main> | ||
| ); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| window.__vinextScriptDedupeExecutions = (window.__vinextScriptDedupeExecutions || 0) + 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The in-flight join path calls
onLoadon success andonErroron failure, but never callsonReady. Compare with the first-caller path at line 189 which callsonReady?.()afteronLoad. Next.js firesonReadyfor every caller after load completes — this diverges.See the suggestion on line 146 above which addresses this together with the premature
loadedScripts.addfix.