Component children-as-template: skip the <template id=> when colocating#326
Component children-as-template: skip the <template id=> when colocating#326brianmhunt merged 11 commits intomainfrom
Conversation
When a component is registered without a template (no template in the
config and no static/instance template on the viewModel), the instance
element's own children now serve as the template. The viewModel binds
against them in place — no cloning, no throw.
components.register('ko-greeting', {
viewModel: function () { this.name = ko.observable('TKO') }
})
<ko-greeting>
<input ko-textInput="name" />
<p>Hello, <strong ko-text="name"></strong>.</p>
</ko-greeting>
Colocates markup with its mount point for single-use components. For
reuse with different instances sharing a template, the classic <template
id="..."> + template config still works exactly as before.
Also relaxes ComponentABC: subclasses can omit both `template` and
`element` to opt into children-as-template. The previous "`element`
must be overloaded" error is gone — a missing element just means
"no separate template, use my children".
Added behaviors:
- binding.component: children-as-template with data-bind
- binding.component: children-as-template with native ko- attrs
- binding.component: empty element renders nothing (no throw)
- utils.component: ComponentABC.register() succeeds without overloads
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 6 minutes and 53 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughComponent binding now supports "children-as-template" mode, where a component uses its element's existing child nodes as the template when no explicit template is provided. This required removing the error condition that previously rejected missing templates, making the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0842590218
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (!componentDefinition.template) { | ||
| this.cloneTemplateIntoElement(componentName, viewTemplate, element) | ||
| if (viewTemplate) { | ||
| this.cloneTemplateIntoElement(componentName, viewTemplate, element) | ||
| } |
There was a problem hiding this comment.
When componentDefinition.template is missing and viewTemplate is falsy, this branch leaves the current DOM children in place and immediately re-applies descendant bindings. That works only for the first render; if the component on the same host is rebuilt (e.g., observable name switches away and back, or switches from a templated component to this mode), those nodes are already bound, so binding re-entry triggers You cannot apply bindings multiple times to the same element and the component cannot be rebuilt correctly. This new mode should reset children to fresh clones of the original template nodes before rebinding, just like the templated path does.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Enables “children-as-template” components: when a component is registered without a resolved template (and the instance doesn’t supply one), the component element’s existing children are treated as the template and bound against the component view model.
Changes:
- Remove the hard error for components with no template and allow descendant bindings to run against existing child nodes.
- Update
ComponentABCdefaults to allow omitting bothtemplateandelement(opting into children-as-template mode). - Add/adjust tests covering children-as-template (including
ko-*attribute bindings) and no-template/no-children behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/utils.component/src/ComponentABC.ts | Allows registering components without template/element to opt into children-as-template mode. |
| packages/utils.component/spec/ComponentABCBehaviors.ts | Updates expectations so bare subclasses can register without throwing. |
| packages/binding.component/src/componentBinding.ts | Removes “no template” throw and introduces children-as-template behavior when no templates are provided. |
| packages/binding.component/spec/componentBindingBehaviors.ts | Adds tests for children-as-template (data-bind + ko-* attrs) and no-template/no-children rendering. |
| class CX extends ComponentABC { | ||
| it('registers when neither template nor element is overloaded (children-as-template mode)', function () { | ||
| class CXTwo extends ComponentABC { | ||
| customElementName() { |
There was a problem hiding this comment.
This test class overrides customElementName as an instance method, but ComponentABC.register() reads the static getter customElementName. As written, this doesn’t actually exercise overriding the registered element name, and the test description becomes misleading. Consider changing it to static get customElementName() (or assert the default kebab-case name if that’s what you want to cover).
| customElementName() { | |
| static get customElementName() { |
| } | ||
| // else: no template configured — the element's own children serve as | ||
| // the template. They were captured in originalChildNodes and are still | ||
| // attached to the element, ready for descendant binding below. |
There was a problem hiding this comment.
In children-as-template mode (no componentDefinition.template and no viewModel.template), the binding leaves the element’s current DOM intact and immediately re-runs applyBindingsToDescendants. If the component is rebuilt (e.g., observable component name changes) after previously rendering a templated component, the element will still contain already-bound nodes from the old component, which can trigger the “You cannot apply bindings multiple times…” binding error and/or leave stale markup. Consider explicitly resetting the element’s children for this branch (e.g., restore/clone originalChildNodes, or empty + reinsert them) so a rebuild always starts from a clean, unbound DOM tree.
| } | |
| // else: no template configured — the element's own children serve as | |
| // the template. They were captured in originalChildNodes and are still | |
| // attached to the element, ready for descendant binding below. | |
| } else { | |
| // No template configured — the element's original children serve as | |
| // the template. Restore fresh clones so rebuilds do not rebind stale | |
| // or already-bound DOM left behind by a previous component render. | |
| virtualElements.setDomNodeChildren(element, cloneNodes(this.originalChildNodes)) | |
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/utils.component/spec/ComponentABCBehaviors.ts (1)
67-71:customElementNameshould be astatic getto actually override.
ComponentABC.customElementNameis a static getter, so defining it as an instance method onCXTwodoesn't override anything —register()uses the default kebab-case ofCXTwo('c-x-two'), not'a-b'. The test passes either way, but the intent reads as ineffective.Suggested tweak
class CXTwo extends ComponentABC { - customElementName() { - return 'a-b' - } + static get customElementName() { + return 'a-b' + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/utils.component/spec/ComponentABCBehaviors.ts` around lines 67 - 71, The CXTwo class defines customElementName as an instance method but ComponentABC defines customElementName as a static getter, so CXTwo does not actually override it; change CXTwo.customElementName to a static getter (static get customElementName()) so it correctly overrides ComponentABC and register() will use 'a-b' instead of the default kebab-case name.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/binding.component/src/componentBinding.ts`:
- Around line 150-157: When rebuilding a component whose definition has no
template and no viewTemplate, reattach the originally captured child nodes so
stale cloned nodes from a previous render are removed; in the block that checks
if (!componentDefinition.template) (and where
cloneTemplateIntoElement(componentName, viewTemplate, element) is called when
viewTemplate exists), add logic to detect the absence of both
componentDefinition.template and viewTemplate and then restore the captured
originalChildNodes (the array captured in the constructor) back into element (or
clear element and append originals), e.g. by adding a
restoreOriginalChildren/reattach routine invoked from componentBinding where
element and originalChildNodes are in scope so subsequent swaps to a
"children-as-template" component show the original children.
---
Nitpick comments:
In `@packages/utils.component/spec/ComponentABCBehaviors.ts`:
- Around line 67-71: The CXTwo class defines customElementName as an instance
method but ComponentABC defines customElementName as a static getter, so CXTwo
does not actually override it; change CXTwo.customElementName to a static getter
(static get customElementName()) so it correctly overrides ComponentABC and
register() will use 'a-b' instead of the default kebab-case name.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 225f4100-5510-4a22-b302-bb192c8a91ef
📒 Files selected for processing (4)
packages/binding.component/spec/componentBindingBehaviors.tspackages/binding.component/src/componentBinding.tspackages/utils.component/spec/ComponentABCBehaviors.tspackages/utils.component/src/ComponentABC.ts
Two fixes, one root cause:
(1) builds/knockout/spec has a parallel copy of the component-binding
behaviors that also asserted the old "throws if no template" path.
Updated to match the children-as-template behavior.
(2) The root cause of why this slipped through locally: the knockout-
build specs import ko from the prebuilt bundle
(builds/knockout/dist/browser.min.js). Local \`bunx vitest run\`
uses whatever bundle is on disk — which is stale after a source
change unless you rebuild. CI runs \`bun run build\` first so it
caught the mismatch; locally it silently passed.
Fix: \`bun run test\` now runs \`build && vitest run\`, matching
CI. Added \`test:fast\` as an escape hatch for iterating when you
know the bundles don't need a rebuild.
Also picks up an auto-format fix on the edited spec files.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Measured: bun run build is ~200ms wall (1059% CPU, parallel), vitest run is ~6.9s. Build is ~3% of test time. No latency argument for a skip-build path — one \`test\` script is enough. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
No template + no children is a mistake, not an intentional empty render. Restore the "has no template" throw, but guard it behind a children check: the throw only fires if originalChildNodes are empty (or contain only whitespace text). <my-comp></my-comp> → throws (no template, no children) <my-comp> </my-comp> → throws (whitespace-only counts as empty) <my-comp><p>hi</p></my-comp> → renders children in place Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Test: rendering two sibling instances + a later third instance. Mutations to the first clone (className, textContent) don't bleed into the second sibling or into a subsequently-bound third host. Confirms per-instance template clones are independent of each other and of prior instances. Also drops the inline explanatory block from applyComponentDefinition — inline comments bloat function bodies and can push past inlining heuristics. JSDoc on the helper stays (outside the function body). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
<my-comp>Hello, <strong>{{ name }}</strong>!</my-comp>
Verifies text nodes, mixed elements, and mustache interpolation all
survive the children-in-place render path and stay reactive. Added the
two mustache providers (Text + Attribute) to the test provider stack so
this path actually exercises them.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Rewrote the children-as-template tests to use registered <hello-world> custom elements (via ComponentProvider) instead of the old-style <div data-bind="component: ..."> wrapper. Also filled three gaps: - Configured template wins over inline children (inline content is discarded when the registration supplies a template) - $component / $data / $parent all resolve inside a children-as-template body - Nested components (<outer-comp><inner-comp>...</inner-comp></outer-comp>) both render via children-as-template (ko-component attribute not tested directly: ko-* attributes route through AttributeProvider's strict identifier lookup, so the tag-name path is the more realistic shape for modern examples.) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Biome wanted the .some() predicate on one line. No behavior change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CI runs biome before tests; local \`bun run test\` didn't, so lint errors slipped through until push. Now \`test\` runs \`biome ci\` → build → vitest, matching CI's first three steps. Adds ~50ms — same false-negative argument as the build step. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Used var following the file's prevailing knockout-era style, but new code should use const/let. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Codex (P1) + Copilot + CodeRabbit all flagged the same issue: on a
rebuild (observable component name change) my children-as-template
branch re-ran applyBindingsToDescendants on the element's current
DOM — which by then was the PREVIOUS component's already-bound clones.
Either throws "You cannot apply bindings multiple times" or leaves
stale markup from the prior render.
Fix: always route through cloneTemplateIntoElement for the no-template
path, using the captured originalChildNodes as the template source.
setDomNodeChildren then swaps in fresh clones each render, matching the
templated path's behavior. The originalChildNodes references stay
pristine because the first render never binds against them directly —
only against their clones.
Added a test for the rebuild scenario (observable name: 'comp-a' →
'comp-b', where comp-b uses children-as-template). Verified the
rendered tree flips cleanly without errors.
Also fixed Copilot's other finding: customElementName was defined as an
instance method in one of my tests, so it didn't actually override the
static getter on ComponentABC. Made it `static get`.
Per user request, the new rebuild test uses a class-based viewModel
(`class CompB { msg = 'from-children' }`) — the resolveViewModel path
treats classes as constructors via `new`, so this is a supported shape.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
When a component is registered without a template, the instance's own children now serve as the template. You bind your viewModel against inline markup without needing a separate
<template id="...">or a string template in JS.Good for single-use components that want template + state colocated. For reused components with a shared template, the classic
<template id="...">+static get template()pattern still works unchanged.Changes
@tko/binding.component: when neithercomponentDefinition.templatenorviewModel.templateis set, the element's existing children stay in place and descendant bindings apply with the viewModel context. The prior "Component has no template" throw is gone.@tko/utils.component:ComponentABCno longer throws when subclasses omit bothtemplateandelement; those classes opt into children-as-template. Theelementgetter now returnsundefinedby default instead of throwing.Tests
New behaviors (all passing):
binding.component: children-as-template withdata-bindbinding.component: children-as-template with nativeko-*attrsbinding.component: empty element + no template renders nothing (no throw)utils.component:ComponentABC.register()succeeds on a bare subclassUpdated existing tests that asserted the old throws.
Full suite: 2692 passed, 42 skipped, 0 failed.
Test plan
bunx vitest run— all greenconfig.templateis untouched)viewModel.templateinstance hook still worksstatic get template()orstatic get element()still works🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes