Skip to content

test(unhead): resolve reactive root entry input#777

Merged
harlan-zw merged 2 commits into
mainfrom
perf/single-root-resolve
Jun 12, 2026
Merged

test(unhead): resolve reactive root entry input#777
harlan-zw merged 2 commits into
mainfrom
perf/single-root-resolve

Conversation

@harlan-zw

@harlan-zw harlan-zw commented Jun 12, 2026

Copy link
Copy Markdown
Collaborator

🔗 Linked issue

Follow-up to #776 / the CodeRabbit "avoid resolving the root input twice" thread.

❓ Type of change

  • 📖 Documentation
  • 🐞 Bug fix
  • 👌 Enhancement
  • ✨ New feature
  • 🧹 Chore
  • ⚠️ Breaking change

📚 Description

This PR originally removed the explicit root resolve in normalizeEntryToTags since walkResolver also resolves the root, making it look like a redundant double pass. Digging into history showed it is intentional: walkResolver unwraps function values before applying resolvers, so a resolvable that yields a function, e.g. useHead(ref(() => ({...}))), only renders if a resolve pass runs first. The pre-call dates to a255df0 (fix(vue): broken reactivity, the vite-pwa/vite-plugin-pwa#832 input class), and the full test suite passed without it because only the plain-ref root shape had coverage.

The optimization is reverted here. What lands instead:

  • a comment on the pre-call explaining why the root intentionally passes through the resolver chain twice
  • root-shapes.test.ts covering both root shapes (ref({...}) and ref(() => ({...}))); the second fails against the "optimized" version

Net behavior change vs main: none.

Summary by CodeRabbit

  • Tests

    • Added SSR unit tests to validate that root-resolved head entries (including function-wrapped patterns) render expected head tags, improving runtime reliability.
  • Chore

    • Added clarifying internal comments to the normalization logic to improve maintainability; no runtime behavior changes.

walkResolver already applies the resolver chain at the root, so the explicit
pre-call meant every entry input hit each propResolver twice at the top
level (inherited from v3). Resolvers must now tolerate a single root pass;
all framework adapter resolvers are unwrap-style and unaffected.
@coderabbitai

coderabbitai Bot commented Jun 12, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

Adds clarifying comments in normalizeEntryToTags documenting walkResolver’s function unwrapping and root pass-through behavior, and adds Vitest SSR tests verifying useHead with root ref shapes (value and function-returning) produce expected head tags.

Changes

Clarify resolver behavior & SSR tests

Layer / File(s) Summary
Document walkResolver root pass behavior
packages/unhead/src/utils/normalize.ts
Add comments in normalizeEntryToTags describing that walkResolver unwraps functions before resolving prop resolvers and how the root intentionally passes through the resolver chain.
SSR root-resolvable shapes tests
packages/vue/test/unit/ssr/root-shapes.test.ts
Add Vitest tests asserting SSR head tags for useHead(ref(...)) and useHead(ref(() => ...)), with a @ts-expect-error for the function-root runtime case.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Possibly related PRs

  • unjs/unhead#776: Modifies resolver-walking behavior in normalizeEntryToTags and touches related root-pass handling.

Poem

A rabbit reads comments in the night,
Unwraps functions with gentle light,
Tests hum softly, SSR sings true,
Root shapes checked—behavior in view. 🐇✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description check ✅ Passed The PR description is comprehensive and complete, covering the linked issue, type of change, and detailed rationale for why the double-root resolve is load-bearing with test coverage.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The PR title 'test(unhead): resolve reactive root entry input' focuses on testing aspects, but the primary change involves performance optimization and fixing resolver behavior through comments and tests.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch perf/single-root-resolve

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions

github-actions Bot commented Jun 12, 2026

Copy link
Copy Markdown
Contributor

Bundle Size Analysis

Bundle Size Gzipped
Client (Minimal) 11.1 kB 4.5 kB
Server (Minimal) 10.7 kB 4.4 kB
Vue Client (Minimal) 12.1 kB 5 kB
Vue Server (Minimal) 11.6 kB 4.8 kB

…function inputs

Reverts the single-root-resolve optimization. walkResolver unwraps function
values before applying resolvers, so a resolvable yielding a function (e.g.
useHead(ref(() => ({...}))), the vite-pwa/vite-plugin-pwa#832 class of input)
only renders when an explicit resolve pass runs first; the root passing
through the resolver chain twice is intentional. Adds a comment encoding the
reason and regression tests for both root shapes so the pre-call doesn't get
optimized away again.
@harlan-zw harlan-zw changed the title perf(unhead): resolve root entry input through propResolvers once test(vue): pin double root resolve as load-bearing Jun 12, 2026
@harlan-zw harlan-zw changed the title test(vue): pin double root resolve as load-bearing test(unhead): resolve reactive root entry input Jun 12, 2026
@harlan-zw harlan-zw merged commit f0f057b into main Jun 12, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant