fix(script): dedupe concurrent same-src loads#1263
Conversation
Multiple next/script components with the same src could append duplicate DOM scripts when they mounted before the first script load event fired. That double-executed third-party code and diverged from Next.js' in-flight script cache behavior. The shim now tracks remote scripts that are currently loading separately from scripts that have completed loading, and fans out load/error callbacks without adding another script element. Adds a Pages Router browser regression that verifies simultaneous same-src scripts produce one DOM script and one execution.
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.
Good refactor — extracting loadClientScript to share between the component and imperative paths is the right call, and the in-flight dedup cache mirrors the Next.js ScriptCache / LoadCache split correctly. The test fixture and Playwright spec are clean.
Three issues to address, one correctness bug and two behavioral gaps:
- Bug: premature
loadedScripts.add(key)on in-flight join — marking the key loaded before the promise settles means a third+ caller arriving before load completes skips attaching callbacks entirely. loadingScriptsnever cleaned up — settled promises (especially rejected ones) accumulate forever, preventing retry after transient failures.onReadymissing for duplicate callers — the in-flight join path firesonLoad/onErrorbut notonReady, diverging from both the first-caller path and Next.js behavior.
| if (src) { | ||
| const existingLoad = loadingScripts.get(src); | ||
| if (existingLoad) { | ||
| if (key) loadedScripts.add(key); |
There was a problem hiding this comment.
Bug: this marks the key as loaded immediately, before the script has actually finished loading. If a third caller arrives while the script is still in-flight, it will hit the loadedScripts.has(key) early return at line 136, skip attaching onLoad/onError callbacks, and (if fireReadyWhenAlreadyLoaded is true) fire onReady prematurely — before the script has executed.
Defer this into the .then() success handler:
| if (key) loadedScripts.add(key); | |
| void existingLoad.then( | |
| (event) => { | |
| if (key) loadedScripts.add(key); | |
| onLoad?.(event); | |
| onReady?.(); | |
| }, | |
| (event) => onError?.(event), | |
| ); |
This also fixes issue #3 below (onReady missing for duplicate callers) — see the onReady?.() call added in the success branch.
| }); | ||
| }); | ||
| loadPromise.catch(() => undefined); | ||
| loadingScripts.set(src, loadPromise); |
There was a problem hiding this comment.
The loadingScripts map is never cleaned up after load or error. Two problems:
- Memory leak — every unique
srcaccumulates a settledPromisefor the lifetime of the page. - No retry after failure — if a script fails to load (network error, 404), the rejected promise stays in the map forever. Any future attempt to load the same
srcwill join the already-rejected promise and immediately fail, even if the resource is now available.
Clean up on both settle paths:
| loadingScripts.set(src, loadPromise); | |
| loadingScripts.set(src, loadPromise); | |
| void loadPromise.then( | |
| () => loadingScripts.delete(src), | |
| () => loadingScripts.delete(src), | |
| ); |
Alternatively, use .finally() if the project's TS target supports it. Note that the no-op .catch() on line 196 is still needed to suppress the unhandled rejection, so keep both.
| const existingLoad = loadingScripts.get(src); | ||
| if (existingLoad) { | ||
| if (key) loadedScripts.add(key); | ||
| void existingLoad.then( |
There was a problem hiding this comment.
The in-flight join path calls onLoad on success and onError on failure, but never calls onReady. Compare with the first-caller path at line 189 which calls onReady?.() after onLoad. Next.js fires onReady for every caller after load completes — this diverges.
See the suggestion on line 146 above which addresses this together with the premature loadedScripts.add fix.
| const BASE = "http://localhost:4173"; | ||
|
|
||
| test.describe("next/script", () => { | ||
| // Ported from Next.js: packages/next/src/client/script.tsx |
There was a problem hiding this comment.
Nit: the reference comment says "Ported from" packages/next/src/client/script.tsx, which is the implementation file, not a test. Consider updating to reference the actual Next.js test file:
| // Ported from Next.js: packages/next/src/client/script.tsx | |
| // Mirrors Next.js ScriptCache behavior in packages/next/src/client/script.tsx | |
| // https://github.com/vercel/next.js/blob/canary/packages/next/src/client/script.tsx |
Or reference the Next.js E2E test if there's a specific dedup test there.
| return ( | ||
| <main> | ||
| <h1>Script Dedupe</h1> | ||
| <Script src="/dedupe-script.js" /> |
There was a problem hiding this comment.
Consider also testing the onLoad / onReady callback dedup behavior — e.g., verify that both <Script> components receive their onLoad callbacks even though only one DOM node is created. That would catch the onReady gap flagged above. Could be a follow-up test.
|
Review submitted. Here's a summary of the findings: Review Summary for PR #1263Overall: Good refactor that correctly identifies the dedup gap and mirrors Next.js's 1. Bug: Premature
|
- Defer loadedScripts.add(key) to after in-flight promise resolves - Add onReady() callback for duplicate callers joining in-flight load - Clean up loadingScripts entry on promise settlement to allow retry
|
/bigbonk review |
There was a problem hiding this comment.
Clean refactor that correctly mirrors the Next.js ScriptCache/LoadCache split. The three issues from the previous review (premature loadedScripts.add, missing loadingScripts cleanup, missing onReady for duplicate callers) are all addressed in the second commit.
The loadClientScript extraction is a nice win — it eliminates the duplicated DOM manipulation logic between the component and imperative paths.
Two small observations (neither blocking):
| }); | ||
| }); | ||
| loadPromise.catch(() => undefined).finally(() => loadingScripts.delete(src)); | ||
| loadingScripts.set(src, loadPromise); |
There was a problem hiding this comment.
Nit: set runs after .catch().finally() is chained, which is correct — but the visual ordering reads like "clean up, then register". Consider swapping lines 199–200 so the map insertion comes first and the cleanup chain second:
| loadingScripts.set(src, loadPromise); | |
| loadingScripts.set(src, loadPromise); | |
| loadPromise.catch(() => undefined).finally(() => loadingScripts.delete(src)); |
Purely cosmetic — the runtime behavior is identical since .finally() always runs as a microtask.
| (event) => { | ||
| if (key) loadedScripts.add(key); | ||
| onLoad?.(event); | ||
| onReady?.(); |
There was a problem hiding this comment.
Worth noting: this is actually more correct than Next.js, which silently drops onReady for duplicate callers who chain onto an in-flight load (their tier-2 path only does .then(onLoad, onError)). Good divergence.
|
Review submitted. Here's the summary: Approved PR #1263 — The script dedup fix is solid. All three issues from the prior review round are resolved:
Left two non-blocking comments:
|
2 participants
Overview
next/scriptcomponents request the same remote script before it finishes loading.src, separate from the completed-load cache.packages/vinext/src/shims/script.tsx,tests/e2e/pages-router/script.spec.tsWhy
next/scriptmust treat a remote script request as owned by itssrc, including the interval between DOM insertion and the browser load event. Without an in-flight cache, simultaneous components can all miss the completed-load cache and each append a<script>, which double-executes third-party code.srcremote script should be inserted once, even while loading.loadingScriptspromises separately fromloadedScripts.onLoadandonError.ScriptCache/LoadCachesplit.What changed
srcscripts mount togethersrccaller arrives while the first is loadingloadedScripts.Maintainer review path
packages/vinext/src/shims/script.tsx: inspect the split between completed loads and in-flight remote loads, plus the sharedloadClientScriptpath used by both component and imperative loading.tests/fixtures/pages-basic/pages/script-dedupe.tsx: minimal reproduction page with two simultaneous same-srcscripts.tests/e2e/pages-router/script.spec.ts: browser assertion for one DOM script and one execution.Validation
vp test run tests/script.test.tsPLAYWRIGHT_PROJECT=pages-router npx playwright test tests/e2e/pages-router/script.spec.tsvp checkThe new Playwright regression failed before the fix with two
script[src="/dedupe-script.js"]nodes, then passed after the in-flight cache was added.Risk / compatibility
next/scriptremote loading now matches Next.js' same-srcde-duplication semantics more closely.srccallers still get load/error callbacks through the existing promise.References
ScriptCacheandLoadCachenext/scriptduplicate and readiness behavior around script loading.next/scriptdocs