perf: compact DOM script handling - #7976
Conversation
📝 WalkthroughWalkthroughSolid and Vue router script handling now uses shared attribute application, direct duplicate detection, and explicit element cleanup. New client-side tests verify external and inline script rendering, deduplication, attributes, contents, and teardown behavior. ChangesScript handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
View your CI Pipeline Execution ↗ for commit d8c453f
☁️ Nx Cloud last updated this comment at |
🚀 Changeset Version PreviewNo changeset entries found. Merging this PR will not cause a version bump for any packages. |
Bundle Size Benchmarks
The following scenarios have bundle-size changes compared with the baseline:
Current gzip tracks all emitted client JS chunks. Initial gzip tracks only the entry/import graph. Trend sparkline is historical current gzip ending with this PR measurement; lower is better. |
![nx-cloud[bot]](https://avatars.githubusercontent.com/in/80458?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nx Cloud has identified a flaky task in your failed CI:
🔂 Since the failure was identified as flaky, we triggered a CI rerun by adding an empty commit to this branch.
🎓 Learn more about Self-Healing CI on nx.dev
Merging this PR will degrade performance by 5.6%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Memory | mem server peak-large-page (react) |
953 KB | 2,166.8 KB | -56.02% |
| ❌ | Memory | mem server error-paths redirect (vue) |
358.2 KB | 474.7 KB | -24.54% |
| ❌ | Memory | mem server error-paths unmatched (react) |
263.2 KB | 316.2 KB | -16.75% |
| ❌ | Memory | mem client navigation-churn (solid) |
538.1 KB | 574.3 KB | -6.31% |
| ❌ | Memory | mem server error-paths not-found (react) |
250.7 KB | 260.7 KB | -3.84% |
| ❌ | Memory | mem server server-fn-churn (react) |
265 KB | 275.6 KB | -3.84% |
| ⚡ | Memory | mem server request-churn (react) |
940 KB | 493.6 KB | +90.44% |
| ⚡ | Memory | mem client unique-location-churn (react) |
762.5 KB | 664.9 KB | +14.68% |
| ⚡ | Memory | mem server server-fn-churn (vue) |
274.7 KB | 263.3 KB | +4.3% |
| ⚡ | Simulation | ssr request loop (solid) |
157.7 ms | 153 ms | +3.07% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing codex/dom-script-micro-passes (d8c453f) with main (abf9b81)
Footnotes
-
10 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/solid-router/tests/Scripts.test.tsx (1)
260-266: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the inline
typeattribute.Both tests declare
type: 'module'but only assert inline script content. A regression that omitstypeinsetScriptAttrsstill passes.
packages/solid-router/tests/Scripts.test.tsx#L260-L266: Assert thatgetInlineScript()?.getAttribute('type')is'module'.packages/vue-router/tests/Scripts.test.tsx#L215-L221: Assert thatgetInlineScript()?.getAttribute('type')is'module'.🤖 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/tests/Scripts.test.tsx` around lines 260 - 266, Update the inline script assertions in packages/solid-router/tests/Scripts.test.tsx:260-266 and packages/vue-router/tests/Scripts.test.tsx:215-221 to verify getInlineScript()?.getAttribute('type') is 'module', alongside the existing content and uniqueness checks.
🤖 Prompt for all review comments with 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.
Inline comments:
In `@packages/solid-router/src/Asset.tsx`:
- Around line 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.
- Around line 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.
---
Nitpick comments:
In `@packages/solid-router/tests/Scripts.test.tsx`:
- Around line 260-266: Update the inline script assertions in
packages/solid-router/tests/Scripts.test.tsx:260-266 and
packages/vue-router/tests/Scripts.test.tsx:215-221 to verify
getInlineScript()?.getAttribute('type') is 'module', alongside the existing
content and uniqueness checks.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 18630d2c-bba4-4cd7-9789-c526d1a57af8
📒 Files selected for processing (4)
packages/solid-router/src/Asset.tsxpackages/solid-router/tests/Scripts.test.tsxpackages/vue-router/src/Asset.tsxpackages/vue-router/tests/Scripts.test.tsx
| 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()) |
There was a problem hiding this comment.
📐 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 theif (dataScript)return at Line 94.packages/vue-router/src/Asset.tsx#L95-L127: Use braces around theif (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()) |
There was a problem hiding this comment.
🎯 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.
| 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.
1 participant
Summary
querySelectorAllresults directly instead of allocating arrays and callbacksscript.remove()for Solid cleanupPublic props, rendered attributes, script matching, and lifecycle behavior are unchanged.
Bundle impact
Measured against exact
mainbase697ebb6ddbd433d052b6b4707938a5c595865d58:Validation
Full attribution and the 17-scenario matrix are in
RESULT-optimization-dom-script-passes.md.Summary by CodeRabbit
Bug Fixes
Tests