test(compat): defer next/head serialization whitespace - #2889
test(compat): defer next/head serialization whitespace#2889james-elicx wants to merge 4 commits into
Conversation
|
/bigbonk review for issues |
commit: |
|
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: test(compat): defer next/head serialization whitespace
Reviewed the diff, verified the count math, and checked the parity claim against existing coverage. This is a clean, low-risk classification change and I found no blocking issues.
Verified
- Count math is internally consistent. The suite moves from
SUPPORTED_SUITE_FEATURES(36→35) intoSUITE_SUPPORT_POLICY(33→34), so:deferred25→26 ✓NON_SUPPORTED_SUITES33→34 ✓CLASSIFIED_SUITESstays 69 (34 + 35) and unique count stays 69 ✓ (it only changed category, not membership)- I confirmed all 69 keys across both maps are distinct.
- Parity claim holds. The reason ("charset order is already correct") is directly backed by
tests/head.test.ts:606("emits charset first, then viewport, before user tags") and the client-side ordering tests (tests/head.test.ts:884-885). The remaining e2e failure being formatting whitespace between siblings — not a semantic ordering bug — is consistent with that coverage. vp checkpasses on both changed files (format, lint, types).vp test run tests/compatibility-support.test.ts— 5 passed.- The new dedicated test (
defers Next.js-specific next/head serialization whitespace) exactly mirrors the policy entry, so the classification is pinned against silent drift.
Non-blocking observations
-
Reason wording could be sharper. The reason says "Next.js internal HTML serialization whitespace", but the PR body describes the delta as vinext retaining formatting whitespace between siblings while the Next.js test asserts a contiguous raw substring. Since this string is surfaced on the user-facing compatibility page (
page.tsx/contribution-grid.tsx), consider phrasing it so it's clear the whitespace is a rendering-output formatting difference (semantic order matches), rather than implying it's purely Next.js-internal. Minor. -
deferredvsneeds-vite-equivalent. The otherdeferredentries all track unimplemented features (Cache Components, PPR, segment cache). This one is a cosmetic serialization/whitespace gap where the behavior is already correct.deferredis defensible (excludes it from the supported pass rate, which is the goal), but it's a slightly different flavor from its siblings. Not worth changing unless you want the taxonomy to stay strict.
Neither observation blocks merge. The change is well-scoped and correctly tested.
|
Review posted to PR #2889. SummaryReviewed PR #2889 ( Verified:
Non-blocking notes I flagged:
Neither observation blocks merge. |
Performance benchmarksCompared 0 improved · 0 regressed · 6 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
|
Independent policy review at exact head Verified independently:
|
…ack-next-head-2890 # Conflicts: # apps/web/app/compatibility/suite-support.ts # tests/compatibility-support.test.ts
|
/bigbonk review for issues |
Review: test(compat): defer next/head serialization whitespaceIndependent re-review at head Verified against the actual PR commit (
|
|
Review posted to PR #2889. SummaryReviewed PR #2889 ( Key verification: I checked the actual PR commit (
Verified:
Non-blocking notes: (1) the reason string reads "Next.js internal HTML serialization whitespace" but the real delta is vinext retaining whitespace — worth sharpening since it's user-facing; (2) |
|
/bigbonk review for issues |
Review: test(compat): defer next/head serialization whitespaceIndependent re-review at head Verified against the actual PR commit (
|
|
Review posted to PR #2889. SummaryReviewed PR #2889 ( Key methodology: I verified the PR's own commit ( Verified:
Non-blocking notes:
I also clarified in the review that the working-tree assertions (33/2) differ from the commit's (34/3) purely because of the sibling stack, and both states are self-consistent with passing tests. |
…o codex/fix-next-head-charset-31439707085
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: test(compat): defer next/head serialization whitespace
Independent re-review of the current head. This iteration is the two follow-up doc commits (6f73531 clarify next/head deferral, 0306645 clarify Worker condition coverage) layered on the stack. No blocking issues.
Verified
- The two prior non-blocking notes are now resolved. The earlier reviews flagged the
next-headreason as inverted ("Next.js internal HTML serialization whitespace"). It now reads "Vinext preserves formatting whitespace between correctly ordered head tags, while the exact Next.js assertion requires contiguous serialized tags." — which correctly attributes the delta to vinext retaining whitespace, not a Next.js-internal issue. No stale copies of the old string remain anywhere in the tree. - Count math is internally consistent in the working tree.
SUITE_SUPPORT_POLICYhas 8 non-supported statuses:deferred26,needs-vite-equivalent2,unsupported5 →NON_SUPPORTED_SUITES33,CLASSIFIED_SUITES69, all unique. Thecompatibility suite support policytest asserts exactly these numbers and passes. (The 34/3 numbers from the isolated PR commit8bb6d2fdiffer only because the siblingimport-conditions/react-versionstack movedreact-versioninto supported — both states are self-consistent.) - Parity claim holds.
tests/head.test.ts:606-622asserts charset → viewport → user-tag ordering directly via index comparison (charsetIdx >= 0,viewportIdx > charsetIdx,userIdx > viewportIdx). The residual e2e failure is genuinely inter-sibling formatting whitespace, not a semantic ordering bug. - Taxonomy has no numeric effect on the headline rate.
bucketSupportedPassRateaccumulates only whensupportStatus === "supported", so all three non-supported statuses are excluded equally. Movingnext-headout of supported correctly drops its failing e2e from the supported-pass-rate denominator. - New/updated tests pin against drift. The dedicated
next-headtest mirrors the policy entry exactly, and the Worker-condition test now asserts the "2 of 12 passing" and "equivalent Worker test passes across middleware" phrasing so the clarified reason can't silently regress.
Quality gates
vp check apps/web/app/compatibility/suite-support.ts tests/compatibility-support.test.ts— format, lint, types all clean.vitest run tests/compatibility-support.test.ts tests/head.test.ts— 59 passed (6 + 53).
Non-blocking observations
- Taxonomy flavor (unchanged from prior reviews). The other
deferredentries track unimplemented features (Cache Components, PPR, segment cache); this one is a cosmetic whitespace gap where behavior is already semantically correct.deferredremains defensible since the numeric outcome is identical, but it's a different flavor from its siblings. Not worth changing.
The change is well-scoped, correctly counted, properly tested, and the earlier wording concern is fixed. LGTM.
|
Review posted to PR #2889. SummaryReviewed PR #2889 ( Key findings:
Quality gates verified locally:
The only remaining note is the (defensible, previously-raised) taxonomy-flavor observation, which doesn't block merge. |
1 participant
Summary
test/e2e/next-head/index.test.tsas deferredThe original failure is in run 31439707085, job 93624401572. A targeted rerun reproduced 1 failure and 4 passes: the browser DOM contains charset, viewport, and user tags in the correct order, but vinext retains formatting whitespace between siblings while the test requires a contiguous raw substring.
tests/head.test.tsmachine-checks the semantic charset → viewport → user-tag order directly.Validation
vp test run tests/compatibility-support.test.ts tests/head.test.ts: 59 passedvp check: cleangit diff --check: clean