fix(script): honor async false for client scripts#1286
Conversation
Client-side next/script insertion left dynamically created script elements in the browser's forced async state when callers passed async={false}. That breaks ordered third-party script loading because the shim skipped false boolean props instead of clearing the DOM script async flag.
Set script boolean attributes through their DOM properties and explicitly set/remove disabled boolean attributes to clear force async. Cover the regression with a focused handleClientScriptLoad test.
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.
Solid fix. The core problem (dynamically-created scripts start in the browser's force-async state, so async={false} was silently ignored) is correctly identified and the fix matches the Next.js approach: set the DOM property, then setAttribute+removeAttribute to clear the force-async flag.
I compared setBooleanScriptAttribute against Next.js's setAttributesFromProps and the behavior is equivalent for the important cases. One minor edge case: the value === "false" (string) handling actually improves on Next.js — vinext correctly sets the DOM property to false, while Next.js sets it to true (via !!"false") then only removes the attribute (property stays true). I'd call that a win.
Test is well-structured — mock starts with async: true to simulate the browser's force-async default, then verifies both the property and the attribute absence after the fix.
Two small observations below, neither blocking.
| break; | ||
| case "noModule": | ||
| case "nomodule": | ||
| el.noModule = enabled; |
There was a problem hiding this comment.
Nit: Matching "nomodule" (lowercase) in addition to "noModule" is more permissive than Next.js, which only recognizes noModule through DOMAttributeNames. This is fine — arguably better — but worth noting in case strict parity matters for some future test.
Separately, when the removal branch fires with attr = "nomodule" (lowercase), the el.setAttribute("nomodule", "") call will set the lowercase attribute name. In real browsers setAttribute is case-insensitive for HTML elements, so this works correctly, but it's a subtle difference from Next.js which always uses the camelCase "noModule" for the attribute name.
| if (attr === "className") { | ||
| el.setAttribute("class", String(value)); | ||
| if (value === undefined) continue; | ||
| if (setBooleanScriptAttribute(el, attr, value)) continue; |
There was a problem hiding this comment.
Pre-existing gap (not introduced by this PR): numeric attribute values like data-count={42} are silently dropped because the value falls through both the typeof value === "string" and typeof value === "boolean" checks. Next.js handles this with el.setAttribute(attr, String(value)) for all non-boolean attrs.
Not a blocker for this PR since it's pre-existing, but worth a follow-up issue if you want full parity.
|
Review submitted. Here's a summary of what I found: Approved. The fix correctly addresses the browser's force-async behavior for dynamically-created script elements. The implementation:
I left two non-blocking observations:
|
2 participants
Overview
next/scriptelements when callers passasync={false}.handleClientScriptLoadand the client-side<Script>insertion path.packages/vinext/src/shims/script.tsx,tests/script.test.tsWhy
Dynamically-created script elements start in the browser's force-async state. A
next/scriptshim must treatasync={false}as an execution-order instruction, not as a false boolean prop to skip.async={false}must clear force async before append.async,defer, andnoModuleare assigned throughHTMLScriptElementproperties.next/scriptbehavior where it is browser-observable.What changed
handleClientScriptLoad({ src, async: false })el.asyncstayed true.asyncattribute is absent.Maintainer review path
packages/vinext/src/shims/script.tsx: reviewsetBooleanScriptAttributeand its call fromsetScriptAttributes.tests/script.test.ts: review the focused client insertion regression that starts from a forced-async script element.Validation
mainwith a mocked DOM script element:handleClientScriptLoad({ async: false })leftasyncastrue.vp test run tests/script.test.tsvp check packages/vinext/src/shims/script.tsx tests/script.test.tsvp run vinext#buildcompleted successfully. The build still reports the existing unresolved virtual or external import warnings for generated entry points.Risk / compatibility
async={true}remains enabled, omittedasyncremains untouched, andasync={false}now does what callers requested.References
setAttributesFromPropsasync,defer, andnoModulethrough script DOM properties and clears disabled attributes to override force async.async={false}require explicit DOM state changes.