Conversation
Adds 454 tests across 8 surfaces of the Template runtime (packages/templating/src/template.js): events DSL, lifecycle, callback params, key bindings, DOM scoping, data context, subtemplate settings, tree traversal. 30 tests are expected-fail pins documenting confirmed bugs surfaced by the coverage campaign. Subsequent commits fix each bug; the corresponding pin tests turn green commit by commit. Includes shared scaffolding at packages/templating/test/_helpers/: stub engine, fresh-Template fixture, browser shadow-DOM mounting, registry cleanup, synthetic event/key dispatch. Surface 2 also revives the previously-commented lifecycle integration tests in packages/component/test/browser/component.test.js (replacing lines 461-608's TODO block with a 14-test Lifecycle Events describe).
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
⚪ No Meaningful Change for
|
| metric | Change |
|---|---|
add-20 |
-0.1% – +0.0% |
bulk-add-500 |
-0.9% – +1.5% |
create-10k |
-0.2% – +0.5% |
create-1k |
-1.2% – +0.5% |
edit-cycle-5 |
-0.8% – +0.2% |
filter-cycle-20 |
-0.6% – +0.4% |
hydrate-each-100 |
-1.9% – +2.0% |
remove-10-middle |
-0.2% – +1.9% |
remove-first-10 |
-0.1% – +1.5% |
remove-last-10 |
-0.2% – +0.1% |
remove-middle-10 |
-0.5% – +0.3% |
remove-row-back-10 |
-1.6% – +0.4% |
remove-row-front-20 |
-0.7% – +1.4% |
remove-row-middle-20 |
-0.8% – +1.3% |
replace-1k |
-1.6% – +0.2% |
select-40 |
-0.6% – +1.3% |
swap-rows-20 |
-1.1% – +1.1% |
toggle-10 |
+0.3% – +1.1% |
toggle-all-20 |
-0.1% – +0.1% |
toggle-first-10 |
-0.3% – +0.1% |
toggle-last-10 |
-1.2% – -0.6% |
toggle-middle-10 |
-0.7% – -0.1% |
update-10th-10 |
-2.0% – +1.5% |
🔍 Unsure (6)
Inconclusive (4)
The measured difference is small, and our sampling couldn't confidently place it above or below zero. Running more samples in a future run might settle these metrics.
| metric | Change | Expected Noise |
|---|---|---|
append-1k |
-3.2% – +1.7% | ±1% |
clear-10k |
-1.7% – +4.8% | ±1% |
edit-start-10 |
-3.0% – -0.7% | ±1% |
remove-5-back |
+1.5% – +7.7% | ±2% |
Too Fast to Measure Precisely (2)
On benches this short, system jitter (scheduling, GC, JIT) masks sub-4% changes; larger deltas still resolve cleanly.
| metric | Change | Test Time | Expected Noise |
|---|---|---|---|
clear-completed-250 |
+0.7% – +2.4% | ~58ms | ±3% |
remove-5-front |
-0.1% – +2.4% | ~89ms | ±2% |
Sample size: 50 · Resolution floor: ±2% · Timeout: 3min · Wall-clock: 18m23s
The previous test commit shipped a `_helpers/` folder with a stubbed
rendering engine, browser fixture, and other faked scaffolding. That
pattern bypasses the real Renderer the user hits, defeating the point
of testing.
Tests now use:
- `new Template({...})` directly for pure-logic surfaces
- Real `Renderer` + `ServerRenderer` from `@semantic-ui/renderer`
for tests that need `initialize()` / `attach()`
- Light DOM (plain element as renderRoot) by default
- Shadow DOM only where boundary semantics are the test target
(Surface 5 + the deep keyword in Surface 1, parameterized via
`RENDER_TARGETS.forEach(({ name, target }) => describe(name, ...))`)
Cross-package test deps are normal for tightly-coupled packages —
templating tests reach into `@semantic-ui/renderer` directly without
needing `defineComponent`.
All 30 bug pins preserved; 0 flips:
- B1 falsy override (4)
- B2a + B2b lifecyclePromise (2 in templating, 2 in component)
- B3 cascade leak (8)
- B4 keys whitespace (2 via it.fails)
- B5 kebab lookup (6)
- B6 setParent/removeParent (4)
- B7 value || vs ?? (1)
- B8 deep keyword (1)
Adds 6 new lifecycle-args tests in callback-params.test.js to pin
that $/state/settings/findParent/etc. reach lifecycle hooks correctly.
Surface 5 (DOM scoping) now parameterized over light/shadow render
targets (48 tests vs 26 — same logic, two modes plus 4 shadow-only).
Surface 1 (events) similarly parameterized for 31 tests × 2 modes
plus 14 unparameterized parser/bubble-map plus 3 shadow-only.
`_helpers/` directory removed entirely — no shared test infrastructure
beyond inline local helpers per file.
The events guide promises default-mode handlers don't fire on slotted content. The mechanism — `isNodeInTemplate` range check at line 538 — only activates when `startNode`/`endNode` markers are set, which the renderer does for subtemplates but `WebComponentBase.attach()` does not for top-level components. Top-level filter was inert. Adds a `Node.contains()` check on `template.renderRoot` for default and `bind` event types. Slotted content (light-DOM, not a descendant of the shadow root) is rejected. Subtemplate path is unaffected — the range filter still does the precise scoping; the new gate passes because subtemplates share the parent's renderRoot. Tests pin both shapes: - Q3 PIN: slotted button click does NOT fire default handler - Q3 sanity: shadow-internal button click DOES fire default handler
`targetElement?.value || event.target?.value || event?.detail?.value` clobbered falsy-but-real values to undefined when an earlier operand was a falsy non-undefined and a later one would have provided a real value. Most visibly: empty-string `<input>` (cleared field) delivered `value: undefined` to handlers instead of `value: ''` — confusing for form code that expected the empty string. Switching to `??` short-circuits only on null/undefined; falsy values (0, false, '') flow through correctly. The 3 cases that passed under `||`'s last-operand semantics still pass.
createReactiveState used `if (dataValue)` to decide whether passed
data overrides the default for a state key. Falsy values (0, false,
'', null) silently skipped the override — `defaultState: { count: 5 }`
rendered as `{>child count=0}` initialized state.count to 5, not 0.
The same falsy-clobbering pattern lived on the next line in the
fallback resolution `config?.value || config`. For complex configs
(`{ value: 0, options: {} }`) this would have returned the whole
config object instead of the value 0. Same bug class, switched to
`??` for consistency.
Subtemplate settings (line 933) already uses `!== undefined` —
both paths now agree on falsy-override semantics.
`keySequence.split(',')` left whitespace in the alternates, so
`keys: { 'up, down': handler }` produced `['up', ' down']`. The
leading-space alternate would only match after some prior keystroke
left a trailing space in the sliding-window buffer; pressing ArrowDown
from a fresh buffer never matched ' down'.
Trim each split alternate. Same fix handles padded forms like
`'up , down '`.
defineComponent normalizes tagName to camelCase via kebabToCamel before
storing it as templateName. Looking up via the kebab form
(`findParent('ui-panels')`, `findTemplate('ui-panel')`) missed because
the find* statics did strict equality against the camel registry key.
Normalize the input at the entry of each static (findTemplate,
findParentTemplate, findChildTemplates). kebabToCamel is idempotent
on already-camel input, so both forms now reach the same registry key.
Single chokepoint per method; binders stay as plain pass-throughs.
The line-538 range filter only excepted `global`. Deep events whose
target lived outside the template's startNode/endNode range (slotted
content, nested child shadow DOM) were rejected before line 544's
deep-aware filter could let them through. Net effect: `events: { 'deep
click .x': handler }` did NOT fire on slotted matches inside a
subtemplate.
Mirror line 544's idiom — except both `deep` and `global` from the
range check. The semantics now match: range filter applies to default
and `bind` modes; `deep` and `global` opt out by design.
findTemplate, findParent, findChild, findChildren previously spread the template's instance with its closure data (subtemplate cascade) or its merged dataContext (DOM cascade). The merged dataContext exposes state Signals; the closure data leaks values the parent passed in. Neither was the documented contract. The parallel `.component()` / `.dataContext()` Query methods are deliberately separate — `component` returns the createComponent factory output (the public API surface), `dataContext` is for the rare case you need internals. find* should match `component`'s contract: just the instance. Strip the data/dataContext spreads from all five find* paths (findTemplate, findParentTemplate DOM cascade, findParentTemplate subtemplate cascade, findChildTemplates DOM cascade, findChildTemplates subtemplate cascade). Stale test that pinned the old leaky shape now asserts the leak is gone.
setParent could be called twice with the same parent and push the
child twice, or called with a new parent without detaching from the
old one — child stayed in BOTH parents' _childTemplates. removeParent
filtered out of the parent's array but never cleared this.parentTemplate,
so isSubtemplate() lied after destroy.
Latent today (the renderer's cloneInstance is the only production
caller and goes through one setParent per Template lifetime), but
subtree caching and in-place subtemplate updates will exercise re-
attachment soon. Heap-leak class — destroyed children holding
parentTemplate keep the parent reachable through the cycle.
setParent now does an O(1) idempotency check and detaches from any
prior parent before wiring the new one. removeParent clears
this.parentTemplate. Constructor stops auto-wiring parentTemplate —
setParent is the single source of truth for both directions of the
parent/child link.
Behavioral change: `new Template({ parentTemplate: X })` directly,
without a setParent call, no longer counts as a subtemplate. Pre-1.0
edge path; the renderer's cloneInstance always follows clone with
setParent, so production paths are unaffected.
bindKey checked `Object.keys(this.keys).length == 0` to decide whether
bindKeys() needed to install document listeners. The check was meant
to handle "first key registered" but it also fired after every full
unbind/rebind cycle: unbindKey only deletes from this.keys, doesn't
abort the listeners. So bindKey('a',h1) → unbindKey('a') →
bindKey('b',h2) installed a SECOND keydown listener atop the still-
alive first one, and h2 fired twice per keystroke.
Add a `_keysListenersInstalled` flag that bindKeys() checks for
idempotency. Cleared in removeEvents so a new attach cycle can
install fresh listeners against the new eventController.
- Rename `_keysListenersInstalled` to `hasKeybindings` (no underscore
prefix; name describes the meaning, not the tracker)
- Drop `name && kebabToCamel(name)` guard at find* entries —
kebabToCamel(undefined) returns '' via its default param, and
downstream `if (templateName && ...)` checks treat '' the same as
undefined ("match any")
- Trim verbose pin-context comments and restate-the-code comments
added during the bug fixes
Strip campaign-internal artifacts from the Template test surface so the files read cleanly as open-source contracts: - Drop bug IDs (B1–B12, Q1, Q3), pin terminology (PIN, EXPECTED FAIL), stage labels, surface numbers, convergent/finding labels, and workspace path references from test names and comments. - Test names now read as release-note-style behavior or contract descriptions, not implementation jargon. - Group tests under concept-level describe blocks; merge duplicate describes where they overlapped. - Apply the three-level comment hierarchy from the code-formatting guide; remove restate-the-code comments; keep only non-obvious WHY notes (Vite/Svelte voice). - File headers match the minimal style of test/template.test.js. No test logic, assertions, or fixtures changed. Same 461 passing, 2 expected-fail (lifecyclePromise — fix in flight), 2 skipped. ~500 lines net reduction across 11 files.
The lit render-template directive called setParent AFTER attach, which relied on Template's constructor auto-wiring parentTemplate so that isSubtemplate() was already true by the time attach()'s initialize() ran. With setParent now the sole authority for parent wiring, the late setParent call meant initialize() saw isSubtemplate() === false and skipped createSubtemplateSettings — breaking subtemplate settings reactivity for any component using the lit engine. Reorder: setElement → setParent → attach. Matches the native engine's canonical sequence and the tested clone+wire+initialize flow.
resolveLifecyclePromise was called from inside dispatchEvent. Two consequences hit consumers: 1. Late awaiters hung. lifecyclePromise(name) lazy-created a Promise + resolver pair on first access. If the consumer awaited el.created AFTER the event already fired without prior access, the new pair was created against an already-fired event — promise never resolved. 2. Hydration suppressed promise resolution. The lifecycle wrappers gate dispatchEvent on !isHydrating to avoid re-broadcasting DOM events during DSD hydration. Promise resolution was incidentally gated too — every awaiter of el.created / el.rendered hung during hydration even though the component was rendered. Fix: call resolveLifecyclePromise from the lifecycle wrappers (onCreated, onRendered, onUpdated, onDestroyed) directly, not from dispatchEvent. Cache an immediately-resolved promise on first fire so late awaiters get a non-hanging result. dispatchEvent is now a plain DOM event helper.
The constructor accepted onUpdated but never stored it as onUpdatedCallback, so user-provided onUpdated functions were silently dropped. Manual invocation of the wrapper would fire only the DOM event, not the user callback. Store onUpdated alongside the other lifecycle callbacks; have the wrapper invoke it inside the microtask debounce. Comment notes that the state-Reaction only tracks defaultState Signals — fine-grained block-level reactivity does not flow through this hook, so consumers needing universal coverage should observe the `updated` DOM event or invoke the wrapper manually.
A no-selector handler ('click', 'mouseover', etc) bound at renderRoot
missed events on the host's own surface — clicks on padding/border or
host-dispatched events don't enter the shadow tree's bubble path. The
docs lead with mouseover hovering 'any part of the component'; binding
on the host matches that semantic.
'global hashchange' / 'global scroll' (no selector) now binds to window implicitly. Authors no longer need to repeat the obvious 'global scroll window' for the typical page-level event use cases.
The 4th arg destructured only eventSettings/querySettings, silently dropping passive/capture/once when passed in the natural shape. Production at inpage-menu.js:269 was in the broken shape today and only escaped via query.js's autoPassive for scroll/resize. Reshape: querySettings stays namespaced; everything else (passive, capture, once, signal, ...) is collected via rest into eventSettings and forwarded to addEventListener through Query's .on().
A bare-comma descriptor ('keys: { ",": fn }') split into two empty
strings, and endsWith('') is always true — so the handler fired on
every keystroke. Filter empties out of the alternates list.
$ from inside onCreated (which fires before attach() sets renderRoot) threw 'root.querySelectorAll is not a function' because the globalThis fallback was Window, which doesn't implement ParentNode. Use document on the client so authors can query the surrounding page from onCreated as the design intended.
…hurn
removeTemplate set(name, []) on the empty array — keys never deleted.
With monotonically-increasing names (Anonymous #N, route-driven distinct
components), the Map grew forever. delete the key when its templates
list empties.
Also guards findTemplate(null) at entry — kebabToCamel's default param
covers undefined but null bypasses defaults and threw on .split('-').
findTemplate, findParent, findChild, and findChildren returned just the
instance, breaking real use cases that read state from the result —
e.g. CodePlaygroundPreview's findParent('codePlayground').currentFiles.
Restore the merge: DOM cascade spreads parentNode.dataContext (state
signals exposed via the host's runtime context); subtemplate cascade
spreads template.data (which accumulates the runtime context after
the first render through setDataContext). Pin tests flipped from
'no leak' to 'exposes parent awareness'.
$$ rooted at a shadow root missed slotted nodes when the slotted node itself matched the selector — the assignedNodes branch recursed into each slotted node but never tested it via .matches() before recursing. Only descendants got picked up. Add the explicit .matches() check on each slotted node so the slotted root is a first-class candidate.
cloneInstance in both engines passed 'subTemplates: parent_registry' to
template.clone(), overriding the prototype's own subTemplates declaration.
A child that referenced its own grandchild via {>grandchild} silently
failed to render because the cloned child inherited the parent's
registry instead of its own.
Drop the override line in both engines. Template.clone() already defaults
to the prototype's own subTemplates; without the override, each cloned
level uses its own declaration. No inheritance — each level owns what it
references.
Hour-long stuck Vitest watcher sessions (and their Playwright/Chromium children) cause spurious 'Failed to fetch dynamically imported module' errors via port collisions, plus outright suite hangs. Document the ~1m30s suite budget, the 2-minute timeout heuristic, and the kill steps so the next agent doesn't burn cycles assuming flakiness.
Full repo suite is ~28s (3624 tests / 82 files). 60s timeout is the right alarm threshold, not 2m.
Adds two pins exercising self-referencing subTemplates after S7-G1:
- Recursive with base case: a tree node references itself in its own
subTemplates and uses an {#if children.length} guard. Three-level
data renders all labels.
- Cyclic without base case: two components reference each other with
no terminator. The renderer's recursion hits the V8 stack limit and
throws RangeError — bounded failure, captured via window.error.
Documents the failure mode so authors know to include a guard.
findParent's parent-walk and findChildren's recursive search trusted that the parent/child links formed an acyclic tree. A self-referential parentTemplate caused findParent to infinite-loop; a self-referential _childTemplates caused findChildren to blow the V8 stack with RangeError. Misuse-only, but the failure modes are bad enough (page hang, browser crash) to warrant guards. Add a visited Set to each: findParent breaks when it re-encounters a template; findChildren skips already-visited subtrees.
After onDestroyed, the parent's traversal helpers (findChild, findChildren) still surfaced live children because _childTemplates was never cleared. Drop the array — children outlive the parent only via their own refs, not via stale traversal through a dead parent. Also trims the comments on the cycle guards added in the prior commit; the visited Sets and break conditions are self-explanatory in context.
Group the boolean/state resets (rendered, destroyed, hasKeybindings, _childTemplates) together at the top of the destroy sequence before the side-effecting cleanups (abort, clearReactions, removeEvents, removeObservers, removeParent). Reads as one unit and matches the pattern of state-then-effects elsewhere.
The destroy-time flag resets get a named method paired with the existing markRendered(): markDestroyed() handles rendered/destroyed/ hasKeybindings/_childTemplates as one unit. onDestroyed reads as 'mark destroyed, then run the side-effect cleanups.'
Removes the multi-line block above onUpdated, the multi-line attachEvent docstring, the isServer 'mirror of utils' comment, the parentTemplate 'wired through setParent only' note (already obvious from the setParent method), and the 'event delegation at the shadow root' line that just restated the next line of code. Trims the naked-event and bindKey-gate comments to one line each. Per repo convention: source comments are for non-obvious WHY only — short, load-bearing, vite/svelte voice.
Catches up the docs to the source state after the campaign: - events.mdx: el vs target args table, isDeep description, projection vs piercing on Deep Events, host binding for component-wide handlers, global default to window. Composed callout points at the framework dispatchEvent helper which sets composed: true by default. - keys.mdx + lifecycle.mdx: event param added to callback tables. - lifecycle.mdx: SSR section distinguishes callbacks (fire same order) from DOM events (suppressed during hydration). - dom.mdx: $ vs $$ example reframed for in-component scope. - instances.mdx + examples/templates/subtemplates/row.js: findParent examples in camelCase form. - subtemplates.mdx: parent-fallback path for subtemplate settings. - api/templating/template.mdx: clone() reframed as prototype-to-instance manifestation; onUpdated dropped from the Options table. - authoring/component-events skill: bubble-map adds load and unload rows to the Non-Bubbling Event Mapping table. - authoring/component-state skill: state vs settings precedence inverted to match overlaySettingsSignals reality. Source-of-truth lists: ai/workspace/template-coverage/stage-1-5-batch.md (D1-D10) and stage-3-5-verified.md (Stage 3.5 extras).
Five pins exercising the Template-tier reactive surface end-to-end: the rendered DOM contains the unwrapped value (not the Signal object), state.x.set updates the DOM, and the mutation helpers (.increment, .toggle, .push) propagate through the renderer to the rendered output.
Sweeps the doc cleanup batch for AI-tell patterns: cuts the gratuitous 'projection vs piercing' cross-link callout, the camelCase note on findParent (already shown by the example), the 'manifests a new instance' verbosity around clone(). Trims the Component-Wide, Global, and Composed callouts in events.mdx, the SSR section in lifecycle.mdx, the Inherited Settings section in subtemplates.mdx, and the state-vs-settings explanation in the component-state skill. Replaces all added em dashes with periods, semicolons, or rephrased sentences. The Global example now shows the no-selector default to window via an inline comment rather than a paragraph above.
Adds the patterns we had to fix in PR #174's first-draft body: - New 'Internals' layer in the bullet-layer table, with the 'would this mean anything to a reader who hasn't read the diff' check. - Tell #6 'Verb-first mechanism frames' — Let X / Stop Y / Wire Z / Make Z openers with bad-good rewrite table. - Tell #7 'Internal symbols and line numbers' — line-538, internal field names, alongside-X cross-references with rewrite table. - 'Subgroup long sections' rule: bold sub-labels above ~8 bullets, not heading levels (navigation generator reserves those). - Voice-check item in the post-draft checklist: read aloud, imagine texting the reviewer, watch for the new tells specifically.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR improves coverage of templating package. Any bugs discovered during testing were added as fixes in this PR.
Changes
Templates
Events
deepevents fire on slotted content (was filtered out)'deepclick'no longer parses asdeep+click(word boundary)attachEventignoring passed-through listener optionsvaluecallback arg clobbering empty-string and other falsy valuesKeys
bindKey/unbindKeycycles from stacking duplicate document listenerskeys: { 'up, down': h }comma-listskeys: { ',': h }no longer fires on every keystrokeTree traversal & wiring
find*helpers accept kebab tag-names:findParent('ui-panels')findParent/findChildexpose parent state and data alongside the instancefindParent/findChildrenno longer hang on self-referential cyclesfindChildreturns nothing instead of stale childrensetParentis the sole authority for parent wiring; idempotent and detaches firstLifecycle
$()inonCreatedno longer throws (queries document)lifecyclePromiseno longer hangs on hydration paths or late awaitersonUpdatedcallback now actually firesTemplate.renderedTemplatesmap growing forever on unique-name churnData
defaultStateoverriding falsy values (0,'',false) passed in as dataRenderer
setParentbeforeattachso subtemplate settings init correctlysubTemplatesare no longer overridden by the parent's registryQuery
$$no longer ignores the slot root when matchingFeat
'global scroll'(no selector) now defaults towindowRefactor
Renderer/ServerRendereronDestroyed; extractmarkDestroyed()mirroringmarkRendered()hasKeybindings; trim over-explanatory commentsTests
state.x.push/toggle/incrementpropagation to rendered DOMDocs
Harness
testingskill to explain how to clean up stale test runnersRisk
7/10. Touches Template event/lifecycle/find* and the Renderer clone path. Most fixes have direct pin tests; a few cross-engine paths (lit subtemplate ordering, slotted
querySelectorAllDeep) ride through component tests. Potential snags worth a careful read:setParentno longer gets added in constructor. This is a contract change for templates.