Skip to content

perf: compact DOM script handling #7976

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.

Sign up for GitHub

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

Open
Sheraff wants to merge 9 commits into
base: main
Choose a base branch
from
Open

perf: compact DOM script handling #7976

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
6e12093
perf: share script attribute copying
Sheraff Aug 6, 2026
b16f2eb
perf: scan script elements without temporary arrays
Sheraff Aug 6, 2026
d674d4c
perf(solid-router): use idempotent script removal
Sheraff Aug 6, 2026
b8ed93f
test: cover compact DOM script handling
Sheraff Aug 6, 2026
e5d61ae
test: cover compact DOM script handling [Self-Healing CI Rerun]
nx-cloud[bot] Aug 6, 2026
6015c73
Delete RESULT-optimization-dom-script-passes.md
Sheraff Aug 6, 2026
d8c453f
Merge branch 'main' into codex/dom-script-micro-passes
Sheraff Aug 6, 2026
b6742f2
Merge branch 'main' into codex/dom-script-micro-passes
Sheraff Aug 7, 2026
da136c7
changeset
Sheraff Aug 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 30 additions & 46 deletions packages/solid-router/src/Asset.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,20 @@ interface ScriptAttrs {
src?: string
}

function setScriptAttrs(
script: HTMLScriptElement,
attrs: ScriptAttrs | undefined,
) {
if (!attrs) {
return
}
for (const [key, value] of Object.entries(attrs)) {
if (value !== undefined && value !== false) {
script.setAttribute(key, typeof value === 'boolean' ? '' : String(value))
}
}
}

function Script({
attrs,
children,
Expand All @@ -88,77 +102,47 @@ function Script({
return attrs.src
}
})()
const existingScript = Array.from(
document.querySelectorAll('script[src]'),
).find((el) => (el as HTMLScriptElement).src === normSrc)

if (existingScript) {
return
for (const el of document.querySelectorAll('script[src]')) {
if ((el as HTMLScriptElement).src === normSrc) {
return
}
}

const script = document.createElement('script')

for (const [key, value] of Object.entries(attrs)) {
if (value !== undefined && value !== false) {
script.setAttribute(
key,
typeof value === 'boolean' ? '' : String(value),
)
}
}
setScriptAttrs(script, attrs)

document.head.appendChild(script)

onCleanup(() => {
if (script.parentNode) {
script.parentNode.removeChild(script)
}
})
onCleanup(() => script.remove())
Comment on lines +105 to +116

@coderabbitai coderabbitai Bot Aug 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Use braces for the early returns.

Both changed lifecycle functions contain an unbraced if (dataScript) return.

  • packages/solid-router/src/Asset.tsx#L105-L116: Use braces around the if (dataScript) return at Line 94.
  • packages/vue-router/src/Asset.tsx#L95-L127: Use braces around the if (dataScript) return at Line 81.

As per coding guidelines, TSX control statements must use curly braces. Based on learnings, make this local change in the Solid adapter without broad normalization.

📍 Affects 2 files
  • packages/solid-router/src/Asset.tsx#L105-L116 (this comment)
  • packages/vue-router/src/Asset.tsx#L95-L127
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/solid-router/src/Asset.tsx` around lines 105 - 116, Wrap the early
return in the dataScript condition with braces in
packages/solid-router/src/Asset.tsx at lines 105-116 and
packages/vue-router/src/Asset.tsx at lines 95-127, updating the lifecycle
functions containing if (dataScript). Make only this local formatting change and
avoid broader control-statement normalization.

Sources: Coding guidelines, Learnings

}

if (typeof children === 'string') {
const typeAttr =
typeof attrs?.type === 'string' ? attrs.type : 'text/javascript'
const nonceAttr =
typeof attrs?.nonce === 'string' ? attrs.nonce : undefined
const existingScript = Array.from(
document.querySelectorAll('script:not([src])'),
).find((el) => {
if (!(el instanceof HTMLScriptElement)) return false
for (const el of document.querySelectorAll('script:not([src])')) {
if (!(el instanceof HTMLScriptElement)) {
continue
}
const sType = el.getAttribute('type') ?? 'text/javascript'
const sNonce = el.getAttribute('nonce') ?? undefined
return (
if (
el.textContent === children &&
sType === typeAttr &&
sNonce === nonceAttr
)
})

if (existingScript) {
return
) {
return
}
}

const script = document.createElement('script')
script.textContent = children

if (attrs) {
for (const [key, value] of Object.entries(attrs)) {
if (value !== undefined && value !== false) {
script.setAttribute(
key,
typeof value === 'boolean' ? '' : String(value),
)
}
}
}
setScriptAttrs(script, attrs)

document.head.appendChild(script)

onCleanup(() => {
if (script.parentNode) {
script.parentNode.removeChild(script)
}
})
onCleanup(() => script.remove())
Comment on lines 119 to +145

@coderabbitai coderabbitai Bot Aug 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Use else if when attrs.src is defined.

When one asset has both src and string children, this separate if appends an inline script after the external script. The SSR branch at Lines 158-160 renders only the external script. The Vue implementation also uses else if. This creates client and server divergence.

Proposed fix 
-    if (typeof children === 'string') {
+    else if (typeof children === 'string') {
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if (typeof children === 'string') {
const typeAttr =
typeof attrs?.type === 'string' ? attrs.type : 'text/javascript'
const nonceAttr =
typeof attrs?.nonce === 'string' ? attrs.nonce : undefined
const existingScript = Array.from(
document.querySelectorAll('script:not([src])'),
).find((el) => {
if (!(el instanceof HTMLScriptElement)) return false
for (const el of document.querySelectorAll('script:not([src])')) {
if (!(el instanceof HTMLScriptElement)) {
continue
}
const sType = el.getAttribute('type') ?? 'text/javascript'
const sNonce = el.getAttribute('nonce') ?? undefined
return (
if (
el.textContent === children &&
sType === typeAttr &&
sNonce === nonceAttr
)
})
if (existingScript) {
return
) {
return
}
}
const script = document.createElement('script')
script.textContent = children
if (attrs) {
for (const [key, value] of Object.entries(attrs)) {
if (value !== undefined && value !== false) {
script.setAttribute(
key,
typeof value === 'boolean' ? '' : String(value),
)
}
}
}
setScriptAttrs(script, attrs)
document.head.appendChild(script)
onCleanup(() => {
if (script.parentNode) {
script.parentNode.removeChild(script)
}
})
onCleanup(() => script.remove())
else if (typeof children === 'string') {
const typeAttr =
typeof attrs?.type === 'string' ? attrs.type : 'text/javascript'
const nonceAttr =
typeof attrs?.nonce === 'string' ? attrs.nonce : undefined
for (const el of document.querySelectorAll('script:not([src])')) {
if (!(el instanceof HTMLScriptElement)) {
continue
}
const sType = el.getAttribute('type') ?? 'text/javascript'
const sNonce = el.getAttribute('nonce') ?? undefined
if (
el.textContent === children &&
sType === typeAttr &&
sNonce === nonceAttr
) {
return
}
}
const script = document.createElement('script')
script.textContent = children
setScriptAttrs(script, attrs)
document.head.appendChild(script)
onCleanup(() => script.remove())
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/solid-router/src/Asset.tsx` around lines 119 - 145, Change the
string-children handling in Asset so it is an else-if branch guarded by an
undefined attrs.src, preventing inline script creation when an external source
is present. Preserve the existing inline deduplication and cleanup behavior for
assets without src, matching the SSR external-script behavior.

}
})

