ComponentABC: auto-discover <template id> for multi-instance reuse#328
ComponentABC: auto-discover <template id> for multi-instance reuse#328brianmhunt wants to merge 4 commits intomainfrom
Conversation
|
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 52 minutes and 54 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 (3)
✨ 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: 6a9aab6079
ℹ️ 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".
| const autoElement = typeof document !== 'undefined' ? document.getElementById(this.customElementName) : null | ||
| return autoElement ? { element: autoElement } : undefined |
There was a problem hiding this comment.
<template> for auto-discovered component IDs
The new auto-discovery path accepts any element returned by document.getElementById, but the behavior is documented and tested as <template id="$customElementName"> discovery. If an app already has a non-template element with that ID (for example a layout <div>), registration will silently stop using children-as-template and instead clone that unrelated node’s children, changing rendered output unexpectedly. Restrict this branch to tagName === 'template' (or equivalent) so non-template ID collisions still fall back to children-as-template.
Useful? React with 👍 / 👎.
…reuse
Marries PR 326's children-as-template with multi-instance reuse via a
convention-over-configuration default in ComponentABC.
Resolution order (first match wins):
1. prototype has `template` → instance hook
2. subclass overrides `static get template()` → use that
3. subclass overrides `static get element()` → { element: this.element }
4. document.getElementById(customElementName) exists → use it
5. otherwise undefined → children-as-template (from PR 326)
Developer experience for shared markup across many instances becomes:
<template id="hello-world">
<h1 data-bind="text: name"></h1>
</template>
<hello-world></hello-world>
<hello-world></hello-world>
<hello-world></hello-world>
class HelloWorld extends ko.Component {
name = ko.observable('world')
}
HelloWorld.register()
No `static get template`, no explicit id wiring — the id matches the
custom element name by convention. Per-instance children-as-template
from PR 326 is the fallback when no matching <template id> exists.
Tests:
- auto-discovers <template id> when no template/element is set (2 instances share)
- falls back to children-as-template when no matching id exists
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Locality wins. Previous order put the shared <template id> default above the instance's own children, but children-as-template is more local and should take precedence. New resolution order (in componentBinding.applyComponentDefinition): 1. Explicit config template → use it (discards children, unchanged) 2. viewModel.template → use it (unchanged) 3. Instance has meaningful children → children-as-template 4. <template id="\$componentName"> exists → use it (shared fallback) 5. Else throw Moved the id lookup from ComponentABC.template (registration-time) to componentBinding (bind-time) so it falls through AFTER the children check. ComponentABC no longer does auto-discovery itself — keeps the "explicit config wins" semantics for anyone setting static get template or static get element. New test: two <test-component> instances, one bare + one with children. The bare one renders from the shared <template id>; the one with children renders its children instead. Both coexist. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Hot path — runs on every no-template component bind. .some() allocates a closure per call; tight for loop avoids it and is friendlier to V8's inlining heuristics for small method bodies. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two sources of `as any` in the children-as-template work:
1. ComponentABCBehaviors spec imported from '../dist' (no .d.ts files
there, so everything came through as any). Every test then cast
class references back to any to reach .register(). Fix: import from
'@tko/utils.component' (tsconfig path → source) so types flow.
Dropping the casts surfaced real issues the casts were hiding:
- static-getter overrides need `override` modifier (noImplicitOverride)
- dispose() override also needs `override`
- constructor(...args) super-spread is unsound — replaced with class
field initializers, which are cleaner anyway.
2. findSharedTemplate inspected `el.content` with triple `as any`. Fixed
with `el instanceof HTMLTemplateElement`, which narrows to the real
DOM interface where `.content: DocumentFragment` is typed.
Net: 11 `as any` casts removed from the spec, 3 from the source. No
behavior change, just better type coverage.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
brianmhunt
force-pushed
the
component/auto-template-by-name
branch
from
517f93c to
2f2ded4
Compare
| n => n.nodeType === Node.ELEMENT_NODE || (n.nodeType === Node.TEXT_NODE && (n.nodeValue ?? '').trim().length > 0) | ||
| ) | ||
| const nodes = this.originalChildNodes | ||
| for (let i = 0; i < nodes.length; i++) { |
There was a problem hiding this comment.
const nodes = this.originalChildNodes;
// 1. Cache the length
for (let i = 0, len = nodes.length; i < len; i++) {
const n = nodes[i];
if (n.nodeType === Node.ELEMENT_NODE) return true;
// 2 & 3. Drop '??' and use Regex to avoid string allocation
if (n.nodeType === Node.TEXT_NODE && /\S/.test(n.nodeValue)) {
return true;
}
}
There was a problem hiding this comment.
or
for (const n of this.originalChildNodes) {
if (n.nodeType === Node.ELEMENT_NODE) return true;
if (n.nodeType === Node.TEXT_NODE && /\S/.test(n.nodeValue)) return true;
}
There was a problem hiding this comment.
ifalways with{ }(But tokens cost money)ofis modern- Drop
??is good, but perfectly fine for simple things
|
We should define an option for the componentConventionalTemplate = (component?: c) => string
// e.g.
componentConventionalTemplate = (c) => `ko-${c.elementName}` |
| */ | ||
| findSharedTemplate(componentName: string): Node[] | null { | ||
| if (typeof document === 'undefined') return null | ||
| const el = document.getElementById(componentName) |
There was a problem hiding this comment.
just use document?.getElementByID; no typeof
2 participants
Summary
Stacks on #326. Combines PR 326's children-as-template (single-instance, inline) with multi-instance reuse via a simple convention:
<template id="\$customElementName">is auto-discovered.All three
<hello-world>instances share the template by id. No boilerplate. Explicitstatic get template/static get elementstill works when you want to override.Resolution order in ComponentABC.template
'template' in prototype→ instance hookstatic get template()→ use thatstatic get element()→{ element: this.element }document.getElementById(customElementName)exists → use itWhy this is the right marriage
The user asked: "how do we marry using one's own children with re-use of those children in multiple components?" Three options considered:
static get template() { return { element: 'hello-world' } }) — works today, but boilerplate.Tests
Two new tests in
ComponentABCBehaviors:auto-discovers <template id="\$customElementName"> when no template or element is set— two instances render identically from the shared template.falls back to children-as-template when no matching <template id> exists— inline markup still works per-instance.Full suite: 2700 passed, 42 skipped, 0 failed.
Test plan
bun run testgreen🤖 Generated with Claude Code