perf(unhead): skip defensive tag clones when no mutating hooks registered#776
Merged
Conversation
resolveTags cloned every tag and its props on each render to protect cached entry tags from hook mutation. Only registered tags:*/ssr:render(ed)/ dom:rendered hooks can mutate resolved tags, so the clone now only happens when one is present, and the server payload plugin hook is marked non-mutating since it only appends. sanitizeTags copies script tags before escaping instead of relying on the render clone. normalizeEntryToTags previously deep-cloned the whole input via walkResolver before normalizeProps walked it again. Prop resolution now happens inline during normalizeProps in a single traversal with identical resolver call semantics. Also: hook calls skip context allocation when no listeners are registered, tagToString caches close-tag regexes, capo weights inlined with redundant async re-checks dropped, escapeHtml switch replaced with a lookup map, client createHead reuses TagPriorityAliases, double-deprecated renderSSRHead wrapper collapsed, and the bogus optionalPlugins package.json field removed.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughRefactors client/server tag-weighting, tag resolution, normalization, and HTML rendering utilities; introduces DEFAULT_TAG_WEIGHT and conditional cloning for mutating hooks; adds tests; removes package metadata and re-exports server render helpers. ChangesTag Resolution and Rendering Refactor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Contributor
Bundle Size Analysis
|
![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
🤖 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/unhead/src/utils/normalize.ts`:
- Around line 55-60: The for...in loop in normalize.ts iterates inherited
enumerable properties; before reading input[prop] inside the loop that uses
isUnsafeKey and walkResolver, add an ownership guard using Object.hasOwn(input,
prop) (i.e. skip if !Object.hasOwn(input, prop)); do the same fix for the other
for...in loop referenced (around the block at lines 131-143) so inherited keys
are never promoted into tag.props; keep existing isUnsafeKey and walkResolver
logic but only run them when the property is an own property.
In `@packages/unhead/src/utils/resolve.ts`:
- Around line 145-148: The needsClone branch currently shallow-copies tag.props
but leaves container props like class (Set) and style (Map) shared; update the
cloning logic in the block that computes ctx.tags (the code using
TAG_MUTATING_HOOKS, hooks, ctx.tags and entries) to deep-clone or recreate
tag.props.class and tag.props.style when they exist (e.g., new
Set(tag.props.class) and new Map(tag.props.style)) so mutating hooks cannot
mutate cached e._tags; ensure you still copy other props shallowly as before and
only perform the extra cloning when needsClone is true.
🪄 Autofix (Beta)
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
Run ID: ef041128-f324-4867-a269-a7a8635c5f59
📒 Files selected for processing (11)
packages/unhead/package.jsonpackages/unhead/src/client/createHead.tspackages/unhead/src/server/createHead.tspackages/unhead/src/server/index.tspackages/unhead/src/server/sort.tspackages/unhead/src/server/util/propsToString.tspackages/unhead/src/server/util/tagToString.tspackages/unhead/src/utils/dedupe.tspackages/unhead/src/utils/normalize.tspackages/unhead/src/utils/resolve.tspackages/unhead/test/unit/server/util.test.ts
💤 Files with no reviewable changes (1)
- packages/unhead/package.json
Gzip impact matters more to consumers than the last few percent of SSR throughput. Revert normalizeEntryToTags to the walkResolver pass (keeping a skip of the resolver closure when no propResolvers are registered) and drop the per-prop resolution from normalizeProps. Replace the spelled-out tag-mutating hook list with a regex test over registered hook names. Against bench/bundle baselines the server entry is now 186 bytes smaller than main (+14 gz) and the client entry costs +217 min / +91 gz, down from +642 / +250 at the first commit. The clone-skip keeps the bulk of the perf: +22% transformed useSeoMeta x50 SSR and +17% core renderSSRHead vs main, giving back the fused-normalization gains on plugin-heavy pages.
harlan-zw
changed the title
The defensive clone copied props one level deep, so a tags:* hook calling tag.props.class.add() or tag.props.style.set() mutated the Set/Map shared with the cached entry tags and leaked into later renders (pre-existing on main, surfaced by CodeRabbit on this diff). Copy the two containers in the clone path and add a regression test that fails against the shallow copy.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/unhead/src/utils/normalize.ts`:
- Around line 102-110: The code pre-applies propResolvers by calling
resolve(undefined, input) before calling walkResolver, but walkResolver already
calls resolve(undefined, val) for the root, causing double invocation; remove
the pre-run line so propResolvers are only applied via walkResolver (i.e.,
delete or comment out the input = resolve(undefined, input) call) and keep the
resolve function and input = walkResolver(input, resolve) as-is to ensure root
is resolved exactly once.
🪄 Autofix (Beta)
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
Run ID: 8952fd58-f442-48cf-b811-fb00df7b2f6c
📒 Files selected for processing (2)
packages/unhead/src/utils/normalize.tspackages/unhead/src/utils/resolve.ts
props.class/style are typed as string but are Set/Map at runtime after normalization; widen the clone's props record and route the test casts through unknown.
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🔗 Linked issue
n/a
❓ Type of change
📚 Description
resolveTagscloned every tag plus its props on every render to protect cached entry tags from hook mutation. A CPU profile of the SSR e2e bench showed the resolve pipeline at ~56% of self time while actual HTML rendering took ~17%, with the defensive clone and its GC churn as the biggest avoidable cost.Changes:
tags:*,ssr:render(ed),dom:rendered, matched by name). The serverunhead:payloadhook only appends a fresh tag, so it is marked_nonMutatingand no longer forces the clone on every SSR render.sanitizeTagsis copy-on-write: script tags are copied before escaping instead of relying on the render clone.normalizeEntryToTagsskips building the resolver closure when nopropResolversare registered, which speeds up core (non-framework) usage without touching the walk semantics.tagToStringcaches close-tag regexes instead of allocating one per call, capo script weights inlined with redundantasyncre-checks dropped,escapeHtmlswitch replaced with a lookup map, clientcreateHeadreusesTagPriorityAliases, the double-deprecatedrenderSSRHeadwrapper inunhead/servercollapsed to a re-export, and the invalidoptionalPluginsfield removed from package.json.propsToStringskips enumerable inherited props (theObject.hasOwnserialization guard now has coverage), and hook mutations of class/style containers no longer leak into the entry cache across renders.📊 Benchmarks
vitest bench(bench/*), main vs this branch:useSeoMetax50 SSR (vite transformed)useSeoMetax50 SSR (runtime)Standalone core (no plugins, 20k iterations):
renderSSRHead68.5k -> ~80k ops/s (+17%),transformHtmlTemplate46.4k -> ~57k ops/s (+22%). GC pauses over 200k simulated requests dropped ~11%.Pages using plugins that register tag-mutating hooks (templateParams, inferSeoMeta) keep the clone and see smaller gains; plain
useHead/useSeoMetasetups get the full win.📦 Bundle size
bench/bundlevslast.json:The first iteration of this PR also fused prop resolution into a single normalization pass, worth another ~5% core / +27pp on the transformed
useSeoMetabench, but it cost +90 gz on the client entry so it was dropped. Easy to revisit if the trade reads differently.Note:
pnpm typecheckfails locally on main (unused@ts-expect-errorinpackages/schema-org/src/svelte.ts:14), unrelated to this diff; CI typecheck passes.Summary by CodeRabbit
Bug Fixes
Refactor
Chores
Tests