Expand Down
71 changes: 71 additions & 0 deletions packages/solid-router/tests/Scripts.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,77 @@ describe('ssr scripts', () => {
)
})

test('injects client script attributes and removes the script on cleanup', async () => {
const externalScript = {
src: 'solid-client-script.js',
async: true,
defer: false,
crossOrigin: 'anonymous' as const,
}
const inlineScript = {
id: 'solid-client-inline-script',
type: 'module',
children: 'window.__solidClientScript = true',
}
const rootRoute = createRootRoute({
scripts: () => [
externalScript,
externalScript,
inlineScript,
inlineScript,
],
component: () => (
<>
<div data-testid="solid-client-script-root" />
<Scripts />
</>
),
})
const indexRoute = createRoute({
path: '/',
getParentRoute: () => rootRoute,
})
const router = createRouter({
history: createMemoryHistory({ initialEntries: ['/'] }),
routeTree: rootRoute.addChildren([indexRoute]),
isServer: false,
})

await router.load()
const result = render(() => <RouterProvider router={router} />)
expect(
await screen.findByTestId('solid-client-script-root'),
).toBeInTheDocument()

const getScript = () =>
document.head.querySelector<HTMLScriptElement>(
'script[src="solid-client-script.js"]',
)
await waitFor(() => expect(getScript()).not.toBeNull())
expect(getScript()?.hasAttribute('async')).toBe(true)
expect(getScript()?.hasAttribute('defer')).toBe(false)
expect(getScript()?.getAttribute('crossorigin')).toBe('anonymous')
expect(
document.head.querySelectorAll('script[src="solid-client-script.js"]'),
).toHaveLength(1)
const getInlineScript = () =>
document.head.querySelector<HTMLScriptElement>(
'script#solid-client-inline-script',
)
await waitFor(() => expect(getInlineScript()).not.toBeNull())
expect(getInlineScript()?.textContent).toBe(
'window.__solidClientScript = true',
)
expect(
document.head.querySelectorAll('script#solid-client-inline-script'),
).toHaveLength(1)

getScript()?.remove()
expect(() => result.unmount()).not.toThrow()
expect(getScript()).toBeNull()
expect(getInlineScript()).toBeNull()
})

