refactor(core): settle the four remaining preview-vs-render divergences - #3097
Conversation
…le four divergences Rendering a composition and mounting one now derive root discovery, scope identity, asset sources and order, hoisted links, variable carriers and nested-host enumeration from the same module. Each keeps its own I/O, which is where they genuinely differ. The compiler's local depth cap and root lookup and the runtime's three pre-filtered head parameters are gone; the runtime hands over the head node and lets the module decide what comes out of it. Four divergences are settled here. Each changes behaviour, so each is stated with what actually differs rather than with a description of the edit. Inline head scripts. The compiler looped head scripts with a src branch and no else, so an inline one was silently discarded on render while the runtime ran it. The compiler was losing code, not holding a convention. Head and content scripts now run through one loop, head first, order preserved. A non-templated sub-composition with an inline head script went from zero collected scripts to one, wrapped, with its body intact. Link hoisting. Conditional on render, unconditional on mount, so a templated sub-composition's webfont link was dropped in video and kept in preview. Hoisting is the superset and matches what the author declared, so hoisting wins. A templated composition with a stylesheet link in its head went from no external links to that link. The parity fixture that previously recorded this shape as a known exclusion now gates it instead. Anonymous hosts. With a host naming no id, the compiler fell back to the first declared composition and scoped to it; the mount left the content unflattened and injected its stylesheet into the host head unscoped, so a composition's CSS leaked into whatever mounted it. The compiler's answer wins. The mount now flattens like every other mount and restores the declared id afterwards. The injected rule went from a bare class selector to one scoped to the composition. This flips an assertion that documented the old behaviour as intentional. Its premise no longer holds. What that test actually cared about, the root and its content being present under the host, still holds and is still asserted; the claim that nothing was flattened is now false and the test asserts the scoping instead. Scope ids. The compiler splits the CSS scope id from the script composition id; they differ only when a host names an id the content does not declare, and there the scripts follow the declared id so their self-referencing queries resolve. The runtime used one for both. The split wins: a host naming captions-comp over content declaring captions now emits scripts bound to captions while its CSS still scopes to captions-comp. Left alone deliberately: the variable-carrier divergence and its TODO, recursion on the mount path, and the three copies each of the flattened-root helper and the id assignment. The first two are behaviour changes with their own units. The third looks mergeable and is not cheaply, and it touches the instancing contract the pixel harness guards. Verified: core 1694 passing, producer 573 passing, the parity contract now nine tests with fixtures gating the two divergences it can observe. Lint clean, typecheck:runtime and the runtime preview guards clean, package cycles unchanged. A trap worth recording: the parity test's compiler arms import core's built dist while the mount arm imports source, so core must be rebuilt before that lane means anything after a compiler change. Skipping it produces a phantom divergence.
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
APPROVE @ 4cb164793d8d0737d7699915af49f63d5bae0828 — R1
Both assembly paths now derive root discovery, scope identity, asset sources + order, hoisted links, and nested-host enumeration from the compositionAssembly module introduced in #3094 — the compiler's local depth cap and root lookup and the runtime's three pre-filtered head parameters (headStyles/headScripts/headLinks) are gone. Each of the four called-out divergences is verified against the diff:
D1 — Inline <head> scripts. The compiler's head loop had a src branch and no else, so an inline head script was silently dropped on render while the runtime executed it. Both paths now iterate plan.scriptSources, which is head-first-then-content by construction (compositionAssembly.ts lines building scriptSources: [...toArray(assetHead?.querySelectorAll(SCRIPT_SELECTOR)), ...toArray(contentNode.querySelectorAll(SCRIPT_SELECTOR))]). Gated by packages/core/src/compiler/inlineSubCompositions.test.ts:6-29 (collects an inline <head> script instead of discarding it — asserts both result.scripts contains the inline body AND scriptItems records an inline kind) plus the sibling runtime test packages/core/src/runtime/compositionLoader.test.ts:339-362 (executes an inline <head> script of a non-templated composition). Not carried by the parity contract because it has no script-body field; the two unit sites are the gate. CORRECT.
D2 — <link> hoisting. Previously conditional on !contentRoot in the compiler, unconditional on the mount — so a templated composition's webfont link was dropped on render while preview kept it. compositionAssembly.ts computes linkSources: toArray(head?.querySelectorAll(HOISTED_LINK_SELECTOR)) with no hasTemplate gate; the compiler's for (const link of plan.linkSources) (inlineSubCompositions.ts:180+) and the mount's identical loop (compositionLoader.ts:483+) now hoist by the same rule. Gated by inlineSubCompositions.test.ts:31-59 plus the new parity fixture in htmlCompiler.parity.test.ts:196-211 (a TEMPLATED composition hoisting a head stylesheet link), and the old exclusion comment in the parity file is now deleted. CORRECT.
D3 — Anonymous hosts. Compiler fell back to the first declared composition and scoped to it (with a data-composition-id restore after flatten); the mount left the content unflattened and injected its stylesheet into the host <head> unscoped, so a composition's .label { … } rule leaked onto the host document. The mount now runs the same prepareFlattenedInnerRoot(innerRoot) path and copies the compiler's restore verbatim in packages/core/src/runtime/compositionLoader.ts:510-517 (the if (!params.authoredCompositionId && authoredScopeCompositionId) { flattenedRoot.setAttribute("data-composition-id", authoredScopeCompositionId); } block, with an in-file comment Mirrors the identical restore in inlineSubCompositions). Gated by the flipped compositionLoader.test.ts:1219+ test — the pre-PR test explicitly documented "not flattened" as intentional, and this PR flips the premise with both a positive assertion (mountedRoot?.getAttribute("data-hf-inner-root") = "true") and an absence assertion (expect(injectedCss).not.toMatch(/^\s*\.label\s*\{/) — no unscoped rule) — plus a parity fixture an anonymous host scoping to the id its content declares. Semantic, not presence-only. CORRECT.
D4 — Scope id split. The runtime collapsed CSS scope and script scope into a single authoredScopeCompositionId; the compiler had always kept them apart (scopeCompId = compId || inferredCompId, scriptCompositionId = inferredCompId || scopeCompId). The shared plan encodes the split as two fields with opposite fallback polarity — authoredCompositionId: compositionId || declaredCompositionId (host wins) and scriptCompositionId: declaredCompositionId || compositionId (declared wins) — and the mount now threads plan.scriptCompositionId into toPendingScript (compositionLoader.ts:498). Gated by compositionLoader.test.ts:364-390 — asserts __hfCompId = "captions" AND not.toContain('__hfCompId = "captions-comp"') when host names captions-comp and content declares captions. CORRECT.
Adversarial pass:
-
Shared-primitive move, not per-surface patch. All four divergences trace to a single shared primitive (composition assembly decisions), and the fix removes the per-side branch rather than papering both.
MAX_SUB_COMPOSITION_DEPTH, thequeryByAttrroot lookup and the runtime'sheadStyles/headScripts/headLinksparams are deleted, not duplicated — the shape signature you'd want from a real convergence. Passes the opposite-direction-workarounds lens (D2 and D3 pushed opposite directions; both are dissolved together). -
Authoring-variant coverage. The plan is invoked across the 2×2 grid (templated/full-doc × named/anonymous host).
assetHead = hasTemplate ? null : (head ?? null)correctly keeps head styles/scripts out of a templated composition while still hoisting its links. Fixture coverage confirms:a full-document composition hoisting a head stylesheet link(pre-existing),a TEMPLATED composition hoisting a head stylesheet link(new — D2),an anonymous host scoping to the id its content declares(new — D3). Runtime unit tests cover the two script-body cases (D1, D4). -
runtimeScopeCompositionIdfallback dropped, deliberately. The mount's pre-fixparams.runtimeCompositionId || authoredScopeCompositionId || nullis nowparams.runtimeCompositionId || null. Comment justifies: "an anonymous host has no runtime id, and the compiler emits no runtime scope selector and no variable table for one." That matches what the compiler is emitting, and the anonymous-host parity fixture would catch drift if the two disagreed on this. Fine. -
Documented non-coverage is honest. The
TODO(template-var-carriers)and mount-recursion cases are called out in the PR body as intentionally untouched, and the parity contract file's comment now enumerates exactly what remains ungated there (the two script-body cases D1/D4, gated by unit tests) rather than the pre-fix generic exclusion. Nothing is silently deferred. -
variableDefaultCarriers— compiler-only consumer. The shared plan returnsvariableDefaultCarriers, but only the compiler reads it — the mount still routes variable defaults throughparams.subCompositionVariablespopulated elsewhere. That's the same asymmetry Miguel's "not covered" section flags asTODO(template-var-carriers). Field is dead code at the mount call site today; harmless, but worth folding into that same follow-up. -
Tests assert semantics not presence. D3 asserts scoped-CSS presence AND unscoped-CSS absence; D4 asserts declared-id presence AND host-id absence. A mutation that leaked the rule or collapsed the ids would flip these.
CI: 37/56 passing, 12 pending, 7 skipping, 0 failing. Format, Lint, all four Perf lanes (parity/drift/fps/load), CodeQL/Analyze all green. Pending are the required Test/CLI-smoke/Windows/regression-shard lanes — worth watching before merge, but nothing in the diff would move a red-lane green.
— Via
james-russo-rames-d-jusso
left a comment
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Reviewed at 4cb164793.
This is the wire I flagged as missing on #3094: compositionAssembly.ts had zero non-test consumers there and the "one owner" claim was aspirational; here both compiler/inlineSubCompositions.ts:24 and runtime/compositionLoader.ts:1 import planCompositionAssembly, and every one of the shared module's decisions (innerRoot, authoredCompositionId, scriptCompositionId, authoredRootId, styleSources, scriptSources, linkSources, variableDefaultCarriers) is read on both sides. The invariant is real now. That's the shape I wanted #3094 to converge on and the follow-up delivering on the promise is the right thing to see.
Traced each of the four divergences the PR body claims against actual code:
- D1 — inline
<head>scripts. Pre-PR the compiler head loop atinlineSubCompositions.tshad asrcbranch and noelse, so an inline head script fell through and was silently discarded; the mount path always executed it. Post-PR head and content scripts shareplan.scriptSources(compiler:294iterates it and takes thewrapInlineScriptWithErrorBoundarybranch for!externalSrc; mount:490-491builds aPendingScriptwithkind:"inline"). Locked at both suites — compiler test:289, runtime test:349. - D2 —
<link>hoisting. Pre-PR compiler gated hoisting on!contentRoot(link loop insideif (!contentRoot && compDoc.head)), so a templated composition's webfont was dropped from the render and kept in preview. Post-PR compiler runsplan.linkSourcesunconditionally at:279-289. Locked by compiler test:314+ new parity fixture "a TEMPLATED composition hoisting a head stylesheet link" athtmlCompiler.parity.test.ts:198, and the previous carve-out comment (last PR's:226-231) is now closed and its wording updated at:263-269to enumerate what remains unpinned by this contract vs what's gated elsewhere. - D3 — anonymous hosts. Pre-PR runtime's
innerRootlookup was gated onparams.authoredCompositionIdbeing truthy — for an anonymous host it stayed null,contentNode = params.sourceNode, and the whole template mounted with its CSS injected unscoped so a rule like.label { color: rgb(1,2,3) }landed on the host document at large. Post-PRplan.innerRootfalls back to the first declared root (compositionAssembly.ts:166-167), the mount takes the innerRoot branch, and:513-518restoresdata-composition-idon the flattened wrapper — mirroring the identical restore the compiler has always done atinlineSubCompositions.ts:398-405. Locked by the rewritten runtime test at:1244(the "preserves data-composition-id unflattened for a host with no id of its own" assertion is flipped, deliberately, per the PR body) + parity fixture "an anonymous host scoping to the id its content declares" athtmlCompiler.parity.test.ts:213. - D4 — scope ids. Pre-PR the runtime collapsed
authoredScopeCompositionId = innerRoot?.getAttribute("data-composition-id") || params.authoredCompositionId || nulland used the same value for bothscopeCssToCompositionand the script wrap, so a host namingcaptions-compover content declaringcaptionsbound scripts to an id nowhere in the DOM. Post-PR readsplan.authoredCompositionId(host wins for CSS) andplan.scriptCompositionId(content wins for scripts) —:424,:429— and the two ids are only distinct when the host names a synthetic one, exactly the case the split is for. Locked at runtime test:378.
What lands cleanly beyond the four:
- The "one owner" claim is now a real dependency on both sides. The module itself is unchanged (still zero imports, still structural-typed via
AssemblyAttributed/AssemblyQueryable<TElement>per:47-56, still gated by the source-scanning test at:257-268), so both consumers picking it up is the visible progress. - Head-then-content ordering is enforced by the module, not by the callers.
plan.styleSourcesandplan.scriptSourcesare returned in the order both paths need (head first, then content) — the runtime's old[...(params.headScripts ?? []), ...Array.from(params.sourceNode.querySelectorAll("script"))]concatenation is gone, replaced by a single iteration. Symmetric on the compiler side. - Nested-host enumeration reasons match pre-PR strings byte-for-byte.
NestedHostSkipReason = "circular composition reference" | "nesting depth exceeded"(compositionAssembly.ts:203) is what the pre-PR compiler emitted viaonMissingComposition— no behaviour change for callers reading those messages. - Variable-default merge order is preserved.
plan.variableDefaultCarriers = [documentElement, innerRoot].filter(...)(:193), and the compiler's newObject.assignloop replicates the old spread order (documentElement first, innerRoot second, hostVariables last) — later wins, same as pre-PR spread semantics. - The "one assertion changed, deliberately" call-out in the PR body is real.
compositionLoader.test.ts:1219(pre-PR) documented anonymous-host non-flattening as intentional with a citation to a #1886 audit trail — that premise is now false, and the rewritten test at:1244asserts the flattening AND the newly-correct scoping. Reader can find the old promise in git history if they need context on why it was believed intentional. That's the right way to flip a load-bearing assertion: replace the test and its justification, not silently edit it. - Testing-hygiene note in the PR body — "the parity test's compiler arms import core's built dist while the mount arm imports source" — is exactly the sort of "one lane is stale" trap that produces phantom divergences. Recording it in the PR body (and presumably in a run-instructions comment somewhere) is the right kind of defensive documentation for a gate that will be re-run every time either path is edited.
One subtle behaviour alignment worth naming (concern, not a blocker): runtimeScopeCompositionId's fallback to authoredScopeCompositionId is REMOVED (compositionLoader.ts:432 — pre-PR was params.runtimeCompositionId || authoredScopeCompositionId || null, post-PR is params.runtimeCompositionId || null). The PR body justifies this cleanly ("an anonymous host has no runtime id, and the compiler emits no runtime scope selector and no variable table for one"), and it IS the right alignment with the compiler — but for an anonymous host with authored variable defaults, pre-PR the runtime would stash them under the declared id (stashInstanceVariables at :534-535), and post-PR it won't. That's a functional change beyond the four D1-D4 divergences the PR body enumerates, and while it removes a runtime-vs-compiler divergence in the correct direction (runtime was doing work the compiler wasn't), a caller relying on runtime-only variable exposure loses it. Worth landing on the PR body's list under "Not covered" or similar, so the next reader trying to trace variable behaviour finds the note here rather than through git log. Not a blocker — the compiler behaviour is authoritative for a shipped composition.
One nit: NestedCompositionHost.ancestry on the return type (compositionAssembly.ts:214-219) is designed as "pass it back in when this host is itself assembled" (per docstring), but the compiler caller doesn't use it — it recomputes nestedAncestry = [...ancestry, src] before pushing to the queue and passes THAT, not nestedHost.ancestry. So the field is computed but never consumed by the only caller today. Either drop the field, or refactor the caller to use it. Purely a tidiness note.
Curiosity, not a review point: the "Not covered" section says the mount path's variable-carriers TODO(template-var-carriers) (compositionLoader.ts:696-703) and the mount's non-recursion are one-line-ish changes now that the module reports what they need. Given the frequency of composition-nested-composition patterns in the catalog, is the plan to bundle those into a follow-up alongside TODO(template-var-carriers), or interleave with whatever the U3 unit is? Not to solve here — just noting that with those two closed the parity contract's "assembled" subset stops leaking to unit-only gates.
Series note across today's HF PRs: this is Miguel's fifth open one in ~6 hours (#3091, #3092, #3094, #3096, #3097), and #3094→#3097 is a coherent two-step (introduce module + gate; then wire it and settle the divergences the gate exposed). Reading these back-to-back is honestly the ideal reviewer shape for parity work — the "what changed and why" trace between commits is stronger than the trace within any one PR. LGTM from my side.
Review asked to either drop it or have the caller use it. Adopting it would be wrong rather than merely churn: the field is the chain including the host's own src, and the queue loop appends that src again when it processes the host. Threading it through would duplicate the entry, inflating the depth cap by one and letting circular detection fire on a host's own children. The caller derives the chain where it needs it, which is correct and stays. That leaves the field with no consumer, which is what the dead-code gate exists to catch. The unit that gives the mount path recursion will need an ancestry and can add one back shaped to how it actually threads it. The enumeration test keeps everything the function still promises -- which hosts come back, in what order, that the element is the real host, that nothing was skipped -- and stops asserting the field.
|
Both points addressed. Thank you for tracing all four against the code rather than against the body — the D3 catch in particular is why I wanted a reviewer on this. The The On your question about sequencing. The variable-carrier Core 1694, producer 574, lint clean after the change. |
3 participants
Why
#3094 fixed one way the mount and render paths disagreed, and added the gate that catches disagreement. It deliberately left the rest.
Four divergences are still live. Each one means a composition assembles differently depending on whether it is being previewed or rendered — the same class of defect that shipped three catalog components unstyled, just with smaller blast radii.
How
Both paths now derive root discovery, scope identity, asset sources and order, hoisted links, variable carriers and nested-host enumeration from the shared module #3094 introduced. Each keeps its own I/O, which is where they genuinely differ. The compiler's local depth cap and root lookup and the runtime's three pre-filtered head parameters are gone; the runtime hands over the head node and lets the module decide what comes out of it.
Four behaviour changes, each stated by what actually differs rather than by the edit:
Inline
<head>scripts. The compiler looped head scripts with asrcbranch and noelse, so an inline one was silently discarded on render while the runtime ran it. That is losing code, not holding a convention — the runtime's answer wins. Head and content scripts now share one loop, head first, order preserved. A non-templated sub-composition with an inline head script went from 0 collected scripts to 1, wrapped, body intact.<link>hoisting. Conditional on render, unconditional on mount, so a templated sub-composition's webfont link was dropped in video and kept in preview. Hoisting is the superset and matches what the author declared. A templated composition with a stylesheet link went from no external links to that link. The parity fixture that previously recorded this shape as a known exclusion now gates it.Anonymous hosts. With a host naming no id, the compiler fell back to the first declared composition and scoped to it; the mount left the content unflattened and injected its stylesheet into the host
<head>unscoped, so a composition's CSS leaked into whatever mounted it. The compiler's answer wins. The injected rule went from a bare.label { … }to[data-composition-id="scoped-text"] .label { … }.Scope ids. The compiler splits the CSS scope id from the script composition id; they differ only when a host names an id the content does not declare, and there the scripts follow the declared id so their self-referencing queries resolve. The runtime used one for both. The split wins: a host naming
captions-compover content declaringcaptionsnow emits scripts bound tocaptionswhile its CSS still scopes tocaptions-comp.Test plan
Core 1694 passing, producer 574 passing, the parity contract now gates the two divergences it can observe (the other two carry no contract field, so they are gated by unit tests naming the exact before/after). Lint 0,
typecheck:runtimeand the runtime preview guards clean, package cycles unchanged.Characterization-first: both suites were run and recorded green before any decision moved, so a behavioural drift would surface as a red test rather than a silent difference.
One assertion changed, deliberately. A runtime test asserted that an anonymous host's composition is not flattened, and documented that as intentional. That premise is now false. What the test actually cared about — the root and its content present under the host — still holds and is still asserted; the "not flattened" claim flipped, and the test now also asserts the scoping that was missing.
Not covered
The variable-carrier divergence and its
TODO(template-var-carriers)are untouched by design, as is recursion on the mount path — a sub-composition containing its owndata-composition-srcis still silently dropped in live preview. Both are behaviour changes with their own units, and both are now one-line-ish changes because the shared module already reports what they need.runtimeScopeCompositionIdno longer falls back to the authored scope id. This is a functional change beyond the four above, surfaced in review: for an anonymous host with authored variable defaults, the runtime previously stashed them under the declared id, and now does not. It removes a runtime-vs-compiler divergence in the correct direction — the runtime was doing work the compiler never did, and the compiler is authoritative for a shipped composition — but a caller relying on runtime-only variable exposure loses it.The three copies each of the flattened-root helper and the id assignment are left alone: they look mergeable and are not cheaply, and they touch the instancing contract the pixel harness guards.
Worth knowing
The parity test's compiler arms import core's built dist while the mount arm imports source, so core must be rebuilt before that lane means anything after a compiler change. Skipping it produces a phantom divergence that looks exactly like a real one.