fix: preserve data script content during client hydration#6653
Conversation
📝 WalkthroughWalkthroughAdds detection and client render path for non-JS "data" scripts (e.g., application/ld+json) across React, Solid, and Vue router packages and introduces React tests. Inline data-script children are rendered into script.innerHTML on the client instead of leaving empty placeholders or duplicating elements. Changes
Sequence Diagram(s)sequenceDiagram
participant SSR as Server (SSR)
participant Browser as Browser (initial HTML)
participant Client as Client Runtime
participant Head as document.head
SSR->>Browser: Serve HTML with <script type="application/ld+json">content</script>
Browser->>Client: Load bundle and hydrate
Client->>Client: Script component evaluates attrs & children
alt dataScript && children is string
Client->>Head: Render/ensure <script> with innerHTML (no dynamic injection)
else src or JS/module script
Client->>Head: Effect injects or re-inserts executable/module <script>
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (beta)
No actionable comments were generated in the recent review. 🎉 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 |
1874b8c to
1472165
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@packages/react-router/src/Asset.tsx`:
- Around line 52-55: The dataScript classification wrongly treats an
empty-string type as a non-JS data script; update the logic around the
dataScript constant in Asset.tsx (the attrs?.type checks) to treat empty string
as 'text/javascript' by normalizing the type first (e.g., let type =
attrs?.type?.trim(); if type === '' set to undefined or 'text/javascript') or by
guarding with a truthy check (e.g., only compare when attrs.type is truthy),
then use that normalized value in the existing comparisons (attrs.type !==
'text/javascript' and !== 'module') so scripts with type="" are treated as
executable JS.
In `@packages/solid-router/src/Asset.tsx`:
- Around line 42-45: The condition computing const dataScript should guard
against an empty-string type like the other implementations; update the
conditional that checks attrs?.type (in the const dataScript declaration) to
also require attrs.type !== '' so dataScript is false for an empty string, while
preserving the existing checks for attrs.type !== 'text/javascript' and
attrs.type !== 'module'.
In `@packages/vue-router/src/Asset.tsx`:
- Around line 57-60: The dataScript boolean currently treats an empty string
type as a data script; update the condition that computes dataScript in
Asset.tsx to also ensure props.attrs?.type !== '' (i.e., add a check against the
empty string alongside the existing checks for 'text/javascript' and 'module')
so that a blank type value is not misclassified; locate the expression that
assigns dataScript and adjust it to include the props.attrs?.type !== '' guard.
🧹 Nitpick comments (1)
packages/vue-router/src/Asset.tsx (1)
57-60:dataScriptis not reactive — won't update ifprops.attrs.typechanges after mount.Since
dataScriptis a plainconstcomputed once insetup(), it won't react to prop changes. This is likely fine since script types don't change dynamically, but if reactivity is ever needed, wrap it inVue.computed().
1472165 to
22cda23
Compare
The Script component renders all <script> tags with empty content on the client side, then uses useEffect to recreate them via DOM manipulation. While necessary for executable scripts (they need document.createElement to trigger execution), this causes data-only scripts like application/ld+json to briefly flash empty during hydration. Per the HTML spec, any script with a type that isn't a JavaScript MIME type or "module" is a "data block" that the browser never executes. These can safely render their content directly via innerHTML on both server and client, just like <style> tags already do in the same file. Changes across react-router, solid-router, and vue-router: - Detect non-executable script types via the type attribute - Early-return in the client-side effect for data scripts - Render data script content directly instead of emptying it
22cda23 to
58d8547
Compare
|
View your CI Pipeline Execution ↗ for commit b7f68af
☁️ Nx Cloud last updated this comment at |
Fixes #6062
Fixes #6627
Closes #6569
Problem
The
Scriptcomponent inAsset.tsxrenders all<script>tags with empty content on the client, then recreates them viauseEffect/ DOM manipulation. This is necesary for executable scripts (they needdocument.createElementto actually run), but it causes data-only scripts likeapplication/ld+jsonto flash empty during hydration — and in some cases the content never comes back properly.Fix
Per the HTML spec, any
<script>with atypethats nottext/javascriptormoduleis a "data block" — the browser will never execute it. So we can safely render those with their content directly (like<style>tags already do in the same file), skipping the useEffect recreation entirely.Changes across react-router, solid-router and vue-router:
typeattributeTest cases added
application/ld+jsonrenders content on SSRapplication/ld+jsonpreserves content on client hydrationapplication/jsonpreserves content on client hydrationtext/javascript(executable) still renders empty on client (unchanged behavior)modulescripts still render empty on client (unchanged behavior)Summary by CodeRabbit
New Features
Tests