fix(shims/head): match Next.js charset/viewport order and merge _document.getInitialProps head#1677
fix(shims/head): match Next.js charset/viewport order and merge _document.getInitialProps head#1677james-elicx wants to merge 2 commits into
Conversation
|
/bigbonk review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: fix(shims/head): match Next.js charset/viewport order and merge _document.getInitialProps head
Clean PR. The architecture is sound: seeding defaults via defaultHead() and routing _document.getInitialProps through the same dedupe pipeline is exactly the right approach. A few items worth addressing below.
_document.getInitialProps always fires for the default Document class
The guard at dev-server.ts:139 and pages-page-response.ts:345 checks typeof DocAny.getInitialProps === "function", but the default Document class in shims/document.tsx:112 always defines a static getInitialProps. This means even apps without a custom _document.tsx will call it every request, getting back { html: "" } and setting documentInitialHead to [].
This is functionally benign (empty array, no work in the dedupe pipeline), but it's unnecessary overhead: an await + try/catch + Array.isArray check on every render for apps that don't customize their document. Consider adding a check like DocAny.getInitialProps !== Document.getInitialProps (comparing against the base class's own method) to skip the call for the default implementation, or annotating the intent in a comment so future readers understand it's intentional.
Client-side _applyHeadPropsToElement doesn't use jsxAttrToHtml
The SSR path now correctly maps JSX attribute names to HTML (charSet → charset, etc.) via jsxAttrToHtml at line 324. But the client-side _applyHeadPropsToElement at line 397-408 still passes the raw JSX key to domEl.setAttribute(). For charSet, setAttribute("charSet", "utf-8") will produce <meta charset="utf-8"> in most browsers because setAttribute is case-insensitive for HTML elements — so this works by accident. But for httpEquiv, setAttribute("httpEquiv", "...") produces the attribute httpequiv (lowercase), not http-equiv (hyphenated). This would be a hydration mismatch if the SSR output has http-equiv but the client sets httpequiv. Not introduced by this PR (pre-existing), but worth noting since you've now fixed the SSR side.
Duplicated getInitialProps call logic
The getInitialProps invocation block is duplicated nearly identically between dev-server.ts:138-154 and pages-page-response.ts:343-360. Both construct the same minimal DocumentContext, do the same try/catch, extract the same initialHead array. This is the kind of runtime behavior that AGENTS.md says should live in a helper under server/* rather than being inlined in two places. A small helper like callDocumentGetInitialProps(DocCtor, setDocumentInitialHead) would eliminate the duplication and make it testable in isolation.
Minor: defaultHead() allocates on every call
defaultHead() creates two fresh React.createElement calls on every invocation of getSSRHeadHTML(). Since these are always the same elements, consider hoisting them to module-level constants. Negligible perf impact in practice, but it's a free improvement.
Test quality
The new tests are thorough and well-structured. Good coverage of:
- Default ordering (charset → viewport → user)
data-next-headattribute presence- charSet meta-type dedupe override
- Key-based viewport override
setDocumentInitialHeadmerge and reset isolation
The existing test adjustments (expect(html).toBe("") → expect(html).not.toContain(...)) are correct — defaults are now always present so empty-string checks no longer apply.
Overall this is solid work that brings vinext closer to Next.js parity on a user-visible issue. The architecture of flowing defaults and _document initial props through the same dedupe pipeline is the right design.
|
Review posted on PR #1677. Here's a summary of the key findings: Architecture: Sound design — seeding Items raised:
|
|
/bigbonk review |
There was a problem hiding this comment.
Re-review: fix(shims/head): match Next.js charset/viewport order and merge _document.getInitialProps head
All three items from the first review have been addressed cleanly in the follow-up commit:
Previously raised — now resolved
-
getInitialPropsalways fires — Fixed. The newcallDocumentGetInitialPropshelper captures the baseDocument.getInitialPropsreference at module scope and short-circuits when the resolved method is the same reference. Apps without a custom_documentskip the async call entirely. The indexed-access pattern (Document["getInitialProps"]) to avoid oxlint'sunbound-methodrule is a nice touch. -
Client-side
_applyHeadPropsToElementhydration mismatch — Fixed. Both the boolean and string attribute branches now go throughjsxAttrToHtml(), sohttpEquivcorrectly produceshttp-equivon the DOM side matching SSR output. New tests athead.test.ts:542-563verify bothhttpEquivandcharSetmapping. -
Duplicated
getInitialPropscall logic — Fixed. Extracted toserver/document-initial-head.ts. Bothdev-server.tsandpages-page-response.tsnow call the shared helper with identical signatures. The module-level doc comment explains the contract clearly.
Remaining observation (non-blocking)
No unit tests for callDocumentGetInitialProps itself. The helper is exercised indirectly through integration tests (pages-router.test.ts, pages-page-response.test.ts), and the logic is straightforward enough that this isn't blocking. But the doc comment says "lets the (mostly-static) implementation be tested in isolation" — a focused test that verifies the skip-for-default-method behavior and the error-logging path would complete that promise. Could be a follow-up.
defaultHead() still allocates on every call — the minor suggestion to hoist the constant elements to module scope wasn't addressed. Not a real problem; React.createElement is cheap and this runs once per request. Mentioning for completeness.
Ship it.
|
Review posted on PR #1677 — approved. Here's a summary: All three items from the first review were addressed in the follow-up commit:
Two non-blocking observations noted: no direct unit tests for the new helper (exercised indirectly), and |
…ment.getInitialProps head (#1569) Pages Router head emission did not match Next.js's canonical order (`<meta charset>` then `<meta viewport>` then user tags, all with `data-next-head=""`) and ignored head tags returned from a user `_document.getInitialProps()`. - `next/head` shim now seeds `defaultHead()` (charset + viewport) into the SSR collector before user tags, mirroring `defaultHead()` in `next/src/shared/lib/head.tsx`. They flow through the same dedupe pipeline so users can still override via `key="charset"` / `key="viewport"`. - Serialised attribute names follow HTML conventions (`charSet` -> `charset`, `httpEquiv` -> `http-equiv`, etc.), matching Next.js's `test/e2e/next-head/index.test.ts` exact-string assertion. - `next/document` shim no longer hardcodes charset/viewport in `<Head>` (those defaults now flow from `next/head`); fallback HTML shells in `dev-server.ts` and `pages-page-response.ts` likewise drop the duplicates. - New `setDocumentInitialHead()` accessor and ALS-backed state lets the SSR pipeline forward head tags returned by user `_document.getInitialProps()` into the same dedupe pipeline as `next/head` children. Both the dev server and the prod `renderPagesPageResponse` invoke `_document.getInitialProps()` when present and pass the result through. Adds focused regression coverage in `tests/head.test.ts` for ordering, default attribute serialisation, key-based overrides, and the `_document.getInitialProps()` merge path. Refs #1569.
- Extract shared `callDocumentGetInitialProps` helper under `server/document-initial-head.ts` so both `dev-server.ts` and `pages-page-response.ts` go through the same code path. The previous duplicated try/catch blocks drifted independently and were the kind of runtime-behavior that AGENTS.md says should live in a helper. - Skip the call when `DocumentComponent.getInitialProps` is still the unmodified default inherited from vinext's `next/document` shim. Comparing against the captured base reference (`DEFAULT_GET_INITIAL_PROPS`) distinguishes a user override from the default — extending `Document` without overriding the method inherits the same static function, so invoking it on every render is wasted work. - Use `jsxAttrToHtml` on the client-side `_applyHeadPropsToElement` path so `httpEquiv` is set as `http-equiv` rather than `httpequiv` (lowercased by `setAttribute`). Matches the SSR output and prevents a hydration mismatch when SSR emits the hyphenated form. - Update stale `tests/document.test.ts` Head assertions: the shim's `<Head>` no longer hardcodes charset/viewport — those flow through `next/head`'s `defaultHead()` so the dedupe pipeline handles them.
james-elicx
force-pushed
the
fix/issue-1569-head-charset-viewport-order
branch
from
20679f8 to
6d40262
Compare
|
Rebased on |
commit: |
|
1 participant
Summary
Fixes #1569.
next/headshim now seedsdefaultHead()(charset + viewport) into the SSR head collector before user tags, mirroring Next.js'sdefaultHead()inpackages/next/src/shared/lib/head.tsx. They flow through the same dedupe pipeline so users can still override viakey="charset"/key="viewport". Output now matches the exact-string assertion in Next.js'stest/e2e/next-head/index.test.ts:Serialised attribute names follow HTML conventions (
charSet->charset,httpEquiv->http-equiv,crossOrigin->crossorigin, etc.). Previously these were emitted verbatim from the JSX prop name.next/documentshim no longer hardcodes charset / viewport in its<Head>— the defaults now flow throughnext/head's collector withdata-next-head="". Fallback HTML shells indev-server.tsandpages-page-response.tslikewise drop the duplicates.New
setDocumentInitialHead()accessor (plus ALS-backed state inhead-state.ts) lets the SSR pipeline forward head tags returned by user_document.getInitialProps()into the same dedupe pipeline asnext/headchildren. Both the dev server and the prodrenderPagesPageResponsenow invoke_document.getInitialProps()when present and pass the result through.Test plan
pnpm test tests/head.test.tspnpm test tests/pages-page-response.test.tspnpm test tests/pages-router.test.tspnpm test tests/shims.test.tspnpm test tests/app-page-head.test.ts tests/script-head-ordering.test.tspnpm test tests/nextjs-compat/metadata.test.tspnpm test tests/static-export.test.ts -t "charset"vp check(lint, format, types)