fix(shims/head): dedupe Pages Router <Head> tags on re-render#1625
fix(shims/head): dedupe Pages Router <Head> tags on re-render#1625james-elicx wants to merge 3 commits into
Conversation
The client head manager previously purged every `[data-next-head]` element and re-appended a fresh DOM node on every `<Head>` re-render. For `<script src="...">` and `<link rel="stylesheet" href="...">` tags inside `<Head>` this caused the script to re-execute (and the stylesheet to re-fetch) on every parent re-render — a regression against Next.js's head-manager, which diffs old vs new tags via `isEqualNode` and only touches the DOM for the diff. Mirror Next.js's `head-manager.ts:updateElements` reconciliation: bucket desired and existing tags by type, match each desired tag against an equal existing tag (preserving the live DOM node), and only remove or append for the genuine diff. Also handle the browser's nonce-stripping quirk so CSP-tagged scripts compare equal across renders. Refs the upstream e2e in `.nextjs-ref/test/e2e/nonce-head-manager`. Fixes #1473
|
/bigbonk review |
commit: |
|
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Good change — the reconciliation approach is correct and matches the upstream pattern. The core diff-by-type + isEqualNode loop is faithful to Next.js's head-manager.ts:updateElements. A few things to address:
1. Missing meta[charset] handling
Next.js has special handling for meta[charset] in updateElements:
if (type === 'meta') {
const metaCharset = headEl.querySelector('meta[charset]')
if (metaCharset !== null) {
oldTags.add(metaCharset)
}
}This ensures an existing <meta charset="utf-8"> (typically injected by the HTML template without data-next-head) is adopted into the managed set and gets deduped against a <meta charSet="utf-8"> inside <Head>. Without this, a <Head><meta charSet="utf-8" /></Head> will append a duplicate rather than replacing the template one.
Additionally, on the append side, Next.js prepends charset meta to <head> rather than appending:
if (newTag.tagName.toLowerCase() === 'meta' && newTag.getAttribute('charset') !== null) {
headEl.prepend(newTag)
}Both of these are missing from the vinext reconciler. Not a blocker for the dedup fix, but worth a follow-up issue if not addressed here.
2. Missing document.title sync
The upstream updateHead sets document.title directly from the title component before dispatching to updateElements. vinext's reconciler handles <title> as a normal tag through DOM reconciliation, which will work, but there's a subtle difference: Next.js avoids the DOM round-trip by assigning document.title directly. This is a minor parity gap, not a blocker.
3. Nonce test doesn't actually exercise the nonce-stripping code path
The FakeElement class in the test doesn't model the browser's nonce-stripping behavior. Both the old and new FakeElement instances will have matching nonce attributes (since FakeElement.setAttribute always preserves the value), so isEqualHeadNode takes the fast path (oldTag.isEqualNode(newTag) returns true) and the nonce-compensation branch (nonce && !oldTag.getAttribute("nonce")) is never entered.
Additionally, FakeElement is not an instance of HTMLElement, so the instanceof HTMLElement guard in isEqualHeadNode will fail, meaning the nonce-stripping branch is unreachable in these tests regardless.
The test comment at line 711-715 acknowledges this ("We only have attributes in this double, so for the equality test the 'stripped' form just means the attribute string match still works because both sides carry the same nonce attribute"), but that means the nonce-stripping logic in isEqualHeadNode has zero test coverage. Consider either:
- Using
jsdomorhappy-domfor these specific nonce tests to exercise the real code path, or - Adding a direct unit test for
isEqualHeadNodewith mockHTMLElementinstances that simulate the stripped nonce behavior.
4. typeof HTMLElement !== "undefined" guard
Minor: the upstream Next.js version doesn't have this guard — it just does oldTag instanceof HTMLElement && newTag instanceof HTMLElement. Since _reconcileClientHead is only called from syncClientHead (which runs client-side where HTMLElement is always defined), the extra guard is unnecessary. Not wrong, just dead code in practice.
Overall
The core reconciliation logic is sound and faithfully mirrors the upstream approach. The test suite covers the important behavioral cases well (identity preservation, stale removal, unmanaged isolation, multi-step sequence). The nonce test coverage gap is the main thing I'd want fixed — the rest are parity gaps worth tracking but not blocking.
| existingByType.set(tag, bucket); | ||
| } | ||
| bucket.add(el); | ||
| } |
There was a problem hiding this comment.
Next.js has special handling here for meta[charset]. In updateElements, when type === 'meta', it queries headEl.querySelector('meta[charset]') and adds it to oldTags even if it lacks data-next-head. This ensures a <Head><meta charSet="utf-8" /></Head> replaces the template-injected charset meta rather than duplicating it.
On the append side, Next.js also prepends charset metas (headEl.prepend(newTag)) to keep them first in <head>.
Worth a follow-up issue if not addressed here.
| // sides carry the same nonce attribute. | ||
| _reconcileClientHead(doc, [makeScript({ src: "/src-1.js", nonce: "abc123" })]); | ||
| expect(doc.head.children).toHaveLength(1); | ||
| expect(doc.head.children[0]).toBe(firstNode); |
There was a problem hiding this comment.
This test doesn't actually exercise the nonce-stripping branch in isEqualHeadNode. Two issues:
FakeElementis not aninstanceof HTMLElement, so theinstanceofguard inisEqualHeadNodewill always be false — the function falls straight through tooldTag.isEqualNode(newTag).- Even if it did enter the branch,
FakeElement.getAttribute('nonce')returns the nonce on both old and new tags (no browser stripping simulation), so thenonce && !oldTag.getAttribute('nonce')condition would be false.
The test passes because FakeElement.isEqualNode does a structural comparison that happens to work — but the nonce-compensation code has zero coverage.
Consider adding a direct unit test for isEqualHeadNode with objects that simulate the stripped-nonce behavior (e.g., oldTag.getAttribute('nonce') returns null while oldTag.nonce returns the value).
|
Review posted successfully on PR #1625. Here's a summary of the findings: Core logic: Sound. The reconciliation approach (bucket by tag type, diff with Key findings:
|
…e tests Bot-review follow-ups for #1625: - Reconciler now adopts a template-injected <meta charset> into the managed meta bucket when <Head> declares its own charSet, and prepends new charset metas so they remain first in <head>. Mirrors Next.js's updateElements special case. Scoped to the "<Head> declares charset" case so unrelated reconciliations leave the template charset alone. - isEqualHeadNode now has direct unit coverage with HTMLElement-instance doubles, exercising the nonce-stripping branch the reconciler-level tests never reached (FakeElement isn't instanceof HTMLElement, so the branch was previously dead from a coverage perspective). - Clarified why the typeof HTMLElement guard stays: unit tests drive the reconciler in the Node env where HTMLElement is undefined.
|
/bigbonk review |
There was a problem hiding this comment.
Previous review findings addressed. The second commit adds meta[charset] adoption and direct isEqualHeadNode unit tests with a StubHTMLElement that simulates browser nonce-stripping — both items flagged in the first round.
Reconciliation logic: Correct
The diff-by-type + isEqualNode reconciliation in _reconcileClientHead faithfully mirrors updateElements from upstream head-manager.ts. The algorithm is sound:
- Bucket desired and existing managed tags by tag type
- For each desired tag,
createElement+_applyHeadPropsToElement+ match against existing withisEqualHeadNode - Remove unmatched stale tags, append unmatched new tags
The per-type bucketing prevents cross-type false matches (a <meta> can never accidentally match a <link>), matching the upstream approach.
meta[charset] handling: Improvement over upstream
vinext only adopts a template <meta charset> when <Head> itself declares a charset meta. Upstream adopts unconditionally, which means rendering <Head><meta name="description" .../></Head> with no charset meta would still remove the template <meta charset> — arguably a bug. The conditional adoption here is a deliberate, documented improvement. Good.
The prepend vs appendChild for charset metas also fixes an upstream bug: Next.js at line 90-98 does headEl.prepend(newTag) followed by unconditional headEl.appendChild(newTag) (missing else), which moves the charset meta to the end. vinext correctly uses if/else.
Nonce handling: Correct, well-tested
isEqualHeadNode matches the upstream isEqualNode exactly (clone, strip nonce attribute, preserve .nonce property, compare). The typeof HTMLElement !== "undefined" guard is defensive for the Node test environment — not wrong, and the comment documents why it's there.
The new StubHTMLElement-based tests in the isEqualHeadNode describe block directly exercise the nonce-stripping branch with stripNonceAttribute = true, covering the gap from the first review round. The captured-clone assertion (checking getAttribute("nonce") === "" and .nonce === "abc123" on the clone) is a nice touch that verifies the clone's shape, not just the boolean result.
Test coverage: Thorough
The test suite covers:
- Identity preservation (same DOM node instance across re-renders)
- Stale tag removal
- Unmanaged tag isolation
- Full upstream e2e sequence (render → re-render → change src → change back)
- meta[charset] adoption and prepend ordering
- All branches of
isEqualHeadNode(nonce stripped, nonce mismatch, no nonce, nonce present, non-HTMLElement)
One minor note (non-blocking)
The document.title sync gap mentioned in the first review is still present — Next.js assigns document.title directly rather than going through DOM reconciliation for <title>. This is a minor parity gap (the title will still update correctly through DOM reconciliation, just via a different mechanism). Worth tracking but not blocking.
LGTM — ship it.
|
Review posted and approved on PR #1625. Here's the summary: Approved. The second commit addresses both items from the first review round (meta[charset] adoption and direct Key findings:
|
|
CI was failing on formatting only — ran |
1 participant
Summary
The Pages Router
<Head>client manager previously purged every[data-next-head]element and re-appended a fresh DOM node on every re-render. For<script src="...">(and<link>) tags inside<Head>, this caused the browser to re-execute the script on each parent re-render — even when nothing in the head projection had actually changed.Mirror Next.js's
head-manager.ts:updateElementsreconciliation:isEqualNode) and keep the live DOM node when matched.Also handle the browser's nonce-stripping quirk (Chrome/Firefox strip
noncefrom the serialised attribute once an element is in the document) so CSP-tagged scripts still compare equal across renders — matches the upstream behaviour.Test plan
tests/head.test.tscover:srcproduce a fresh node.data-next-head) left untouched.pnpm test tests/head.test.ts tests/script.test.ts tests/shims.test.ts— 1068 tests pass.pnpm run check— format, lint, type check clean.References
next/headmanager re-executes inline scripts on re-render #1473.nextjs-ref/test/e2e/nonce-head-manager/index.test.ts.nextjs-ref/packages/next/src/client/head-manager.ts