test('keeps manifest stylesheet links mounted across repeated Link navigations', async () => {
const history = createTestBrowserHistory()

Expand Down
64 changes: 28 additions & 36 deletions packages/vue-router/src/Asset.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,20 @@ interface ScriptAttrs {
src?: string
}

function setScriptAttrs(
script: HTMLScriptElement,
attrs: ScriptAttrs | undefined,
) {
if (!attrs) {
return
}
for (const [key, value] of Object.entries(attrs)) {
if (value !== undefined && value !== false) {
script.setAttribute(key, typeof value === 'boolean' ? '' : String(value))
}
}
}

const Title = Vue.defineComponent({
name: 'Title',
props: {
Expand Down Expand Up @@ -78,61 +92,39 @@ const Script = Vue.defineComponent({
return attrs.src
}
})()
const existingScript = Array.from(
document.querySelectorAll('script[src]'),
).find((el) => (el as HTMLScriptElement).src === normSrc)

if (existingScript) {
return
for (const el of document.querySelectorAll('script[src]')) {
if ((el as HTMLScriptElement).src === normSrc) {
return
}
}

const script = document.createElement('script')

for (const [key, value] of Object.entries(attrs)) {
if (value !== undefined && value !== false) {
script.setAttribute(
key,
typeof value === 'boolean' ? '' : String(value),
)
}
}
setScriptAttrs(script, attrs)

document.head.appendChild(script)
} else if (typeof children === 'string') {
const typeAttr =
typeof attrs?.type === 'string' ? attrs.type : 'text/javascript'
const nonceAttr =
typeof attrs?.nonce === 'string' ? attrs.nonce : undefined
const existingScript = Array.from(
document.querySelectorAll('script:not([src])'),
).find((el) => {
if (!(el instanceof HTMLScriptElement)) return false
for (const el of document.querySelectorAll('script:not([src])')) {
if (!(el instanceof HTMLScriptElement)) {
continue
}
const sType = el.getAttribute('type') ?? 'text/javascript'
const sNonce = el.getAttribute('nonce') ?? undefined
return (
if (
el.textContent === children &&
sType === typeAttr &&
sNonce === nonceAttr
)
})

if (existingScript) {
return
) {
return
}
}

const script = document.createElement('script')
script.textContent = children

if (attrs) {
for (const [key, value] of Object.entries(attrs)) {
if (value !== undefined && value !== false) {
script.setAttribute(
key,
typeof value === 'boolean' ? '' : String(value),
)
}
}
}
setScriptAttrs(script, attrs)

document.head.appendChild(script)
}
Expand Down
74 changes: 74 additions & 0 deletions packages/vue-router/tests/Scripts.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,11 @@ afterEach(() => {
cleanup()
browserHistories.splice(0).forEach((history) => history.destroy())
window.history.replaceState(null, 'root', '/')
document.head
.querySelectorAll(
'script[src="vue-client-script.js"], script#vue-client-inline-script',
)
.forEach((script) => script.remove())
delete window.$_TSR
})

Expand Down Expand Up @@ -149,6 +154,75 @@ describe('ssr scripts', () => {
expect(scripts[0]!.getAttribute('src')).toBe('script.js')
expect(scripts[1]!.getAttribute('src')).toBe('script3.js')
})

test('injects client script attributes into the document head', async () => {
const externalScript = {
src: 'vue-client-script.js',
async: true,
defer: false,
crossOrigin: 'anonymous' as const,
}
const inlineScriptOptions = {
id: 'vue-client-inline-script',
type: 'module',
children: 'window.__vueClientScript = true',
}
const rootRoute = createRootRoute({
scripts: () => [
externalScript,
externalScript,
inlineScriptOptions,
inlineScriptOptions,
],
component: () => (
<>
<div data-testid="vue-client-script-root" />
<Scripts />
</>
),
})
const indexRoute = createRoute({
path: '/',
getParentRoute: () => rootRoute,
})
const router = createRouter({
history: createMemoryHistory({ initialEntries: ['/'] }),
routeTree: rootRoute.addChildren([indexRoute]),
isServer: false,
})

await router.load()
render(<RouterProvider router={router} />)
expect(
await screen.findByTestId('vue-client-script-root'),
).toBeInTheDocument()

const getScript = () =>
document.head.querySelector<HTMLScriptElement>(
'script[src="vue-client-script.js"]',
)
await waitFor(() => expect(getScript()).not.toBeNull())
expect(getScript()?.hasAttribute('async')).toBe(true)
expect(getScript()?.hasAttribute('defer')).toBe(false)
expect(getScript()?.getAttribute('crossorigin')).toBe('anonymous')
expect(
document.head.querySelectorAll('script[src="vue-client-script.js"]'),
).toHaveLength(1)
const getInlineScript = () =>
document.head.querySelector<HTMLScriptElement>(
'script#vue-client-inline-script',
)
await waitFor(() => expect(getInlineScript()).not.toBeNull())
expect(getInlineScript()?.textContent).toBe(
'window.__vueClientScript = true',
)
expect(
document.head.querySelectorAll('script#vue-client-inline-script'),
).toHaveLength(1)

getScript()?.remove()
getInlineScript()?.remove()
})
})

describe('ssr HeadContent', () => {
Expand Down
Loading