-
Notifications
You must be signed in to change notification settings - Fork 325
fix(script): honor async false for client scripts #1286
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 |
|---|---|---|
|
|
@@ -99,11 +99,41 @@ function buildBeforeInteractiveScriptProps(options: { | |
| return scriptProps; | ||
| } | ||
|
|
||
| function setBooleanScriptAttribute(el: HTMLScriptElement, attr: string, value: unknown): boolean { | ||
| const enabled = value !== false && value !== "false" && Boolean(value); | ||
|
|
||
| switch (attr) { | ||
| case "async": | ||
| el.async = enabled; | ||
| break; | ||
| case "defer": | ||
| el.defer = enabled; | ||
| break; | ||
| case "noModule": | ||
| case "nomodule": | ||
| el.noModule = enabled; | ||
| break; | ||
| default: | ||
| return false; | ||
| } | ||
|
|
||
| if (!enabled) { | ||
| // Dynamic script elements start in the browser's force-async state. | ||
| // Setting and removing the attribute mirrors Next.js and clears that state. | ||
| el.setAttribute(attr, ""); | ||
| el.removeAttribute(attr); | ||
| } | ||
|
|
||
| return true; | ||
| } | ||
|
|
||
| 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)); | ||
| if (value === undefined) continue; | ||
| if (setBooleanScriptAttribute(el, attr, value)) continue; | ||
|
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. Pre-existing gap (not introduced by this PR): numeric attribute values like Not a blocker for this PR since it's pre-existing, but worth a follow-up issue if you want full parity. |
||
| if (attr === "className" && typeof value === "string") { | ||
| el.setAttribute("class", value); | ||
| } else if (typeof value === "string") { | ||
| el.setAttribute(attr, value); | ||
| } else if (typeof value === "boolean" && value) { | ||
|
|
||
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.
Nit: Matching
"nomodule"(lowercase) in addition to"noModule"is more permissive than Next.js, which only recognizesnoModulethroughDOMAttributeNames. 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), theel.setAttribute("nomodule", "")call will set the lowercase attribute name. In real browserssetAttributeis 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.